1408333 - Get rid of nsIIPCBackgroundChildCreateCallback

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

We don't need this interface anymore because the creation of PBackground actor is sync (finally).

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - BroadcastChannel

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - MessagePort

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - UDPSocket

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - MutableBlobStorage

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 5 - IPCBlobInputStreamThread

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 6 - asmjs

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 7 - FileSystem

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 8 - U2F

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 9 - Gamepad Tests

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 10 - gamepad

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 11 - webauth

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 12 - cache

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 13 - idb

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 14 - quota

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 15 - serviceWorkers

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 16 - workers

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 17 - http

Comment 18

[Andrea Marchesini [:baku]]

Attachment: part 18 - layout

Comment 19

[Andrea Marchesini [:baku]]

Attachment: part 19 - ContentChild

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 20 - main patch

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 11 - webauth

Comment 22

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

Just want to note the try builds I found :baku ran here:
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ffc30d4110fca789c987ba7b5e99a84fc0f42a0
- https://treeherder.mozilla.org/#/jobs?repo=try&revision=444a36127c3412eeb931a1705d7cee9a0ca5346d

And that the only non-intermittents were in webauthn which attachment 8918257 [details] [diff] [review] has corrected by moving the `StartSign()` call-hunk beneath the initialization of mInfo now that the promise lambda no longer deferred the call by a turn of the event loop.

Comment 23

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

Comment on attachment 8918215 [details] [diff] [review]
part 5 - IPCBlobInputStreamThread

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

::: dom/file/ipc/IPCBlobInputStreamThread.cpp
@@ +56,4 @@
>      MOZ_ASSERT(mActor->State() == IPCBlobInputStreamChild::eInactiveMigrating);
>  
> +    PBackgroundChild* actorChild =
> +      BackgroundChild::GetOrCreateForCurrentThread();

Please add explicit handling for the case where nullptr is returned.  Given the pre-patch code, it seems like checking and returning NS_OK is most consistent.

Comment 24

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

Comment on attachment 8918232 [details] [diff] [review]
part 8 - U2F

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

::: dom/u2f/U2FManager.cpp
@@ +137,5 @@
> +  PWebAuthnTransactionChild* constructedMgr =
> +    actorChild->SendPWebAuthnTransactionConstructor(mgr);
> +
> +  if (NS_WARN_IF(!constructedMgr)) {
> +    return;

Restating: Previously, this would have triggered a MOZ_CRASH() via the ActorFailed() path if (!constructedMgr).  If control flow returned here, this would be a minor change of behavior, but one that I think the logic handles because ActorDestroyed() literally only does `mChild = nullptr`.  However, unless I'm missing something, the Send*() method will invoke mozilla::ipc::FatalError() in the event of failure, which will invoke NS_RUNTIMEABORT which will culminate in at least a call to Abort()/mozalloc_abort().  And that's something we shouldn't actually return from.

Comment 25

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

Comment on attachment 8918257 [details] [diff] [review]
part 11 - webauth

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

::: dom/webauthn/WebAuthnManager.cpp
@@ +255,5 @@
> +  PWebAuthnTransactionChild* constructedMgr =
> +    actor->SendPWebAuthnTransactionConstructor(mgr);
> +
> +  if (NS_WARN_IF(!constructedMgr)) {
> +    MOZ_CRASH("Failed to create a PBackgroundChild actor!");

re: comment 24, Oh, hm, but you did propagate the crash here... well, I think either way is fine given the underlying abort.  However, it might make sense for part 8 to just have the MOZ_CRASH there too for code reading simplicity.  You call.

Comment 26

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

Comment on attachment 8918233 [details] [diff] [review]
part 9 - Gamepad Tests

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

Restating: The removal of the mChild guards are safe because there's a strong invariant that mChild is either valid or mShuttingDown=true (or we will have already crashed ourselves), and mShuttingDown guards all uses of mChild.  (I wanted to check this in case the !mChild check accidentally protected against crashes through the existence of the mPendingOperations support.)

Comment 27

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

Comment on attachment 8918236 [details] [diff] [review]
part 12 - cache

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

::: dom/cache/CacheStorage.cpp
@@ +505,5 @@
>    MOZ_DIAGNOSTIC_ASSERT(mActor);
>    MOZ_DIAGNOSTIC_ASSERT(mActor == aActor);
>    mActor->ClearListener();
>    mActor = nullptr;
> +  mStatus = NS_ERROR_UNEXPECTED;

When inlining the net results of ActorFailed() here, you lost the `MOZ_DIAGNOSTIC_ASSERT(!NS_FAILED(mStatus));` line.  I think :bkelly was pretty intentional in his assertions, so please add that to the assertion checks above.

Comment 28

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

Comment on attachment 8918237 [details] [diff] [review]
part 13 - idb

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

Aside: I rediscovered the bug about making constructor failures not be fatal, it's bug 1332054.

::: dom/indexedDB/IDBFactory.cpp
@@ +718,5 @@
> +      mBackgroundActor =
> +        static_cast<BackgroundFactoryChild*>(
> +          backgroundActor->SendPBackgroundIDBFactoryConstructor(actor,
> +                                                                idbThreadLocal->GetLoggingInfo()));
> +      MOZ_ASSERT(mBackgroundActor);

Downgrading the existing code to an assert seems incorrect.  As I read the existing code, I think it's most consistent to do something like the following:
  mBackgroundActorFailed = true;
  IDB_REPORT_INTERNAL_ERR();
  aRv.Throw(NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
  return nullptr;

The previous code would have returned a new request from OpenInternal, then later ended up in BackgroundActorFailed() and set mBackgroundActorFailed=true and dispatched a NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR to the request from there.  The IDB_WARNING comes from how OpenInternal reports the error and throws when it encounters mBackgroundActorFailed earlier up in OpenInternal (~line 681).

@@ +771,5 @@
>                   IDB_LOG_STRINGIFY(aVersion));
>    }
>  
>    // If we already have a background actor then we can start this request now.
> +  MOZ_ASSERT(mBackgroundActor);

With the change above that bails if !mBackgroundActor, I think you can lose this assert and the now-moot comment above it.

Comment 29

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

Comment on attachment 8918238 [details] [diff] [review]
part 14 - quota

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

::: dom/quota/QuotaManagerService.cpp
@@ +345,4 @@
>      }
>    }
>  
> +  MOZ_ASSERT(mBackgroundActor);

Same deal as with IndexedDB case, I think this should also set mBackgroundActorFailed and return NS_ERROR_FAILURE like the !backgroundActor case above.

Comment 30

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

Comment on attachment 8918239 [details] [diff] [review]
part 15 - serviceWorkers

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

::: dom/workers/ServiceWorkerManager.cpp
@@ -3038,5 @@
>    }
>  
> -  if (!mActor) {
> -    RefPtr<Runnable> runnable =
> -      new SoftUpdateRunnable(aOriginAttributes, aScope, false, nullptr);

(This runnable class needs to be kept, don't remove it.)

@@ -3179,5 @@
>    AssertIsOnMainThread();
>  
> -  if (!mActor) {
> -    RefPtr<Runnable> runnable =
> -      new UpdateRunnable(aPrincipal, aScope, aCallback,

(This runnable class needs to be kept, don't remove it.)

@@ -3779,5 @@
>  
> -  // We need to postpone this operation in case we don't have an actor because
> -  // this is needed by the ForceUnregister.
> -  if (!mActor) {
> -    RefPtr<nsIRunnable> runnable = new RemoveRunnable(aHost);

The runnable class should be removed too.

@@ -3805,5 @@
>  {
>    AssertIsOnMainThread();
> -
> -  if (!mActor) {
> -    RefPtr<nsIRunnable> runnable = new PropagateRemoveRunnable(aHost);

The runnable class should be removed too.

@@ -3834,5 @@
>    AssertIsOnMainThread();
>    MOZ_ASSERT(XRE_IsParentProcess());
> -
> -  if (!mActor) {
> -    RefPtr<nsIRunnable> runnable = new PropagateRemoveAllRunnable();

The runnable class should be removed too.

@@ -4079,5 @@
>    AssertIsOnMainThread();
> -
> -  if (!mActor) {
> -    RefPtr<nsIRunnable> runnable =
> -      new PropagateSoftUpdateRunnable(aOriginAttributes, aScope);

The runnable class should be removed too.

@@ -4097,5 @@
>    MOZ_ASSERT(aPrincipal);
>  
> -  if (!mActor) {
> -    RefPtr<nsIRunnable> runnable =
> -      new PropagateUnregisterRunnable(aPrincipal, aCallback, aScope);

The runnable class should be removed too.

Comment 31

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

Comment on attachment 8918240 [details] [diff] [review]
part 16 - workers

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

::: dom/workers/RuntimeService.cpp
@@ -1952,5 @@
>    AssertIsOnMainThread();
>  
>    nsLayoutStatics::AddRef();
>  
> -  // Make sure PBackground actors are connected as soon as possible for the main

Restating my understanding: Bug 701634 which introduced IDB support from Worker threads was concerned about a Worker created from a remote Blob received via PContent racing the scriptloader that wanted to consume the blob.  (Presumably this involved the PBlob KnownBlobConstructorParams path.)  Given that we can now synchronously create PBackground, the root concern is gone, not to mention that blobs also work differently now.

Comment 32

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

Comment on attachment 8918241 [details] [diff] [review]
part 17 - http

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

::: netwerk/protocol/http/HttpBackgroundChannelChild.cpp
@@ +91,5 @@
>  {
>    LOG(("HttpBackgroundChannelChild::CreateBackgroundChannel [this=%p]\n", this));
>    MOZ_ASSERT(OnSocketThread());
>  
> +  PBackgroundChild* actorChild = BackgroundChild::GetOrCreateForCurrentThread();

I would add a MOZ_ASSERT(mChannelChild) above this for clarity.  It is the currently the case that the only caller is HttpBackgroundChannelChild::Init and it has already NS_ENSURE_ARG'd the passed-in aChannelChild, but assertions are nice.

@@ +105,5 @@
> +  // hold extra reference for IPDL
> +  RefPtr<HttpBackgroundChannelChild> self = this;
> +  Unused << self.forget().take();
> +
> +  mChannelChild->OnBackgroundChildReady(this);

Please file a follow-up in necko to attempt to further propagate the cleanup.  Specifically, it looks like OnBackgroundChildReady primarily exists for mBgInitFailCallback bookkeeping so that FailedAsyncOpen can be called.  Given that this change makes the open process either guranteed to succeed or guaranteed to follow a failure path that does not result in OnBackgroundChildDestroyed, more cleanup by necko peers seems appropriate.

Comment 33

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

Comment on attachment 8918242 [details] [diff] [review]
part 18 - layout

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

::: layout/base/nsRefreshDriver.cpp
@@ +993,4 @@
>      MOZ_CRASH("PVsync actor create failed!");
>    }
> +
> +  layout::PVsyncChild* actor = actorChild->SendPVsyncConstructor();

I know you're just moving code here, but I think this should adopt the NS_WARN_IF/MOZ_CRASH idiom from above if !actor.  This makes the logic consistent even in the face of bug 1332054 potentially making things fallible.

Comment 34

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

Comment on attachment 8918243 [details] [diff] [review]
part 19 - ContentChild

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

::: dom/ipc/ContentChild.cpp
@@ -1168,5 @@
>    BackgroundChild::Startup();
>  
> -  nsCOMPtr<nsIIPCBackgroundChildCreateCallback> callback =
> -    new BackgroundChildPrimer();
> -  if (!BackgroundChild::GetOrCreateForCurrentThread(callback)) {

Since the changes made in bug 1283609 happened, this has been synchronously creating PBackground at this point.  This seems like a major ordering change and it's not clear to me that we want the first main-thread consumer to be the one paying the price.  Switching review over to :billm for this.

Comment 35

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

Overall restating: There are a number of patches here that change behavior slightly in the shutdown case.  Previously GetOrCreateForCurrentThread(aCallback) could return false if it wasn't going to create an actor and invoke the callback.  Some code handled this differently from their ActorFailed() paths, silently not doing things.  But by unifying the code paths, they now crash.  These patches are:
- Part 1: BackgroundChild::GetOrCreateForCurrentThread(bc) invoked without checking return value but ActorFailed() would MOZ_CRASH().
- Part 4: mozilla::ipc::BackgroundChild::GetOrCreateForCurrentThread(this) invoked without checking return value but ActorFailed() would MOZ_CRASH().
- Part 9: Unused << BackgroundChild::GetOrCreateForCurrentThread(this) but ActorFailed() would MOZ_CRASH().
- Part 10: Unused << BackgroundChild::GetOrCreateForCurrentThread(this) but ActorFailed() would MOZ_CRASH().

Comment 36

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

Many of these patches are under dom/ and :baku can delegate review to me, but some of these are outside DOM and I think the argument can be made that these are cross-cutting IPC patches that don't need review from the specific components, like how changes to RefPtr/etc. don't always get specific component review.  But neither :baku or I are IPC peers.  So, :billm, I'm needinfo-ing you to weigh in on whether you're cool with doing some of this under the IPC banner with delegation to me for review, or to perform an authoritative review yourself, or to point r? at the relevant module peers (or delegate that to :baku to do.)

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Is crashing in shutdown conditions really the reasonable?  It seems like this should be a fallible error like we have today and handled by the caller.

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

How would a user of Pbavkground know to avoid calling this API if a crash would result?

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Now that I'm at my laptop I think I understand better.  When PContent->IsShuttingDown() returns true we get a nullptr PBackgroundChild and can gracefully handle it.  Sorry for my confusion.

Comment 40

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

(In reply to Ben Kelly [:bkelly] from comment #39)
> Now that I'm at my laptop I think I understand better.  When
> PContent->IsShuttingDown() returns true we get a nullptr PBackgroundChild
> and can gracefully handle it.  Sorry for my confusion.

Your original concern is correct, however.  What I was saying is that the previous signature of BackgroundChild::GetOrCreateForCurrentThread(aCallback) would go one of three ways:
1. Returns true, subsequently invokes aCallback->ActorCreated.
2. Returns true, subsequently invokes aCallback->ActorFailed.
3. Returns false (because of shutdown), callback will never be invoked.

In comment 35 I'm referring to situations where case 2 would invoke MOZ_CRASH, but case 3 would not.  By collapsing BackgroundChild::GetOrCreateForCurrentThread() to either return a valid actor or nullptr, there are now only 2 cases.  And the comment 35 situations have folded case 3 into case 2 and will now crash at shutdown where they previously didn't crash.  But note that many of these consumers were already consistently crash happy, triggering MOZ_CRASH in both case 3 and case 2.

Unfortunately, I may have misunderstood what happens to content processes in shutdown.  I need to do more research on this, although if someone has documentation about whether we tear down the tabs and their documents on shutdown, that will answer things.  In particular, ContentParent::Observe transmuting "profile-before-change" into an invocation of ShutDownProcess(SEND_SHUTDOWN_MESSAGE) (https://searchfox.org/mozilla-central/rev/091894faeac5b54b7e40b0a304c3d3268f7b645d/dom/ipc/ContentParent.cpp#2743) is a real problem if we haven't torn down all the documents[1].

Although the good news is that as long as we don't make the changes proposed in Part 19 that stop ContentChild::InitXPCOM from ceasing to early-initialize PBackground, then ChildImpl::GetOrCreateForCurrentThread() on the main thread doesn't really care that ContentChild::IsShuttingDown() because it can just return the actor out of the ThreadLocalInfo.  And since WorkerThreadPrimaryRunnable::Run invokes BackgroundChild::GetOrCreateForCurrentThread() at startup, already-existing workers are fine.  Unfortunately, freshly created workers will have a problem, because WorkerThreadPrimaryRunnable will keep going if it fails to get PBackground and so sub-Workers() that access crash-triggering APIs like BroadcastChannel could trigger a crash.  (The ScriptLoader involving the main thread may mitigate this?)  Also, helper threads could be a problem, although in the child process we tend to not have as many of those, and at least the IPCBlobInputStreamThread isn't aggressively crashy.

I expect the conclusion goes one of 2 ways:
- We stop invoking MOZ_CRASH when we fail to create an actor, spinning off per-component bugs that require tests that verify they cleanly handle shutdown.
- We define and document our shutdown behavior and move even more things over to MOZ_CRASH so that violation of the invariant very quickly manifests as test failures and red alert on crash-stats.mozilla.com.


1: The timeline of observer related things is as follows, none of which help ensure a clean shutdown if there is live content/
- In the parent, ContentParent::Observe transmutes "profile-before-change" to invoking ShutDownProcess(SEND_SHUTDOWN_MESSAGE): https://searchfox.org/mozilla-central/rev/091894faeac5b54b7e40b0a304c3d3268f7b645d/dom/ipc/ContentParent.cpp#2743
- ContentParent::ShutDownProcess invokes SendShutdown(): https://searchfox.org/mozilla-central/rev/091894faeac5b54b7e40b0a304c3d3268f7b645d/dom/ipc/ContentParent.cpp#1389
- In the child, ContentChild::RecvShutdown (after waiting for there to be no nested event loops) sets mShuttingDown which will result in ContentChild::IsShuttingDown() returning true: https://searchfox.org/mozilla-central/rev/091894faeac5b54b7e40b0a304c3d3268f7b645d/dom/ipc/ContentChild.cpp#2997
- ContentChild::RecvShutdown will also notify "content-child-shutdown", but only telemetry cares about this.

Comment 41

[Andrea Marchesini [:baku]]

> Although the good news is that as long as we don't make the changes proposed
> in Part 19 that stop ContentChild::InitXPCOM from ceasing to

I can propose a second version of part 19 where we create the actor on the main-thread.

> I expect the conclusion goes one of 2 ways:
> - We stop invoking MOZ_CRASH when we fail to create an actor, spinning off
> per-component bugs that require tests that verify they cleanly handle
> shutdown.

Personally, I don't like all these MOZ_CRASH()es. I would like to change them.
But in follow-up bugs.

Are we OK with changing part 19, landing these patches, fixing all the MOZ_CRASH()es in follow ups?

Comment 42

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

(In reply to Andrea Marchesini [:baku] from comment #41)
> Are we OK with changing part 19, landing these patches, fixing all the
> MOZ_CRASH()es in follow ups?

I'm okay with this.  Rationale:
- Eliminating MOZ_CRASH is a larger semantic change with potential security implications, deserving of their own distinct bugs reviewed by and possibly implemented by the component peers.
- Content process crashes at shutdown are potentially annoying, but not fatal.
- In the parent process, PBackground shutdown occurs at "xpcom-shutdown-threads" which is nice and late and everything else should be dead at that point, so parent process crashes are less likely.

That said, we're 14 business days from 58 going to beta, so I think it's appropriate to either land this ASAP or wait until 59.

Comment 43

[Andrea Marchesini [:baku]]

Attachment: part 19 - ContentChild

Comment 44

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

Comment on attachment 8921243 [details] [diff] [review]
part 19 - ContentChild

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

Just to reiterate, :billm or an IPC peer really needs to sign off on all of this.

Comment 45

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

(In reply to Andrew Sutherland [:asuth] from comment #34)
> Comment on attachment 8918243 [details] [diff] [review]
> part 19 - ContentChild
> 
> Review of attachment 8918243 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/ContentChild.cpp
> @@ -1168,5 @@
> >    BackgroundChild::Startup();
> >  
> > -  nsCOMPtr<nsIIPCBackgroundChildCreateCallback> callback =
> > -    new BackgroundChildPrimer();
> > -  if (!BackgroundChild::GetOrCreateForCurrentThread(callback)) {
> 
> Since the changes made in bug 1283609 happened, this has been synchronously
> creating PBackground at this point.  This seems like a major ordering change
> and it's not clear to me that we want the first main-thread consumer to be
> the one paying the price.  Switching review over to :billm for this.

I'm not sure I understand this comment. Calling GetOrCreateForCurrentThread() in the content process on the main thread is pretty cheap. It just does a couple allocations and sends an IPC message. In addition, the InitXPCOM caller that's being removed is already on the main thread and already synchronous (despite the API being used).

Regarding a failure to create the actor, I noticed that we never actually run the ActorFailed method right now. I should have caught that in bug 1283609 but I didn't. We just return false from GetOrCreateForCurrentThread.

Regarding shutdown: we try our best to close all documents before profile-before-change (or any other content process shutdown stuff). However, it's really a best-effort sort of thing. I'm sure that stuff slips through, and we see that in crash-stats. Nevertheless, hitting an intentional crash if someone tries to start a worker in such a case probably isn't the end of the world.

Comment 46

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

Also, I think it's fine to avoid getting review from all the modules involved here.

Comment 47

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

(In reply to Bill McCloskey (:billm) from comment #45)
> I'm not sure I understand this comment. Calling
> GetOrCreateForCurrentThread() in the content process on the main thread is
> pretty cheap. It just does a couple allocations and sends an IPC message. In
> addition, the InitXPCOM caller that's being removed is already on the main
> thread and already synchronous (despite the API being used).

The patch was removing the call to GetOrCreateForCurrentThread(aCallback) (which has been synchronous for a while now) without adding a call to GetOrCreateForCurrentThread().  This meant that it would be some different, later caller that would (synchronously) cause the actor to be created.  This has been addressed in the new part 19 that you've reviewed which does add back a call to GetOrCreateForCurrentThread() and so I think we're all on the same page and happy about that.

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fed3aa967a51
Get rid of nsIIPCBackgroundChildCreateCallback - part 1 - BroadcastChannel, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdaa8e4091c4
Get rid of nsIIPCBackgroundChildCreateCallback - part 2 - MessagePort, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0dc9d484856
Get rid of nsIIPCBackgroundChildCreateCallback - part 3 - UDPSocket, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c13a8e40977
Get rid of nsIIPCBackgroundChildCreateCallback - part 4 - MutableBlobStorage, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/b64ffa1024fc
Get rid of nsIIPCBackgroundChildCreateCallback - part 5 - IPCBlob, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/9742e854761a
Get rid of nsIIPCBackgroundChildCreateCallback - part 6 - AsmJS, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7b3c828ed45
Get rid of nsIIPCBackgroundChildCreateCallback - part 7 - FileSystem APIs, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/aae5ad233d21
Get rid of nsIIPCBackgroundChildCreateCallback - part 8 - U2F, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/084984f47abf
Get rid of nsIIPCBackgroundChildCreateCallback - part 9 - Gamepad Test, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/dcc1d3e8fb77
Get rid of nsIIPCBackgroundChildCreateCallback - part 10 - Gamepad API, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/59a00383205e
Get rid of nsIIPCBackgroundChildCreateCallback - part 11 - WebAuthn, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e64bd644469b
Get rid of nsIIPCBackgroundChildCreateCallback - part 12 - Cache API, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f98f06b542e
Get rid of nsIIPCBackgroundChildCreateCallback - part 13 - IndexedDB, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e09df5f05e
Get rid of nsIIPCBackgroundChildCreateCallback - part 14 - QuotaManager, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcaf644bee46
Get rid of nsIIPCBackgroundChildCreateCallback - part 15 - ServiceWorkers, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c7bdc0891da
Get rid of nsIIPCBackgroundChildCreateCallback - part 16 - Workers, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bd31786c1d4
Get rid of nsIIPCBackgroundChildCreateCallback - part 17 - HttpBackgroundChannel, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef23e447c4fa
Get rid of nsIIPCBackgroundChildCreateCallback - part 18 - Layout, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f657b9623fb
Get rid of nsIIPCBackgroundChildCreateCallback - part 19 - ContentChild, r=billm, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1530143d1c54
Get rid of nsIIPCBackgroundChildCreateCallback - part 20 - main part, r=asuth

Comment 49

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fed3aa967a51
https://hg.mozilla.org/mozilla-central/rev/bdaa8e4091c4
https://hg.mozilla.org/mozilla-central/rev/c0dc9d484856
https://hg.mozilla.org/mozilla-central/rev/3c13a8e40977
https://hg.mozilla.org/mozilla-central/rev/b64ffa1024fc
https://hg.mozilla.org/mozilla-central/rev/9742e854761a
https://hg.mozilla.org/mozilla-central/rev/d7b3c828ed45
https://hg.mozilla.org/mozilla-central/rev/aae5ad233d21
https://hg.mozilla.org/mozilla-central/rev/084984f47abf
https://hg.mozilla.org/mozilla-central/rev/dcc1d3e8fb77
https://hg.mozilla.org/mozilla-central/rev/59a00383205e
https://hg.mozilla.org/mozilla-central/rev/e64bd644469b
https://hg.mozilla.org/mozilla-central/rev/2f98f06b542e
https://hg.mozilla.org/mozilla-central/rev/d6e09df5f05e
https://hg.mozilla.org/mozilla-central/rev/fcaf644bee46
https://hg.mozilla.org/mozilla-central/rev/7c7bdc0891da
https://hg.mozilla.org/mozilla-central/rev/5bd31786c1d4
https://hg.mozilla.org/mozilla-central/rev/ef23e447c4fa
https://hg.mozilla.org/mozilla-central/rev/8f657b9623fb
https://hg.mozilla.org/mozilla-central/rev/1530143d1c54