1384618 - Implement BackgroundChild::GetOrCreateForLocalStorage

Status: RESOLVED WONTFIX

(Core :: DOM: Content Processes, enhancement)

Description

[Jan Varga [:janv]]

Originally, I wanted to fix bug 1283609, but due to time constraints, I just did local storage specific method. It's like SynchronouslyCreateForCurrentThread(), but instead of just calling GetOrCreateForCurrentThread and spinning the event loop, everything is done using thread blocking calls.

Patch coming in a sec.

Comment 1

[Jan Varga [:janv]]

Attachment: patch

Comment 2

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

Comment on attachment 8890429 [details] [diff] [review]
patch

I suspect you meant to ask :billm for review on this given the discussion on bug 1283609 and that this code seems derived from existing Background* code.  (And also because neither of us are IPC peers :)

Also, I'm not quite clear on what was broken as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1283609#c8 that merits this specialized variant.  Did you just not want to deal with the existing async consumers and their callbacks?  It seems like that's the difference between the existing dispatched-to-background-thread-then-bounced-back-to-the-main-thread RequestMessageLoopRunnable versus your new RequestMessageLoopHelper.

Also, CreateBackgroundThread is still going to create a RequestMessageLoopRunnable and dispatch it even when this new specialized-path is taken, which freaks me out.  From my perspective, it seems like it would be preferable to make us always synchronously create PBackground and use your RequestMessageLoopHelper monitor approach.  The jank delay is the time to spin up a new nsThread and dispatch a single runnable at it.  All async consumers would end up just being: `CreateSync(); NS_DispatchToMainThread(aCallback);`.  That's some jank, but appreciably less than the current RequestMessageLoopRunnable bounce.

Comment 3

[Jan Varga [:janv]]

I just didn't want to deal with non main thread scenario, for now.

Also, if GetOrCreateForLocalStorage is called after GetOrCreateForCurrentThread, the async process already started and it would be hard to reuse the thread local stuff to look for existing background child actor.

If you guys think that there can only be one RequestMessageLoopHelper that gets the message loop by blocking the main thread in main process then I can do it that way.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8890429 [details] [diff] [review]
patch

Review of attachment 8890429 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not in favor of having this special thing just for local storage. I understand the desire to finish things quickly, but it doesn't seem like that much more work to do this the right way.

I think we could simplify this code somewhat as follows. Right now, calling Open requires a MessageLoop, and we don't have one for a thread that was just created. I would recommend adding an Open overload that takes an nsIEventTarget instead of a MessageLoop. The only use of the MessageLoop is here, as far as I can tell:
http://searchfox.org/mozilla-central/rev/8a61c71153a79cda2e1ae7d477564347c607cc5f/ipc/glue/MessageChannel.cpp#805
We could change that to eventTarget->Dispatch instead of PostTask. For callers passing in a MessageLoop, we could call the event target version, passing messageLoop->SerialEventTarget():
http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/ipc/chromium/src/base/message_loop.h#158

As much as possible, it would be good to change the asynchronous code to use the synchronous paths. If we make the change in the previous paragraph, I can't think of any performance advantages to using the asynchronous paths anymore.

Comment 5

[Jan Varga [:janv]]

Ok, I'll take a look.

However, if I understand you correctly, you want to replace:
static bool
GetOrCreateForCurrentThread(nsIIPCBackgroundChildCreateCallback* aCallback);

static PBackgroundChild*
SynchronouslyCreateForCurrentThread();

with:
static PBackgroundChild*
GetOrCreateForCurrentThread();

Having both, asynchronous and real synchronous (blocking) GetOrCreate() would be hard, since we can't reuse threadLocal->mActor for both.

This all sounds great, except it's quite a big change :)

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Yes, although we could keep the old asynchronous API around so we don't have to change all the callers. But the asynchronous API would just invoke the synchronous API.

Comment 7

[Jan Varga [:janv]]

Ok, one more question ...

In bug 1283609 comment 6 you gave me instructions how to fix BackgroundChild::SynchronouslyCreateForCurrentThread():
1. Create the endpoints, as we do here:
http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/ipc/glue/BackgroundImpl.cpp#1995-2004

2. Create a ChildImpl, like we do here:
http://searchfox.org/mozilla-central/source/ipc/glue/BackgroundImpl.cpp#1534

3. Bind the actor to the child endpoint, like this:
http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/ipc/glue/BackgroundImpl.cpp#1814

4. On the main thread (which might require dispatching to the main thread if we're not there already), send the parent endpoint to the parent process, like this:
http://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/ipc/glue/BackgroundImpl.cpp#2006

5. Return the actor created in step 2.

Step 4 will still be asynchronous, but that's okay. It should be fine to start using the actor before step 4 has completed.


One thing is not clear to me, some other stuff must run on the main thread too, for example for calling CreateEndpoints() I need to get a process id and that is currently done by calling ContentChild::OtherPid().
Also, for the same process case, I need to call CreateBackgroundThread() which is also main thread only.
This can't be done asynchronously.

The only way how to do that is to use a monitor and block the current thread until a runnable dispatched to the main thread is executed. If we are creating a background child for the main thread, we don't need to block anything of course.
Anyway, is blocking ok for other threads, for example DOM workers ?

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Jan Varga [:janv] from comment #7)
> The only way how to do that is to use a monitor and block the current thread
> until a runnable dispatched to the main thread is executed. If we are
> creating a background child for the main thread, we don't need to block
> anything of course.
> Anyway, is blocking ok for other threads, for example DOM workers ?

Ugh, I don't think that's okay.

I think we should be able to call ContentChild::GetSingleton() and ContentChild::OtherPid() on a non-main thread. That data never changes after it's set, and it should be set by the time any of this happens.

CreateBackgroundThread is harder. We could make it thread-safe, but that would take some effort. What if, instead, we pass a special event target to Open that takes any runnable dispatched to it and passes it to the background thread once the background thread is ready? I think this should work, and it shouldn't be too hard to implement.

Comment 9

[Jan Varga [:janv]]

Ok, I'll try to do it the way you suggested.

Comment 10

[Jan Varga [:janv]]

(In reply to Bill McCloskey (:billm) from comment #8)
> I think we should be able to call ContentChild::GetSingleton() and
> ContentChild::OtherPid() on a non-main thread. That data never changes after
> it's set, and it should be set by the time any of this happens.

This seems to work, although I need to do more testing.

> CreateBackgroundThread is harder. We could make it thread-safe, but that
> would take some effort. What if, instead, we pass a special event target to
> Open that takes any runnable dispatched to it and passes it to the
> background thread once the background thread is ready? I think this should
> work, and it shouldn't be too hard to implement.

Now, I'm going to try this ...

Comment 11

[Jan Varga [:janv]]

(In reply to Bill McCloskey (:billm) from comment #8)
> CreateBackgroundThread is harder. We could make it thread-safe, but that
> would take some effort. What if, instead, we pass a special event target to
> Open that takes any runnable dispatched to it and passes it to the
> background thread once the background thread is ready? I think this should
> work, and it shouldn't be too hard to implement.

This seems to work too :)
For now, I'm able to run local storage tests at least w/o any crashes.

I'm going to clean it up and push to try.

Bill, thanks for all the info/suggestions.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Thanks for doing this Jan! It will be great to have this done.

Comment 13

[Jan Varga [:janv]]

Bug 1283609 landed instead.