1283609 - Stop using nested event loop in BackgroundChild::SynchronouslyCreateForCurrentThread

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P3)

Description

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

Now that we have Endpoints from bug 1223240, it's possible to set up top-level protocols synchronously. We should use that to create PBackgrounds.

Comment 1

[Gabor Krizsanits (INACTIVE)]

Since it's unlikely to fix the session store refactoring for M1 let's re-triage this one.

Comment 2

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

What does this bug have to do with session store?

Comment 3

[Jan Varga [:janv]]

The new implementation uses PBackground and it sometimes needs to get it synchronously to avoid using a nested event loop.

Comment 4

[Jan Varga [:janv]]

Well, it's more about local storage.

Comment 5

[Jan Varga [:janv]]

Bill, I need this for bug 1350637. I thought that I could somehow use current BackgroundChild::SynchronouslyCreateForCurrentThread(), but I'm getting assertions about "not safe to run script" when LocalStorage is being initialized.

Do you know someone who would be able to fix this bug ?

I can also try to fix it myself if you give me some pointers.

Comment 6

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

This is basically what needs to happen:

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.

I *think* that this should all work. I don't know who would be the best person to work on it. I don't think it should be a big job, Jan, so feel free to take this if you need it.

Comment 7

[Jan Varga [:janv]]

Thanks a lot for this information, I'll try to do it myself.

Comment 8

[Jan Varga [:janv]]

This mostly works, but there's more work to do. For example moz extensions seem to run in the main process and if a moz extension accesses LocalStorage I'm not able to use content child to create pbackground child.

Comment 9

[Jan Varga [:janv]]

Attachment: Part 1: Add an IToplevelProtocol::Open overload that takes an nsIEventTarget

Comment 10

[Jan Varga [:janv]]

Attachment: Part 2: Rename BackgroundChild::SynchronouslyCreateForCurrentThread to BackgroundChild::GetOrCreateForCurrentThread

Comment 11

[Jan Varga [:janv]]

first try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=286fe5783d901f378d4c117bbb9e50216dc1e74f

doesn't look so bad

Comment 12

[Jan Varga [:janv]]

I fixed all the oranges, but I'm still trying to finish a new method BackgroundParent::GetLiveActors which is needed for local storage event broadcasting.

Comment 13

[Jan Varga [:janv]]

Attachment: Part 3: Core changes, stop using nested event loop in BackgroundChild::GetOrCreateForCurrentThread()

Bill, I had the special event target in this patch, but I decided to remove it. If you look at childActor->Open() it forwards to MessageChannel::Open().
That method uses a monitor to block the thread until the dispatched runnable is done. So the special event target wouldn't help much.

Comment 14

[Jan Varga [:janv]]

try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78afb6f1a8e876cf1e4784dc0dc07d7b4d49bada

Comment 15

[Jan Varga [:janv]]

Attachment: Part 4: Remove unused code

Comment 16

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

Comment on attachment 8892256 [details] [diff] [review]
Part 3: Core changes, stop using nested event loop in BackgroundChild::GetOrCreateForCurrentThread()

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

::: ipc/glue/BackgroundImpl.cpp
@@ +639,5 @@
>  
> +class ParentImpl::CreateActorHelper final : public Runnable
> +{
> +  mozilla::Mutex mMutex;
> +  mozilla::CondVar mCondVar;

Can you just use a Monitor?

@@ +1950,5 @@
> +    }
> +  } else {
> +    nsCOMPtr<nsIRunnable> runnable =
> +      new SendInitBackgroundRunnable(Move(parent));
> +    MOZ_ALWAYS_SUCCEEDS(NS_DispatchToMainThread(runnable));

I think it would be clearer to use a C++ lambda here with NewRunnableFunction.

Comment 17

[Jan Varga [:janv]]

Thanks! 

I really appreciate your help here.

I missed the uplift by one day, but it seems we're going to request a beta approval for this to get a broader testing, so all this stuff can ship in FF 57.

Comment 18

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

Sorry I didn't get to this sooner.

Comment 19

[Jan Varga [:janv]]

No, no, it's not your fault. I'm actually surprised that we managed to get this so soon, given how big this change is.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/846d8a1403be
Part 1: Add an IToplevelProtocol::Open overload that takes an nsIEventTarget; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5b7a4c03364
Part 2: Rename BackgroundChild::SynchronouslyCreateForCurrentThread to BackgroundChild::GetOrCreateForCurrentThread; r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb129c8a92bf
Part 3: Core changes, stop using nested event loop in BackgroundChild::GetOrCreateForCurrentThread(); r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/975d8155e4d0
Part 4: Remove unused code; r=billm

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/846d8a1403be
https://hg.mozilla.org/mozilla-central/rev/e5b7a4c03364
https://hg.mozilla.org/mozilla-central/rev/cb129c8a92bf
https://hg.mozilla.org/mozilla-central/rev/975d8155e4d0

Comment 22

[Jan Varga [:janv]]

Comment on attachment 8891508 [details] [diff] [review]
Part 1: Add an IToplevelProtocol::Open overload that takes an nsIEventTarget

Approval Request Comment
[Feature/Bug causing the regression]: It's not a regression.
[User impact if declined]: This bug is a prerequisite for bug 1350637 which improves performance by removing a main process/main thread sync message.
[Is this code covered by automated tests?]: Yes, many APIs (DOM cache, IndexedDB, etc.) depend on this and there is a lot of tests for them.
[Has the fix been verified in Nightly?]: Yes, this landed independently from bug 1350637 14 days ago. There were no regressions so far.
[Needs manual test from QE? If yes, steps to reproduce]: No manual tests needed.
[List of other uplifts needed for the feature/fix]: We will request approval for bug 1350637 too.
[Is the change risky?]: Not very risky.
[Why is the change risky/not risky?]: It's quite a big change, but we were observing mozilla-central for two weeks and there were no regressions. Stuff works as expected. Even, if there's a problem with bug 1350637 on beta channel, this change can stay I think.
[String changes made/needed]: None.

Comment 23

[Gerry Chang [:gchang]]

Hi :janv,
These are quite a lot changes and it's not a regression. Can we let this ride the train and won't fix in 56?

Comment 24

[Jan Varga [:janv]]

If we won't fix this then there's higher risk that we find regressions late in the game. Beta channel has much broader audience and we can catch possible problems sooner. However there's no evidence so far that this and bug 1350637 can cause severe regressions. Anyway, if there's something that we don't know about yet that can show beta in a bad light, I think it would be better to do it in 56 than 57. 57 must be awesome :)

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8891508 [details] [diff] [review]
Part 1: Add an IToplevelProtocol::Open overload that takes an nsIEventTarget

Complex work that we need to test on beta. I'd rather shake out problems now than in 57, so let's uplift for beta 5.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/df048280b513
https://hg.mozilla.org/releases/mozilla-beta/rev/269bc08c27ed
https://hg.mozilla.org/releases/mozilla-beta/rev/a390db05a606
https://hg.mozilla.org/releases/mozilla-beta/rev/620514babf09

Comment 27

[Worcester12345]

(In reply to Jan Varga [:janv] from comment #24)
> If we won't fix this then there's higher risk that we find regressions late
> in the game. Beta channel has much broader audience and we can catch
> possible problems sooner. However there's no evidence so far that this and
> bug 1350637 can cause severe regressions. Anyway, if there's something that
> we don't know about yet that can show beta in a bad light, I think it would
> be better to do it in 56 than 57. 57 must be awesome :)

Disagree. I use (test) 56, and am OK with beta being a beta. Not OK with beta being an alpha or pre-alpha build. That's not what the original understanding of this program is considered to be. 

Thanks.

Comment 28

[Jan Varga [:janv]]

(In reply to Worcester12345 from comment #27)
> Disagree. I use (test) 56, and am OK with beta being a beta. Not OK with
> beta being an alpha or pre-alpha build. That's not what the original
> understanding of this program is considered to be. 
> 
> Thanks.

I don't understand, in bug 1350637 comment 85 you asked about getting this into 56.

Comment 29

[Andrei Vaida [:avaida]]

(In reply to Jan Varga [:janv] from comment #22)
> [Is this code covered by automated tests?]: Yes, many APIs (DOM cache,
> IndexedDB, etc.) depend on this and there is a lot of tests for them.
> [Has the fix been verified in Nightly?]: Yes, this landed independently from
> bug 1350637 14 days ago. There were no regressions so far.
> [Needs manual test from QE? If yes, steps to reproduce]: No manual tests
> needed.

Setting qe-verify-  based on Jan's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 30

[Worcester12345]

(In reply to Jan Varga [:janv] from comment #28)
> (In reply to Worcester12345 from comment #27)
> > Disagree. I use (test) 56, and am OK with beta being a beta. Not OK with
> > beta being an alpha or pre-alpha build. That's not what the original
> > understanding of this program is considered to be. 
> > 
> > Thanks.
> 
> I don't understand, in bug 1350637 comment 85 you asked about getting this
> into 56.

I see now. There is always a trade-off, and I guess I interpreted the risk:benefit differently, depending on how they were presented. I'll back away now. Sorry.