1350071 - [e10s-multi] Filter localStorage multi-e10s propagation events so that only interested content processes receive broadcasts.

Status: NEW

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Andrew Sutherland [:asuth] (he/him)]

As landed in bug 1285898, any change to localStorage in any content process will result in an async PContent IPC message of that change to all other content processes.  This has performance and (some day when everything else is locked down) security ramifications.

The shorter term plan in bug 1285898 comment 6 was to introduce filtering in the parent.

The longer term plan is bug 1286798's rewrite of DOMStorage/LocalStorage, which would moot the need to perform filtering and this bug.  (Note that related bug 1322316 covers splitting SessionStorage into its own thing.)  The original vision for the re-write called for storing all state in the parent and using sync IPC on each request from the child, ensuring correctness and avoiding the need to broadcast changes entirely.  I think that concept may be up for reconsideration as we currently endeavor to reduce sync IPC's as used by the similarly structured document.cookie API.  (Note that a re-write or extensive overhaul is still a good idea; the current document-associated event propagation was a pragmatic solution but limits keepalive-style caching.)

Comment 1

[Jan Varga [:janv]]

I originally planned to move the cache to the parent using shared memory, so there would be no sync calls. However using shared memory for stuff that dynamically changes is not easy so I opted for sync calls.
If we can't use sync calls then shared memory might be interesting again or something else :)

Comment 2

[Andrew Overholt [:overholt]]

Andrew, did you have any ideas on how to do the filtering here (it's fine to say "no" :)?

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

It might make sense to just address this as part of the localStorage refactor/cleanup in bug 1286798, especially since :baku's SessionStorage/LocalStorage split (bug 1322316) just landed and we have bug 1350637 on the sync load IPC as an issue that's complicated by the current implementation.

Having said that, there is something that can be done without the refactor.  The important thing for correctness is that, on the parent process main thread, we begin buffering change events for the given origin when its preload request is initiated.  Then, when the preload arrives on the main thread from the database thread, we:
- send the response
- send all the buffered messages
- transition to sending all future messages as they arrive

And then we need the nsGlobalWindow to tell us when it's going away.  Since we've strictly constrained the lifecycle of LocalStorageCache instances to that of their owning LocalStorage and thereby nsGlobalWindow by eliminating the keepalive mechanism, this can be accomplished by any participant.


I'd probably suggest doing something like this, adding the following methods to ContentParent and related callers:
- SubscribeToAndBufferLocalStorageChanges(origin) to be invoked by StorageDBParent::{RecvAsyncPreload,RecvPreload}
- FlushLocalStorageChanges(origin) to be invoked by the CacheParentBridge when its LoadDone() fires.
- MaybeDispatchLocalStorageChange(origin, ...) to replace current uses of SendDispatchLocalStorageChange()
- UnsubscribeFromLocalStorageChanges(origin) to be invoked by a new PStorage message CacheUnloaded(origin) to be sent by the LocalStorageCache destructor when it goes away.

Each ContentParent would gain a map from origin to either a buffer list to append to or a boolean that it should send things through.  Alternately, a helper could be added in the dom/storage space, but for life-cycle reasons I think it would want to live on the ContentParent instance for simplicity.


Again, a refactoring where changes to the localStorage store for an origin are sent via PStorage or its child actors would eliminate most of the problem that is due to:
- nsGlobalWindow being the mutation propagation and change application method.
- That being independent from the database thread and bridge, with the IPC bridge and protocol being effectively stateless.
Switching to actor-per-LocalStorageCache and having it be the means of mutation propagation would wildly simplify things.