1609110 - Follow up fixes on async process creation introduced in Bug 1602712 and Bug 1605086

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

This issue goal is to double-check and discuss with :Yoric some changes I had to introduce locally to fix assertion failures I've been triggering in a stack of patches related to the Extension's ServiceWorkers.

---

Some additional context:

The initial assertion failure I triggered with one of my test case is actually a MOZ_CRASH one:

(MOZ_CRASH(Unknown PrincipalInfo type!) at ipc/glue/BackgroundUtils.cpp:163)

the test case did start triggering it after my last rebase on a recent mozilla-central tip and after having investigated the failure a bit using rr, I've been able to track it back to the changes recently introduced by Bug 1605086.

This test case from my patch stack is covering a scenario where a service worker has to be spawned in a process that doesn't exist yet, and so the test does pass through RemoteWorkerManager::LaunchNewContentProcessAsync, which doesn't seem to be currently covered (based on its coverage reports here).

While trying to recreate the same conditions with a different test case ([1]) that doesn't depend from any of my other changes I've been triggering some different issues, by passing through ContentParent::GetNewOrUsedBrowserProcessAsync (which also seems to not be currently covered based on its coverage reports here).

In particular one of the issue is a EXCEPTION_ACCESS_VIOLATION_READ crash, which seems to be triggered from here](https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/dom/ipc/ContentParent.cpp#981)), and produced the following stack trace

Crash reason:  EXCEPTION_ACCESS_VIOLATION_READ
[task 2020-01-10T18:54:15.223Z] 18:54:15     INFO -  Crash address: 0x7cac8886
[task 2020-01-10T18:54:15.223Z] 18:54:15     INFO -  Process uptime: 1 seconds
[task 2020-01-10T18:54:15.224Z] 18:54:15     INFO -  Thread 0 (crashed)
[task 2020-01-10T18:54:15.224Z] 18:54:15     INFO -   0  xul.dll!static mozilla::PreallocatedProcessManager::AddBlocker(mozilla::dom::ContentParent*) [PreallocatedProcessManager.cpp:9c74eecbb1e952ae81bc3a05ab10f79c5ec9f4d5 : 319 + 0x5]
[task 2020-01-10T18:54:15.224Z] 18:54:15     INFO -      eip = 0x55d4321d   esp = 0x001ddea8   ebp = 0x001ddebc   ebx = 0x0070c300
[task 2020-01-10T18:54:15.225Z] 18:54:15     INFO -      esi = 0x7cac83ce   edi = 0x00000000   eax = 0x0eba3610   ecx = 0x0073f780
[task 2020-01-10T18:54:15.225Z] 18:54:15     INFO -      edx = 0x7cac8322   efl = 0x00010202
[task 2020-01-10T18:54:15.226Z] 18:54:15     INFO -      Found by: given as instruction pointer in context
[task 2020-01-10T18:54:15.226Z] 18:54:15     INFO -   1  xul.dll!mozilla::MozPromise<RefPtr<mozilla::dom::ContentParent>,mozilla::ipc::LaunchError,0>::ThenValue<`lambda at z:/build/build/src/dom/ipc/ContentParent.cpp:976:7',`lambda at z:/build/build/src/dom/ipc/ContentParent.cpp:994:7'>::DoResolveOrRejectInternal(mozilla::MozPromise<RefPtr<mozilla::dom::ContentParent>,mozilla::ipc::LaunchError,0>::ResolveOrRejectValue&) [MozPromise.h:9c74eecbb1e952ae81bc3a05ab10f79c5ec9f4d5 : 727 + 0x14]
[task 2020-01-10T18:54:15.227Z] 18:54:15     INFO -      eip = 0x55d3bd4c   esp = 0x001ddec4   ebp = 0x001ddef4   esi = 0x0073f780
[task 2020-01-10T18:54:15.227Z] 18:54:15     INFO -      Found by: call frame info
[task 2020-01-10T18:54:15.227Z] 18:54:15     INFO -   2  xul.dll!mozilla::MozPromise<OSInfo,nsresult,0>::ThenValueBase::ResolveOrRejectRunnable::Run() [MozPromise.h:9c74eecbb1e952ae81bc3a05ab10f79c5ec9f4d5 : 403 + 0x6]
[task 2020-01-10T18:54:15.228Z] 18:54:15     INFO -      eip = 0x53f4ca71   esp = 0x001ddefc   ebp = 0x001ddf04   ebx = 0x0070c300
[task 2020-01-10T18:54:15.228Z] 18:54:15     INFO -      esi = 0x1023c9a0   edi = 0x00000000
[task 2020-01-10T18:54:15.228Z] 18:54:15     INFO -      Found by: call frame info
[task 2020-01-10T18:54:15.229Z] 18:54:15     INFO -   3  xul.dll!nsThread::ProcessNextEvent(bool, bool*) [nsThread.cpp:9c74eecbb1e952ae81bc3a05ab10f79c5ec9f4d5 : 1248 + 0xa]
[task 2020-01-10T18:54:15.229Z] 18:54:15     INFO -      eip = 0x53ffa373   esp = 0x001ddf0c   ebp = 0x001de42c   esi = 0x00000000
[task 2020-01-10T18:54:15.229Z] 18:54:15     INFO -      Found by: call frame info

---

[1]: the test case is "a bit of a stretch", it is cheating quite a bit to recreate the conditions needed to trigger the issues:

https://hg.mozilla.org/try/rev/b30c6a45649c4e412ea5d73ff4a85deef81c1e81

(the service worker fails to run successfully in this test case, but the test is still able to pass through part of the async process creation which doesn't seem we are covering in the test yet).

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1609110 - Follow up fixes on async process creation.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Do I understand correctly that your patch gets the code to pass the additional test?

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to David Teller [:Yoric] (please use "needinfo") from comment #2)

Do I understand correctly that your patch gets the code to pass the additional test?

yes, I'm double-checking that both the test case linked in comment 0 (https://hg.mozilla.org/try/rev/b30c6a45649c4e412ea5d73ff4a85deef81c1e81) and the test case related to the extension's service worker for my other patch stack are both passing successfully with both Bug 1607530 and the patch attached to this bug applied.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/66fe6c785157
Follow up fixes on async process creation. r=Yoric,asuth

Comment 5

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/66fe6c785157

Comment 7

[Jens Stutte [:jstutte]]

We might want to consider this for beta 73 uplift ?

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Jens Stutte [:jstutte] from comment #7)

We might want to consider this for beta 73 uplift ?

If I'm not wrong this fix shouldn't be needed on Firefox 73 because (on the contrary it looks that Bug 1607530 should be considered for a beta 73 uplift).

---

Follows some additional context:

• Bug 1605086 has been landed on Firefox 74, and so 73 beta shouldn't be affected.

• Bug 1602712 did land in Firefox 73, and it has been fixed in Bug 1607530 and so it looks like something we should consider for a beta 73 uplift
  (this patch has been rebased on top of those changes before we landed it, after that rebase the remaining changes in the attached patch
  applied to that part is now just a small non-mandatory tweak, the fix for the crash is now completely coming from the patch landed from
  Bug 1607530).

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Sounds like this issue is 74-only, so no need for Beta uplift.