Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the storage mutex #335

Closed
domenic opened this Issue Nov 13, 2015 · 31 comments

Comments

7 participants
@domenic
Copy link
Member

domenic commented Nov 13, 2015

Nobody has implemented the storage mutex. I think we should not pretend this will eventually happen. Instead we should supplement the spec with appropriate and detailed warnings about how cookies and local/session storage are prone to of multi-window corruption. Including some examples would be good.

If there are alternate patterns that we could recommend, that would be even better. But I think the same problem still occurs in e.g. IndexedDB? Or maybe transactions help there.

Tagging in @inexorabletash as a Blink storage person.

Related: #334 for removing the public API navigator.yieldForStorageUpdates().

@inexorabletash

This comment has been minimized.

Copy link
Member

inexorabletash commented Nov 13, 2015

Indexed DB doesn't have the same problem. Connections and transaction scheduling is well defined (although there's no notification API). I'll take a peek at the storage mutex in HTML and comment if anything in worth salvaging.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 13, 2015

I wonder if @rocallahan still has plans for the perfect implementation of all this.

@rocallahan

This comment has been minimized.

Copy link

rocallahan commented Nov 13, 2015

No, I guess this ship has sailed.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 13, 2015

Sad trombone. @Ms2ger, what about Servo?

@Ms2ger

This comment has been minimized.

Copy link
Member

Ms2ger commented Nov 13, 2015

I don't know if we've really considered it. CC @jdm.

@jdm

This comment has been minimized.

Copy link

jdm commented Nov 13, 2015

I have not put any effort into implementing an equivalent solution to the storage mutex in the spec.

@inexorabletash

This comment has been minimized.

Copy link
Member

inexorabletash commented Nov 13, 2015

Yeah, looks fine to rip it all out, possibly with warnings about raciness sprinkled about. Definitely no impact to Indexed DB or SW's Cache.

The bit about "the length attribute of a Storage object" should probably reinforce the localStorage.length === localStorage.length invariant which should hold even without a mutex i.e. updates to storage from other execution contexts should not be visible to script until the stack unwinds (event loop? microtasks? whatever the current hotness is); we try and ensure that across the platform. I haven't looked at Chromium's code to make sure we actually do that.

(Now would be a good time to solicit feedback on Origin Flags wouldn't it?)

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 13, 2015

@inexorabletash you cannot hold that invariant without a mutex if you use independent processes for multiple same-origin tabs. (Unless perhaps all the storage is in its own process that uses an event loop of sorts. Perhaps that works?)

@inexorabletash

This comment has been minimized.

Copy link
Member

inexorabletash commented Nov 13, 2015

Tabs could maintain snapshots of storage state which are updated only on return to the event loop. Again, I'm not sure if Chromium or other browsers actually do that in any/all cases. If we don't want to require such behavior, then we should call out that such an invariant doesn't hold.

@rocallahan

This comment has been minimized.

Copy link

rocallahan commented Nov 13, 2015

How would you reconcile conflicting updates, then?

@inexorabletash

This comment has been minimized.

Copy link
Member

inexorabletash commented Nov 13, 2015

In Chrome, the source of truth for storage is in its own process. Tabs maintain local caches, observe updates that come in between ticks of the event loop (which in turn produce storage events), and send updates (last writer wins).

@rocallahan

This comment has been minimized.

Copy link

rocallahan commented Nov 13, 2015

Interesting. Is that for localStorage only, or cookies as well? Maybe we should specify that.

@foolip

This comment has been minimized.

Copy link
Member

foolip commented Nov 14, 2015

The section "Serialisability of script execution" should be removed together with this.

@foolip

This comment has been minimized.

Copy link
Member

foolip commented Nov 16, 2015

OK, so in #342 I have blindly removed the storage mutex. What kind of guidance should we add? The affected APIs are localStorage and document.cookie.

@inexorabletash

This comment has been minimized.

Copy link
Member

inexorabletash commented Nov 16, 2015

@rocallahan - re: cookies - they're different (in chrome)

Getting document.cookie does a synchronous IPC (renderer⇢browser) to get the current state.
Setting document.cookie does an asynchronous IPC to set the new cookie.

There's no implicit synchronization mechanism like only updating state at the event loop, so it is possible for to observe cookie state changing during a script execution block. (I have sample code, but it's pretty obvious.)

@foolip

This comment has been minimized.

Copy link
Member

foolip commented Nov 17, 2015

@inexorabletash do you think it's likely to be web compatible to have document.cookie behave like localStorage, with updates only when returning to the event loop?

@rocallahan how do these APIs behave in Gecko, especially with multiprocess enabled?

@rocallahan

This comment has been minimized.

Copy link

rocallahan commented Nov 17, 2015

I don't know. @annevk might know, or might know who knows.

@jdm

This comment has been minimized.

Copy link

jdm commented Nov 17, 2015

Cookie setting and getting are synchronous operations from the child process to the parent, and there's no synchronization logic around updating when returning to the event loop. Storage is based around a process-local cache and async updates from the parent, so I would expect that to be stable during each turn of the event loop.

@inexorabletash

This comment has been minimized.

Copy link
Member

inexorabletash commented Nov 17, 2015

@foolip - My guess is that it's web-compatible, given how subtle it is. It would change the behavior of document.cookie === document.cookie from sometimes failing to always passing, but observing the failure could not be relied on anyway. (I had to have the writing tab hammering the cookie jar with updates and the reading tab busy-waiting between reads.)

That said, I don't think we'd rush to update the code here, although nailing this down before exposing a newfangled cookie API (e.g. to workers) would be a good idea and would merit revisiting the impl.

@foolip

This comment has been minimized.

Copy link
Member

foolip commented Nov 18, 2015

So for localStorage it sounds like at least Blink and Gecko already have the same model: kept stable during script execution, with changes synced when returning to the event loop. Spec'ing that as "sync things" in step 5 ought to work.

For document.cookie, I think the alternatives would be to make it like localStorage, or to spec that both the getter and setter should be synchronous. Preferences?

@inexorabletash said:

Setting document.cookie does an asynchronous IPC to set the new cookie.

I did a very simple test:

console.log(document.cookie);
document.cookie = 'foo=bar';
console.log(document.cookie);

In Chromium, it looks like the second console.log always has the new cookie value, so the setter can't be entirely async. Do you know if this is guaranteed, or am I just lucky with timing?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 18, 2015

Note that document.cookie can also be affected by responses from the network. So I think it would have to be more complicated than synchronous.

@foolip

This comment has been minimized.

Copy link
Member

foolip commented Nov 18, 2015

Can't that be seen as just another source of unpredictable change, like another tab?

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Nov 18, 2015

Yeah, I guess that is fairly similar in that respect.

@foolip

This comment has been minimized.

Copy link
Member

foolip commented Nov 18, 2015

In Chromium, it looks like the second console.log always has the new cookie value, so the setter can't be entirely async.

I guess what's happening is that because the getter is sync, it'll block until the message for the async setter has been processed. Right, @inexorabletash?

@inexorabletash

This comment has been minimized.

Copy link
Member

inexorabletash commented Nov 18, 2015

@foolip Right - async just means we don't wait for a response. Both sync and async IPC get onto the same queue in the receiver, so the async set is guaranteed processed before the async get. (That's implicit but there's even a comment in the code about that)

@foolip

This comment has been minimized.

Copy link
Member

foolip commented Nov 19, 2015

OK, that means that the API will look synchronous to scripts. Do you think there is any combination multiple tabs, out-of-process iframes and communication with them where it could be observed that the setter is using async IPC? If not, it's very tempting to spec document.cookie as a synchronous API.

foolip added a commit that referenced this issue Nov 24, 2015

Remove the storage mutex due to lack of implementation
Add a stern warning for the racy document.cookies API.

Fixes #335

foolip added a commit that referenced this issue Dec 15, 2015

Remove the storage mutex due to lack of implementation
Add a stern warning for the racy document.cookies API.

Fixes #335

foolip added a commit that referenced this issue Dec 15, 2015

Remove the storage mutex due to lack of implementation
Add a stern warning for the racy document.cookies API.

Fixes #335

foolip added a commit that referenced this issue Dec 15, 2015

Remove the storage mutex due to lack of implementation
Add stern warnings for both document.cookies and localStorage.

Fixes #335
@foolip

This comment has been minimized.

Copy link
Member

foolip commented Dec 15, 2015

OK, the wording here was a bit tricky, which is why I left it sitting for so long. I think it's now ready for review again, @annevk @domenic please take another look and find all the things that don't make sense!

@foolip

This comment has been minimized.

Copy link
Member

foolip commented Dec 15, 2015

Note that I didn't try to spec how localStorage works at this point. If anyone thinks that could be made sane, or that it could be interoperably insane, please file a separate issue.

@annevk

This comment has been minimized.

Copy link
Member

annevk commented Dec 15, 2015

The only thing that's somewhat weird is that now we have a warning on cookies, which looks good, but nothing for storage. I guess you did that because strategies differ in implementations. If that's the case, if you open up a new issue for storage so that defining that one way or another gets done in due course, I would be okay with merging this.

@domenic

This comment has been minimized.

Copy link
Member Author

domenic commented Dec 15, 2015

There's a warning for storage analogous to the warning for cookies actually.

@foolip

This comment has been minimized.

Copy link
Member

foolip commented Dec 15, 2015

I have filed #403 to actually define how localStorage works. I don't plan to work on that anytime soon, but the people in this thread could summarize the model of the browsers they work on, that'd be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.