If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Make sure DOM storage acts correctly with multiple content processes

NEW
Assigned to

Status

()

Core
DOM
6 years ago
2 months ago

People

(Reporter: Benjamin Smedberg, Assigned: janv)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [e10s-multi:-])

(Reporter)

Description

6 years ago
Multiple content processes are a primary goal for desktop Firefox, and we need to at least audit that DOM storage API is correctly atomic or serialized, both from the DOM API (document.cookie) as well as at the necko level. I believe that Chromium does not make any guarantees about synchronization, and synchronization was removed from the HTML spec, but roc disagreed and thought it was necessary and possible to lock access across processes.
(Reporter)

Updated

6 years ago
Blocks: 641683
This is localStorage, right?  How much do we care about this going forward?
Synchronization was not removed from the Web storage spec:
http://www.whatwg.org/specs/web-apps/current-work/complete/webstorage.html#threads
There are 4 things we have to take care of:

- propagate selective (time based) and complete cleanup invoked by user from the chrome process (clear recent history etc.)
  * today it is simple, we set all storage instances dirty to force reload from DB and localStorage and globalStorage always load from chrome process

- state of data, = sync among content processes
  * that will be OK for localStorage and globalStorage (both re-read data from chrome process that keeps it centralized)
  * NOT OK for sessionStorage, current design is based on fact we share a single instance on the content process for all interested window instances ; we will have to sync data changes for sessionStorage - achievable by using the same pattern as we use for localStorage

- sending DOM storage events to other windows:
  * currently we manage events on the single content process the same way as in a single process firefox (event never goes up to the chrome process)
  * for multiple content processes this has to go through the chrome process obviously

- filtering: prevent sending DOM storage event to the originating storage instance/window:
  * doesn't apply to globalStorage
  * currently this is prevented by checking on pointer of the localStorage object that the event carries (on equality prevent propagation to the handler on the window)
  * for sessionStorage we use a relatively complex forking mechanism (there is a single central sessionStorage object, but each window keeps a wrapper to communicate with it), we again compare the pointer ; the central sessionStorage instance is in the multiprocess environment held on the content process


Ideal solution is to completely rework the storage system and base it just on sending messages about changing the state in both directions (actually just "set" and "clearAll"), let chrome process do the filtering and relay

No idea how to implement synchronization at the moment, though.  Probably just managed on the chrome process.  Maybe result of a natural event propagation mechanism..  AFAIK we didn't find consensus on how the mutex should be implemented.  We could lock changes/reads since we get an event from a content process and hold it until we receive another event from the same content process that the lock has been released, but when the hell to release it?

Before we start with it, we should propose a change to the spec to post storage events instead of processing them synchronously.
See Also: → bug 584946
(In reply to comment #3)
> - state of data, = sync among content processes
>   * that will be OK for localStorage and globalStorage (both re-read data
> from chrome process that keeps it centralized)
>   * NOT OK for sessionStorage, current design is based on fact we share a
> single instance on the content process for all interested window instances ;
> we will have to sync data changes for sessionStorage - achievable by using
> the same pattern as we use for localStorage

Can we ensure that all windows sharing the same sessionStorage are in the same content process?

> No idea how to implement synchronization at the moment, though.  Probably
> just managed on the chrome process.  Maybe result of a natural event
> propagation mechanism..  AFAIK we didn't find consensus on how the mutex
> should be implemented.  We could lock changes/reads since we get an event
> from a content process and hold it until we receive another event from the
> same content process that the lock has been released, but when the hell to
> release it?

See bug 666722.
(In reply to comment #4)
> (In reply to comment #3)
[...]
> Can we ensure that all windows sharing the same sessionStorage are in the
> same content process?

Kinda jumping in here w/o a ton of context here, but doing that seems hard given that a window containing a webpage running in a given child process can at any time load anything else in an iframe etc. And that means that the containing page has a reference to the page loaded in the iframe, which means that we can't load something in an iframe in a different process than its parent is in, which means we can't force everything in a given origin to be in the same process.
(In reply to comment #5)
> Kinda jumping in here w/o a ton of context here, but doing that seems hard
> given that a window containing a webpage running in a given child process
> can at any time load anything else in an iframe etc. And that means that the
> containing page has a reference to the page loaded in the iframe, which
> means that we can't load something in an iframe in a different process than
> its parent is in, which means we can't force everything in a given origin to
> be in the same process.

Right, but same-origin pages don't automatically share the same sessionStorage, do they?
(In reply to comment #6)
> Right, but same-origin pages don't automatically share the same
> sessionStorage, do they?

They do if they are parent/iframe.  They share the same object (from point of view of the content).  window.open() JS API clones the storage, read as: creates a new object (from the point of view of the content, but also programmatic point of view) and copies the data from the original.
(In reply to comment #7)
> (In reply to comment #6)
> > Right, but same-origin pages don't automatically share the same
> > sessionStorage, do they?
> 
> They do if they are parent/iframe.

Right, but that's not the case here.
Since there is an overhaul of how domstorage is going to be implemented soon, I will not work on this right now.
Assignee: honzab.moz → nobody

Updated

2 years ago
Blocks: 1207306
No longer blocks: 641683
Duplicate of this bug: 1191791
Honza, what's the status of the overhaul you mentioned in comment 9? Can we start looking at fixing this now?
Flags: needinfo?(honzab.moz)
Whiteboard: [e10s-multi:M1]
(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #11)
> Honza, what's the status of the overhaul you mentioned in comment 9? Can we
> start looking at fixing this now?

I was referring to work of Jan Varga and Quota Manager.  That question should be pointed at him.


OTOH, if there are never going to be more than one content process for the same origin, this may be just WORKSFORME.  I've heard there will not be a process per tab, but just per origin.  It would make this a no-problem.

If a scenario where localStorage.foo = "bar" followed with window.postMessage() makes the event handler see the foo = "bar" change in its localStorage (only applies to windows with the same origin, of course) then we can just close this bug.
Flags: needinfo?(honzab.moz) → needinfo?(mrbkap)
I would be surprised if we can ever guarantee that certain domain uses only one process. That is hard to guarantee because of iframes and such.
(Assignee)

Comment 14

a year ago
I just tested this. I have Tab A and Tab B, both in separate content process. If I store something in Tab A, Tab B doesn't see the stored value.

I think this should be fixable independently from the quota manager / local storage work.
I'll take a look.
Flags: needinfo?(mrbkap)
(Assignee)

Updated

a year ago
Depends on: 1286798
Blocks: 1285898
Whiteboard: [e10s-multi:M1] → [e10s-multi:?]
Sharing storage is only one problem. Another is operate on the same data with multiple processes/tabs at the same time. Look this scenerio:
- processA read data
- processB write data
- processA write data
ProcessA will operate on old data because processB change them. Will be any solution to solve this in future? I have similar problem on Greasemonkey:
https://github.com/greasemonkey/greasemonkey/issues/2427
Whiteboard: [e10s-multi:?] → [e10s-multi:M3]
FWIW, I believe we've started encountering this problem in Firefox Accounts, where iframed content in about:accounts has a different view of localStorage to web content in a top-level tab.  See e.g:

  https://github.com/mozilla/fxa-content-server/issues/4060
  https://github.com/mozilla/fxa-content-server/issues/4115
Jan's got this up next-ish on his plate, likely commencing work in November.
Assignee: nobody → jvarga
As ugly as it is, the work done in bug 1285898 is good enough for an e10s-multi MVP, right?
Flags: needinfo?(bugmail)
Flags: needinfo?(amarchesini)
(In reply to Andrew Overholt [:overholt] from comment #18)
> As ugly as it is, the work done in bug 1285898 is good enough for an
> e10s-multi MVP, right?

Yes.  Writes in one content process are perceived in other content processes in as timely a fashion as possible.

The caveat is that races can happen, and although they are detectable by content, the winner of the race will think they lost.

Assume a key "racer" with initial value "initial".  Simultaneously, process A writes "a" and process B writes "b".  They will send events { oldValue: "initial", newValue: "a" } and { oldValue: "initial", newValue: "b" } to the parent process simultaneously.

From the database thread's perspective, there will be a clear winner.  However, the child processes will not know who the winner is because their own events are not re-broadcast to them.  But they will each receive the other processes' message.  If they care, they can notice that the oldValue is not the value they wrote and so a race may have occurred.

At this time, the ambiguity can only be resolved by content either:
- Attempting to re-write the value another time, which may result in another race.  (Repeated iterations of this would be reminiscent of a metastable state.)
- Causing the localstorage backing cache to be forgotten and reloaded in the given content process.  This is hard for content to unreliably do unless it is the only page open for the origin in the process, forces itself to not be bfcached, and closes itself entirely before somehow causing the page to be reopened.

Although this situation could be improved, particularly by :janv's proposed refactor of "keep the data in the parent and use sync IPC to get the data on demand", it's probably best to advise everyone with complicated concurrency needs to just use something with a real transaction model, which currently means IndexedDB.


Note: For the comment 16 case where chrome code may be involved, there's one other possible hiccup.  The current implementation assumes pages for an origin would not exist in both the parent process and content processes at the same time.  This may not be true for chrome-privileged things like Firefox Accounts which could run afoul of the event-propagation asymmetry.  See my review comments in bug 1285898 for more on that.
Flags: needinfo?(bugmail)
Thanks. I'm re-setting the e10s-multi whiteboard field to ? given comment 19 (and bug 1285898 having stuck).
Whiteboard: [e10s-multi:M3] → [e10s-multi:?]

Updated

7 months ago
Whiteboard: [e10s-multi:?] → [e10s-multi:-]
Flags: needinfo?(amarchesini)
See Also: → bug 1362190

Updated

2 months ago
tracking-fennec: --- → ?
blocking-fx: --- → ?
status-firefox55: --- → ?
status-firefox56: --- → ?
status-firefox57: --- → ?
status-firefox-esr52: --- → ?
tracking-thunderbird_esr52: --- → ?
Don't abuse please.
tracking-fennec: ? → ---
blocking-fx: ? → ---
status-firefox55: ? → ---
status-firefox56: ? → ---
status-firefox57: ? → ---
status-firefox-esr52: ? → ---
tracking-thunderbird_esr52: ? → ---
You need to log in before you can comment on or make changes to this bug.