1522649 - Ensure process switches are supported for ServiceWorker-intercepted channels

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

In bug 1467223, support for performing process switches on in-flight loads was added. That implementation made no changes to InterceptedHttpChannel, so it's unlikely to be compatible with intercepted channels yet.

This bug tracks ensuring that there is ServiceWorker-intercepted channel support for process switching loads.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

ni? valentin - do you have enough context to work on this, or should we try to get engagement from the ServiceWorkers team?

Comment 2

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

Honza has a much better understanding of InterceptedHttpChannel than I do, so I CC'd him to the bug.
It would be great to have some tests for this, or at least a description of a test case.
I suspect there's a case where the ServiceWorker responds with a redirect to another domain, for which we'd need to perform a process switch.

Comment 3

[Honza Bambas (:mayhemer)]

Adding Andrew.

I was not exactly following the process redirect work, so I'm not 100% sure what the expected flow here is. Nika, can you please give me a quick summary of what the expected behavior for SW interception is?

I think the response can be anything - it can be served from the DOM cache, be generated by the intercepting SW script, can be a redirect, can be a response to a completely different http request (I never wrote an interception code and honestly, I'm no expert to SW, so please bare with my possible ignorance here.)

If all requirements are clear, I can help with this bug, but I think Andrew is more capable to do it than me.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Optimistically adding M2 as the milestone.

The process switching logic is still in a fairly nascent stage, and it may be changing over the coming weeks and/or months. A summary of the desired result is:

1. Channel is opened in originating process, allowing it to send stuff like POST data etc.
2. Before OnStartRequest is called on the listener, a it is detected that a process switch needs to happen.
3. The process switch acts as an async redirect, blocking the channel until the new process is avaliable.
4. When the new process is ready, the channel resumes, and sends the OnStartRequest and data to the new process.

This is complicated by a few different factors:

1. Only a limited number of channels support redirection, and those which do seem to have disparate implementations of it.
2. We don't want to flip between processes more often than we have to. Ideally we'd only do 1 process swap for each entire load immediately before OnStartRequest (bug 1522641)

The current logic for doing process swaps was exclusively added to nsHttpChannel, and requires that the first listener be an HttpChannelParentListener. It also performs swaps before all redirects are necessarially completed, by deciding to swap in http-on-examine-response.

This means:

1. Only http(s) loads can do process swaps using this mechanism.
2. If redirecting to a non-http(s) URI, no opportunity to swap processes is granted. (bug 1522640)
3. Unnecessary process swaps occur for each stage in redirection, as the swap occurs before the decision of whether or not to redirect. (bug 1522641)
4. Non nsHttpChannel channels which may redirect do not support process swapping (e.g. InterceptedHttpChannel) (this bug)

Ideally this logic would be generic over all channel types, but it does not appear that there is shared common logic between all channels for injecting redirects & proxying events to content.

Comment 5

[Honza Bambas (:mayhemer)]

I'm not sure what the question for me here is.

But it looks like you are looking for some guidelines how to incorporate InterceptedHttpChannel into this? AFAIK, it's not used right now, interception happens on child. I don't know the roadmap for moving interception to happen on the parent process. I think the better person to talk about this is Andrew Sutherland.

We could potentially completely forget about the child process seeing the intermediate redirects and only let it see the first one in the chain (obviously - created by docshell or content consumers) and the last one (that we decide, solely on the parent process) to use.

I don't know what redirect checks are happening on the child process any longer. I would again have to look at what all redirect veto callbacks are called on the child process. If we find out there is no longer any need to let child process possibly veto, we can only process redirects on the parent process and not try to create child side of each channel to perform the veto checks. Only the last channel would call SendRedirect1Begin, and send it to the right process.

Nika, does this answer things for you?

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Yeah, I'm fairly aware of how the pieces are currently working. The main thing I'm looking for is someone who might be able to help me get the process switching logic working correctly once we have parent process interception, which I think is quite soon.

It sounds like Andrew is the person I should be reaching out to for this right now!

Comment 7

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

So to restate the problem as I understand it:

• Our goal is to execute the navigation (redirect) loop in step 5 of https://html.spec.whatwg.org/#process-a-navigate-fetch in the parent process so that if processing a chain of redirects from origin A to origin B to origin C starting from origin A in process A, we should not create an intermediate process B for origin B. (Because the response comes from the network and we don't need the child process to interpret it for us.)
• We want to make sure this works for ServiceWorkers to the extent it can.

The short answer is that in a fission world, I don't think we can avoid creating process B for origin B.

• If there's a "fetch" handler, we'll need to spin up a process B for the ServiceWorker in origin B to live in. If it responds with a redirection, we'll redirect back to an http channel for next steps (including possible interception and redirection to an intercepted channel for origin C).
• If there isn't a "fetch" handler, we'll reset the interception and redirect back to an http channel.

Which is all to say, I think this may already work and we may not be able to optimize more, but obviously we should have tests for this at least, etc.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Hey, do you have an example of a test which sets up one of these fetch handlers and redirects to a different channel? It would be good to have a test to be confident that this already works.

Comment 9

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

Most redirect-related test coverage comes from web platform tests for ServiceWorkers. We do have a mochitest at https://searchfox.org/mozilla-central/source/dom/serviceworkers/test/test_openWindow.html that does some redirects that has successfully identified a problem with the process swap logic with child intercept. The redirect mode is getting lost, and that's tracked on bug 1535699 and I should have a fix and some more and better-informed opinions on ServiceWorker interactions with process swaps in the next few days. (Your overview in comment 4 was very helpful, BTW, although as with all things, I could only truly appreciate its accuracy and precision after Perry and I spent exciting time tracing things down under searchfox and rr! ;)

In bug 1536826 :edenchuang is writing a browser test that's focused on webextensions, but I think that will have the coverage you desire, although we may want to create a cut-down variant of the test that doesn't know anything about webextenions.

Comment 10

[Nhi Nguyen (:nhi)]

From my reading of the comments, there is already ServiceWorker-intercepted channel support for process switching loads; and that we have (or going to have) some existing tests that verify it.

Nika, is there anything else you need from necko for this bug?

Comment 11

[Nika Layzell [:nika] (ni? for response)]

The plan for process switching loads has been evolving over time. There is definitely more work which needs to be done by Necko for process switching, however I haven't finished doing the write-up yet. I will be reaching out to necko folks once the writeup is complete, so we can hash out the specific design plans.

Comment 12

[Nhi Nguyen (:nhi)]

Neha, could you please review the timeline (currently M3) based on the information above and your team's plan? Thanks!

Comment 13

[Neha Kochar [:neha]]

Nika had sent the write-up over email. We'll discuss the timelines once Dragana has had a chance to discuss the design and plan with the Necko team.

Comment 14

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

This bug should also include a test similar to this: https://bugzilla.mozilla.org/show_bug.cgi?id=1547524#c0

Comment 15

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

This makes sense to proceed on once bug 1231213 has landed (soon).

Comment 16

[Neha Kochar [:neha]]

Perry, could you look into this after bug 1231213?

Comment 17

[Perry Jiang [:perry] [no longer employee, use ni?]]

(In reply to Neha Kochar [:neha] from comment #16)

Perry, could you look into this after bug 1231213?

Sure! Keeping the needinfo until then.

Comment 18

[Perry Jiang [:perry] [no longer employee, use ni?]]

Attachment: Bug 1522649 - test process switches for document loads over intercepted channels r?asuth

Renamed browser_navigation_process_swap.js to avoid confusion with this new test.

Comment 19

[Perry Jiang [:perry] [no longer employee, use ni?]]

Looks like the DocumentChannel subsystem handles process switching for everything, so we just added some test coverage for navigations happening through intercepted channels.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b473b4842be
test process switches for document loads over intercepted channels r=asuth

Comment 21

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/6b473b4842be