1730350 - Investigate confusing e10s check in Clients API IsAssociated/SetAsAssociated for FutureSource management

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, task)

Description

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

Nika identified a confusing !XRE_IsE10sParentProcess check in ClientManagerService::ForgetFutureSource that doesn't make sense because PBackground will always be in the parent-process. It's not clear if this was meant as a check of whether e10s was enabled or something else.

Per Nika's investigation, this was added in https://phabricator.services.mozilla.com/D66520?vs=436189&id=441589#toc.

Edit: It could be good to also have additional comments about the IsAssociated mechanism and what the specific check is doing in that case / why it is impacted by e10s.

Eden, can you take a look and clarify what the intent of the changes were here and what additional work may be appropriate here? Note that Nika provided https://phabricator.services.mozilla.com/D125248 as a general shutdown cleanup which perhaps overlaps with some of the changes here, as I believe Nika's fix addresses a ClientManagerService leak that sounds similar to Perry's last reported problems with the future client source patch stack in https://bugzilla.mozilla.org/show_bug.cgi?id=1584007.

Comment 1

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

Specifically, since the XRE_IsE10sParentProcess() check expands to XRE_IsParentProcess() && BrowserTabsRemoteAutostart(), I guess the question is whether the check really wants to be BrowserTabsRemoteAutostart() or if something else was intended.

Comment 2

[Eden Chuang[:edenchuang]]

I added this because we meet test case fails when the following requirements fulfilled

1. Non-e10s Firefox is used.
2. Loading document under a ServiceWorker's scope.

And the test fails because it supposes that ServiceWorker should intercept the subresource fetching but it doesn't.
The root cause is the different behavior on DocumentLoadListener for ParentProcessDocumentChannel and DocumentChannel.

Instead of DocumentChannel, ParentProcessDocumentChannel is created in the parent process for loading document, of course, non-e10s everything is in the parent process. And process switching would not happen, it tries to connect the channel listener to the underlying channel of ParentProcessDocumentChannel. And DocumentLoadListener would not be needed since it has already been redirected to the real channel. So it releases DocumentLoadListener and the underlying ClientChannelHelperParent. Then ForgetFutureSource will be called to release the future source in ClientManagerService::mSourceTable. But ClientChannelHelper would not be freed under the e10s/fission case, since it needs to wait for the process switching completed.

Then we can consider what happens when a ServiceWorker involves.
Once a network request is determined to be intercepted by a ServiceWorker, StartControllingClient is called, and in the end, a FutureClientSourceParent is created in ClientManagerService and waits for a ClientSourceParent to replace it.
For the e10s/fission case, it works since a real ClientSourceParent is created and attached during the process switching.
But for non-e10s case, the FutureClientSourceParent would be removed before the real ClinetSourceParent is attached. This means the ServiceWorker does not really control any ClientSource. Then the test case fails.

So what I thought is we should deattach the FutureClientSourceParent from ClientChannelHelperParent's life cycle for this special case.
XRE_IsE10sParentProcess() is making sure that we only care about "non-e10s" case
IsAssociated() is making sure there is a ClientHandle operation waits for it, in this case, it means a ServiceWorker is waiting for it.

But it seems that this fix makes some side-effect on leaking FutureClientSourceParent.

Comment 3

[Eden Chuang[:edenchuang]]

Attachment: Bug 1730350 - Add comments to explains why !XRE_IsE10sParetnProcess() is needed in ClientManagerService. r=#dom-worker-reviewers

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a82c89e0fe9
Add comments to explains why !XRE_IsE10sParetnProcess() is needed in ClientManagerService. r=dom-worker-reviewers,asuth

Comment 5

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/1a82c89e0fe9