Chrome issue allows two tabs to acquire the lock.

[latentflip]

I'm not sure that the browser test side of this PR is necessarily ready for merge, but I wanted to share it in it's current state to get a fix for this moving along.

There's a bug in fast-mutex in (at least) chrome, which allows two tabs to obtain the lock at the same time.

Chrome doesn't "commit" localStorage changes to "disk" until the current execution stack has finished. This means in the current sequence:

tab one                   |  tab two
setItem("x", "one")       |
                          |  setItem("x", "two")
getItem("x") // => "one"  |

tab one will always read "x" as one, even though tab two has changed it.

Inside fast-mutex, this allows two tabs to acquire the lock concurrently. To handle this, we need to separate set and get by a settimeout call.

To demonstrate the issue (and fix) I have created a browser test page. (Run it with npm run test-browser)

Open it in two tabs (I would recommend putting the tabs in separate windows side-by-side, otherwise chrome has a tendency to not call timeouts more frequently than 1/per second, which confuses things).

This test triggers synchronous runs every :00/:15/:30/:45 seconds past the minute in each tab. In each tab we acquire a lock, and increment a counter. When all tabs are finished, we should see the counter equal the number of open tabs, otherwise there's been a failure (either two tabs acquired the lock concurrently, or one tab failed to complete).

Here is an example of a run before the fix (in sha 67cc985):

With two tabs open, both acquire the lock within 1ms of each other.

[latentflip]

So, while this fixes chrome in the "both tabs are in front" case. I fear it may break chrome and/or other browsers where one browser is not at the forefront, as non-focussed tabs may run setTimeout(0) quite slowly, at which point the 50ms timeout in the contention resolution block is not enough.

Sigh.

[chieffancypants]

First of all, apologies on the delay. That said, if what you're saying is true, this would indeed be a huge issue. Pulling up the localStorage spec from 2016, it mentions thread safety, and actually has it's own internal mutex to ensure it, so it seems surprising.

Your comment regarding chrome not committing the changes until after the stack completes, is this anecdotal or can you pass along more information?

Worst case, I think we can resolve with a StorageEvent to sync the locks, but that may make things much more complicated, so I'd like to avoid if possible.

[latentflip]

Chrome not writing until the stack completes is based on the proof of concept above, but in trying to solve this problem (after I wrote this PR originally) I found threads such as whatwg/html#335 which indicate nobody actually implements the internal mutex. I also experimented with trying to solve this with local storage events. I can’t remember exactly why I couldn’t get it to work reliably. Either events don’t get sent until the value gets written, or they don’t get read in the other tab in time. Ultimately I concluded after a lot of trying that due to the way chrome implements localStorage, it’s not possible to do a safe lock using localStorage. Other browsers fare better as multiple tabs will share an eventloop when running code from the same origin, which means that two tabs _cannot_ run code synchronously avoiding the race, but no joy in chrome. In the end i switched to using indexeddb to build my lock instead which does seem to have much better guarantees (albeit with a bit more complexity). Philip Roberts

…

Le 15 févr. 2018 à 08:24, Wes Cruver ***@***.***> a écrit : First of all, apologies on the delay. That said, if what you're saying is true, this would indeed be a huge issue. Pulling up the localStorage spec from 2016, it mentions thread safety, and actually has it's own internal mutex to ensure it, so it seems surprising. Your comment regarding chrome not committing the changes until after the stack completes, is this anecdotal or can you pass along more information? Worst case, I think we can resolve with a StorageEvent to sync the locks, but that may make things much more complicated, so I'd like to avoid if possible. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[chieffancypants]

Thanks, appreciate the heads up!

[timothycouchhsi]

I also discovered this issue with localstorage caching (or whatever is going on) after much investigation. Could we add a disclaimer on the readme or something that this mutex isn't guaranteed to work on Chrome? Not sure what other browsers as I haven't tested them.

Thanks!