1405270 - LocalStorage: Rework sync preloading using a nested event queue

Status: RESOLVED WONTFIX

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

Description

[Jan Varga [:janv]]

The current sync message approach is problematic if parent needs to do asynchronous stuff (like accessing principals on the main thread, directory locking using Quota Manager, etc.). before sending a reply, because it would lead to a dead lock.

There are multiple options how to solve this:
1. Create a new PBackground like dedicated thread in the parent just for LocalStorage
- this is a lot of work and it brings a lot of overhead and complications

2. Create a LocalStorage IPC thread in content process
- for example GetItem() dispatches a runnable to the IPC thread which uses async IPC messages to communicate with the parent, the original (main) thread is blocked using a monitor while waiting for a reply
- this is again complicated and brings overhead

3. Use current PBackgroundStorage protocol as it is, but create an async subprotocol for "synchronous" preloading and wait for a reply using a nested event loop
- we can't use standard nested event looping of course, because we would fire various unrelated events while waiting for a reply
- however, we can use PushEventQueue/PopEventQueue like dom worker implementation does and set this nested event loop as the event target for the subprotocol/actor
using SetEventTargetForActor(actor, nestedEventTarget)

The last option looks like the best from my point of view, I actually implemented it for the next-generation LocalStorage and it works fine locally.
Today I ported the experimental sync preloading to current LocalStorage implementation to be able to test it on try server and possibly land this as an improvement for current LocalStorage implementation.

I plan to get feedback for this from various Gecko experts, since this technique is not yet used in Gecko.

Comment 1

[Jan Varga [:janv]]

Try doesn't look bad, most of bc failures is caused by disabled preloading which I did to force use of the new code. 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32a3589d31add35b904b02e9b4a9ab2be503fec3

Comment 2

[Jan Varga [:janv]]

Attachment: patch

Comment 3

[Jan Varga [:janv]]

Bill, Ehsan, do you think doing synchronous wait this way is okish ?

The static cast to nsThread is not ok of course, will need to find a better way to expose Push/PopEventQueue, but I'm mostly interested about this:
- get a nested event queue using PushEventQueue
- set the nested event queue for a newly created short lived actor
- spin the nested event loop
- get results from the actor
- PopEventQueue

Comment 4

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

I was thinking something along the lines of option 2 where:
- Perhaps we consolidate dom/file/ipc/IPCBlobInputStreamThread to be a PBackground-in-child analog in the sense that it's used by multiple subsystems and helps linearize non-CPU-bound things a little.
- Use Quantum DOM scheduling and its yield mechanism to block the entire TabGroup until the semantically sync operation completes.

I believe the latter is especially necessary because I don't think spinning the nested event loop is safe as implemented in the patch.  Perhaps the script blocker mechanism is workable for this, but yielding the thread seems less nightmarish.  Especially since the LocalStorage overhaul is going to result in more sync-looking IPC, and the risk from nested event loops goes way up when content can intentionally and reliably trigger them.

Comment 5

[Jan Varga [:janv]]

(In reply to Andrew Sutherland [:asuth] from comment #4)
> I was thinking something along the lines of option 2 where:
> - Perhaps we consolidate dom/file/ipc/IPCBlobInputStreamThread to be a
> PBackground-in-child analog in the sense that it's used by multiple
> subsystems and helps linearize non-CPU-bound things a little.
> - Use Quantum DOM scheduling and its yield mechanism to block the entire
> TabGroup until the semantically sync operation completes.
> 
> I believe the latter is especially necessary because I don't think spinning
> the nested event loop is safe as implemented in the patch.  Perhaps the
> script blocker mechanism is workable for this, but yielding the thread seems
> less nightmarish.  Especially since the LocalStorage overhaul is going to
> result in more sync-looking IPC, and the risk from nested event loops goes
> way up when content can intentionally and reliably trigger them.

Well, I thought that by calling PushEventQueue we eliminate firing of unwanted events while we wait for the parent to send a reply. If something else dispatches to the thread it will be queued and executed after we finish the spinning of nested event loop.

The blocking of entire TabGroup sounds good, but we don't have that yet, right ?

I think that in both cases the parent will behave the same way (use async IPC), it just differs in the way how we block the main thread in content process while waiting for a reply from the parent.
Could we use the nested event loop until we have Quantum DOM scheduling ?

Comment 6

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

(In reply to Jan Varga [:janv] from comment #5)
> Well, I thought that by calling PushEventQueue we eliminate firing of
> unwanted events while we wait for the parent to send a reply. If something
> else dispatches to the thread it will be queued and executed after we finish
> the spinning of nested event loop.

Apologies, I misunderstood what was going on there and have been delayed while trying to better understand how using the currently worker-thread-only API usage interacts with the already-landed Scheduler logic, but haven't found the time.  If we go with the nested event loop, I would suggest adding a comment in the code that clarifies what's going on there.

> The blocking of entire TabGroup sounds good, but we don't have that yet,
> right ?
> 
> I think that in both cases the parent will behave the same way (use async
> IPC), it just differs in the way how we block the main thread in content
> process while waiting for a reply from the parent.
> Could we use the nested event loop until we have Quantum DOM scheduling ?

I agree about the equivalence if there's no Quantum DOM scheduling in place.  I defer to :billm.

Comment 7

[Jan Varga [:janv]]

I think I found a problem in this approach.

Comment 8

[Jan Varga [:janv]]

I took a different route. This is not needed anymore.