arj03 / ssb-browser-demo

A secure scuttlebutt client interface running in the browser
Other
58 stars 11 forks source link

Concurrency issue - Posting from two different tabs windows does not work #139

Closed KyleMaas closed 3 years ago

KyleMaas commented 3 years ago

If you open two different browser tabs (for example, to look at two different threads) and try to post to one, then switch to the other tab and post there, the messages from one will not show up on the other. It seems to behave as if you've forked your identity.

KyleMaas commented 3 years ago

I've done a little bit of investigation on this.

It looks like the only way to share an SSB object (which I think is going to be the only way to solve the concurrency issue) is to make sure that all "new tab" operations (center-clicking things) are converted into window.open() calls so that we can access window.opener.SSB from the child tab. We will then need to keep a reference around to each of those child windows so that we can notify them if the parent window closes, because we will lose our SSB state and they will need to pause. This would also require that all references to the global SSB object are instead redirected to parent windows if we have them. It's doable, and indirection could be done synchronously, making it relatively easy to drop into place. It's not exactly easy, but it's doable.

Now, the better solution, if we can swing it: Theoretically, it might also be possible for a parent window to pass each child window references to all other child windows so that if the parent window is closed it can elect one child window to be the holder of SSB. After closure, that child window would have to re-initialize SSB (because I don't think we can just pass the reference, and it wouldn't have any of the timers it needs for connection scheduling and such) and then proceed. This is arguably a more robust way to do it if we can pull it off, but it would require that all references to global SSB be converted into an asynchronous callback so that child windows would continue running without crashing while the newly-elected SSB holder reinitializes its SSB object.

But to know if that's even realistic, I'll have to do some more testing.

KyleMaas commented 3 years ago

Looks like another option for getting around window.open() conversion might possibly be rel="opener" on all links, which is a lot more doable. See:

https://developer.mozilla.org/en-US/docs/Web/API/Window/opener

Also appears to be supported in at least recent Firefox and Blink-based browsers, probably also WebKit-based. Good possibility IE does not support it (Edge should). More investigation needed.

KyleMaas commented 3 years ago

rel="opener" is likely not going to work. I missed the fact that it only works for links which are also target="_blank".

To use window.open() instead would mean we'd need to hook the click event on all links so we could catch center clicks as well as contextmenu events so we could hook right-clicking to open a new tab and use window.open() instead. Messy, but doable.

We could also use a variable in localStorage to indicate that a tab is already open and communicating with the server, and then have other tabs look for this when opened and (a) start in offline mode, and (b) block any UI elements which change things. This seems really fragile to me.

Web Workers and Service Workers wouldn't work well for this because we'd be limited to message passing which can't serialize functions, so we can't just pass a global singleton SSB back and forth. Broadcast Channel doesn't look like it'd work for the same reason. Chrome's proposed Web Locks API looks interesting but is not widely supported. We might be able to use Broadcast Channel to tell all other SSBs when a change has been made so they know they need to re-EBT before posting anything, but I'm not 100% convinced that's going to work properly.

So I think our least fragile option is hooking click and contextmenu. It's not very robust, but I'm not finding any really good options for sharing/synchronizing SSB state across multiple tabs.

@arj03 Any ideas?

arj03 commented 3 years ago

Seeing https://github.com/ssb-ngi-pointer/ssb-db2/issues/196 it would probably be a good idea to focus on this. I'll give it a look, I have been pretty busy with a bunch of things included this that should give a nice perf boost.

KyleMaas commented 3 years ago

I was expecting this to cause forks, but corrupt the database. So, yeah, this is a fairly high priority issue, then.

KyleMaas commented 3 years ago

Er, I was expecting it would not corrupt the database.

arj03 commented 3 years ago

I think it's basically the same as starting two programs that runs on the same file with different state in each program, so even if the file is atomic wrt. writes, then the state is not so it doesn't really add up in the end if you read it.

arj03 commented 3 years ago

Linking issues to make this a bit easier to navigate. I'm assuming all of these stem from the same underlying problem of the browser not sharing state:

arj03 commented 3 years ago

That is some really nice research you did there @KyleMaas! Sorry for not responding to this earlier.

I think we need to first have a footgun where we use sessionstorage to see if other tabs are open and if so try to get the state window.opener.SSB. If that is not possible, like if you just open multiple tabs then just refuse to run and tell the user why.

Secondly we could use the window.open like you explained to fix links so that window.opener.SSB works. It should be tested in browser.js, so that part seems like a quite small change. I tested the window.opener.SSB and it actually works pretty great!

Is that something you could work on @KyleMaas? Just so we don't step on toes here :)

KyleMaas commented 3 years ago

Sure thing! I just didn't want to try to tackle it until we knew which route we wanted to go with.

arj03 commented 3 years ago

Sweet!

KyleMaas commented 3 years ago

https://github.com/chieffancypants/fast-mutex

KyleMaas commented 3 years ago

So, my plan for this is as follows:

  1. Add a layer of indirection between everything and SSB so that nothing accesses the SSB object directly. I'd like to do this based on a callback so that if we're a child window and our parent window goes away (and there's a mutex held on SSB), an error can be shown to the user. But if there is nothing holding a mutex lock on SSB, we can initialize one and run the callback, providing for automatic recovery for child windows. Side effect to this is that we could load Vue.js right away, so we don't have the internationalization placeholders showing up on-screen while the SSB object is loading. This provides the footgun, and should be robust enough to handle multiple different potential concurrency problems. We also need to provide the user with some way to override the mutex lock in case the browser crashes, probably by having a watchdog timer running on the active SSB holder and asking the user if they want to run anyway if the watchdog hasn't been updated for 5 minutes or so. Otherwise a dead mutex holder could mean having to clear all state to recover.
  2. Once that indirection is in place, we can proceed to tying in multiple windows. At this point we can integrate window.opener.SSB and the modifications to all of the <a> tags to use a window.open() override. This should allow most child windows to work without footgunning, at least as long as the parent window is open and they were opened from the parent. We should also include the ability to walk up multiple levels of window.opener in case of child windows opened from other child windows. If we have proper indirection in place, this should be able to be done without crashing or corrupting anything even if the window.open() overrides don't work every single time. I should note that I don't expect the overrides to work everywhere and every time - too much of the time, browsers "know better" and just don't let you override clicks like that with 100% reliability. But if we can get it to work 90% of the time and have a fallback that keeps it from crashing the other 10%, I'll be quite happy with this. For one, I'm fairly certain overriding context menus isn't going to prevent the browser from showing the default "Open in new tab" options, at least in Safari and possibly others, and will only show our options at the bottom of the context menu. But in that case, we're back to the fallback in (1) and it should still be a safe operation.
  3. Once that works, we can start having windows build a collection of other open windows to coordinate with in the case that the parent window needs to close. That way, if the parent window closes, a different child window can be elected to hold the SSB object so the child windows don't footgun. I don't expect this to be very difficult once (1) and (2) are in place.

At each step, it should be fairly straightforward and testable. Where we really start running into unknowns is at (2). But this is going to require a lot of changes across pretty much the whole codebase (anywhere that accesses the SSB object), so this is going to be a major undertaking to do correctly and elegantly.

arj03 commented 3 years ago

Hmm I wonder if it would be possible do without the indirection. Another way would be to check if there is a window.opener.SSB in browser.js and use that instead of creating one. If you do it that way, are you sure that the SSB object will go away once the first window closed, there should now be an extra reference? And even if the firs window did go away, maybe that is an acceptable trace-off that makes this a lot easier and less error prone?

KyleMaas commented 3 years ago

I guess we can test that. But I think the indirection route would be more robust, because it would allow for a mutex lock gracefully degrade child windows if they couldn't find a parent instead of just crashing.

KyleMaas commented 3 years ago

Tested in both Chromium and Firefox, and if you create a reference in the child window, the SSB object is persisted even if the parent is closed.

arj03 commented 3 years ago

Nice, that is what I hoped for

KyleMaas commented 3 years ago

Interestingly, multiple child windows can hold their own references to the parent's SSB object even when all but one child window is closed. Which means, somehow, it's also carrying that object across process boundaries.

Now, that said, the timers stop running when you close the parent window. Which means that nothing updates after you close the parent window, connections stop working, and presumably the write debouncing in AAOL would stop, leaving buffered writes unwritten.

So...not perfect. To do it properly without indirection, we'd need to stop all of the child windows as soon as the parent window was closed. We'd have to add another layer on top of things, maybe with a broadcast channel. Which is possible, I guess, but feels less clean to me than a layer of indirection with a mutex to manage that and reinitialize as necessary.

KyleMaas commented 3 years ago

AFAICT it looks like it also stops responding to events from open connections once the parent window is closed. So, yeah, it looks like it works, until you try to actually do something with it. It brings the data over, but little of the functionality.

KyleMaas commented 3 years ago

Any further thoughts on this?

arj03 commented 3 years ago

If we can do the indirection without async I'm all for mutex. I think it would be okay that it said something like, oops parent window closed, reinitializing SSB and then things don't work until it finishes that. This way we could avoid changing everything to async.

KyleMaas commented 3 years ago

I'll see what I can do!

KyleMaas commented 3 years ago

Pretty sure we can close this now.