Stop using nested event loop in BackgroundChild::SynchronouslyCreateForCurrentThread

RESOLVED FIXED in Firefox 56

Status

()

defect
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: billm, Assigned: janv)

Tracking

unspecified
mozilla57
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [e10s-multi:M3])

Attachments

(4 attachments)

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.
Priority: -- → P3
(Assignee)

Updated

3 years ago
Blocks: 1286798
Whiteboard: [e10s-multi:?]
Whiteboard: [e10s-multi:?] → [e10s-multi:M1]
Since it's unlikely to fix the session store refactoring for M1 let's re-triage this one.
Whiteboard: [e10s-multi:M1] → [e10s-multi:?]
What does this bug have to do with session store?
(Assignee)

Comment 3

3 years ago
The new implementation uses PBackground and it sometimes needs to get it synchronously to avoid using a nested event loop.
(Assignee)

Comment 4

3 years ago
Well, it's more about local storage.
Whiteboard: [e10s-multi:?] → [e10s-multi:M3]
(Assignee)

Comment 5

2 years ago
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.
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 7

2 years ago
Thanks a lot for this information, I'll try to do it myself.
(Assignee)

Comment 8

2 years ago
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.
(Assignee)

Comment 9

2 years ago
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Attachment #8891508 - Flags: review?(wmccloskey)
Attachment #8891508 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 12

2 years ago
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.
Attachment #8891608 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 13

2 years ago
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.
Attachment #8892256 - Flags: review?(wmccloskey)
(Assignee)

Comment 15

2 years ago
Attachment #8892389 - Flags: review?(wmccloskey)
(Assignee)

Updated

2 years ago
Blocks: 1350637
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.
Attachment #8892256 - Flags: review?(wmccloskey) → review+
Attachment #8892389 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 17

2 years ago
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.
Sorry I didn't get to this sooner.
(Assignee)

Comment 19

2 years ago
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

2 years ago
Pushed by jvarga@mozilla.com:
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
(Assignee)

Updated

2 years ago
See Also: → 1384618
(Assignee)

Comment 22

2 years ago
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.
Attachment #8891508 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Attachment #8891608 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Attachment #8892256 - Flags: approval-mozilla-beta?
(Assignee)

Updated

2 years ago
Attachment #8892389 - Flags: approval-mozilla-beta?
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?
Flags: needinfo?(jvarga)
(Assignee)

Comment 24

2 years ago
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 :)
Flags: needinfo?(jvarga)
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.
Attachment #8891508 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8891608 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8892256 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8892389 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 27

2 years ago
(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.
(Assignee)

Comment 28

2 years ago
(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.
(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.
Flags: qe-verify-

Comment 30

2 years ago
(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.
You need to log in before you can comment on or make changes to this bug.