1601779 - Don't trigger a process switch before loading when DocumentChannel could do it in the response.

Status: RESOLVED FIXED

(Core :: DOM: Navigation, task, P2)

Description

[Matt Woodrow (:mattwoodrow)]

DocumentChannel handles performing a process switch after getting a response (and thus knows the final origin/principal).

We also have other code paths for switching process before we start a load. These are necessary when switching in or out of the parent process (which isn't supported by DocumentChannel - yet).

These shouldn't be necessary for other switches though, and we can't fix bug 1588143 with them (since we process switch, and then end up without a document to load since it gets downloaded). They also mean we can process switch for the initial URI, and then need to switch again during the reponse due to a redirect.

The two ways this can happen are:

nsDocShell calls ShouldLoadURI, which jumps through some hoops and ends up in E10SUtils.shouldLoadURI. This code has checks for the cases that can be handled by DocumentChannel, but is out of date (only handles http(s) and data).

The browser.js frontend _loadURI calls E10SUtils.shouldLoadURIInBrowser. This one doesn't have any checks, and is the reason that bug 1588143 fails.

Comment 1

[Matt Woodrow (:mattwoodrow)]

kmag points out that that this leaks the new URI into the old process, which is something we want to avoid wherever possible.

Given that this will fix bug 1588143 (and real observable behaviour), then I think we need to do this for now, and work on bug 1602318 as the longer term fix.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Doing this causes some new failures: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=280188504&revision=6aabb4893315962eb4f9ab5b353500e58bd3a955

I just debugged the browser/base/content/test/general/browser_alltabslistener.js errors.

The issue is that we're firing

onStateChange(START - 0xf0001) and then onStateChange(STOP - 0xc0010) from the old content process, and then the same pairing from the new content process. The stop from the old content process makes us think we've finished the test, and the extra results cause a lot of failures.

Does anyone have an idea of what we should do here?

We could:

• Suppress the inner (stop, start) pairing. This makes things sane for the front-end/parent-process/RemoteWebProgress, but a bit weird for in-process listeners.
• Add a flag (STATE_PROCESS_SWITCH?) for the inner (stop, start) pairing so that the front-end (and this test) can choose to ignore them.
• Maybe suppress them just within RemoteWebProgress?
• Update the test to expect this state, figure out front-end changes to do the same.

Comment 3

[:Gijs (he/him)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)

Doing this causes some new failures: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=280188504&revision=6aabb4893315962eb4f9ab5b353500e58bd3a955

I just debugged the browser/base/content/test/general/browser_alltabslistener.js errors.

The issue is that we're firing

onStateChange(START - 0xf0001) and then onStateChange(STOP - 0xc0010) from the old content process, and then the same pairing from the new content process. The stop from the old content process makes us think we've finished the test, and the extra results cause a lot of failures.

I'm not sure I understand. Are we loading the same URI twice? If so, why? If not, why do we notify twice?

Comment 4

[Matt Woodrow (:mattwoodrow)]

(In reply to :Gijs (he/him) from comment #3)

I'm not sure I understand. Are we loading the same URI twice? If so, why? If not, why do we notify twice?

Well, we start loading it in one process, realize that it needs to load in a different one (due to fission), and then load it properly in a new process.

Comment 5

[Kris Maglione [:kmag]]

I think we probably want to skip sending STATE_STOP notifications over RemoteWebProgress when the request is about to change process. I think that would give us the most consistent view of what's happening on the parent side. As far as the child is concerned, the request has stopped at that point, because it's only allowed to be concerned about its own docshell. But from the parent's perspective, which is more concerned with the frameloader, the request is still ongoing, albeit being moved to a different process.

Comment 6

[:Gijs (he/him)]

(In reply to Kris Maglione [:kmag] from comment #5)

But from the parent's perspective, which is more concerned with the frameloader, the request is still ongoing, albeit being moved to a different process.

Should we then also stop sending a second STATE_START to the parent from the new process? Perhaps the documentchannel can keep track of whether a state_start has already been fired, and avoid sending it again?

Comment 7

[Kris Maglione [:kmag]]

(In reply to :Gijs (he/him) from comment #6)

(In reply to Kris Maglione [:kmag] from comment #5)

But from the parent's perspective, which is more concerned with the frameloader, the request is still ongoing, albeit being moved to a different process.

Should we then also stop sending a second STATE_START to the parent from the new process? Perhaps the documentchannel can keep track of whether a state_start has already been fired, and avoid sending it again?

I think so, yes. We should aim to give the parent as consistent a view of what's happening as possible.

Comment 8

[Matt Woodrow (:mattwoodrow)]

I spun the nsIWebProgressListener issue into a separate bug, and also filed a few others. These are all existing bugs, doing this just adds better test coverage to expose them.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1601779 - Change test expectations since onSecurityChange can come second now. r?Gijs

The extra onSecurityChange message comes from the process switch, and this patch queue changes the timing of when that happens (to be once we receive a response, not before the load starts).

Depends on D56818

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1601779 - Don't process switch when we actually abort a channel load. r?kmag

We currently do a process switch for a failed load so that we show an error page in the right process. This adds an exception for loads that we just explicitly cancelled, since it shouldn't be necessary, and so that tests that use BrowserUtils.waitForDocLoadAndStopIt don't get confused.

Depends on D58888

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1601779 - Try using response process selection more often for front-end loads. r?kmag

Depends on D58889

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4e8cff48119
Change test expectations since onSecurityChange can come second now. r=dao
https://hg.mozilla.org/integration/autoland/rev/9e48f539d4ef
Don't process switch when we actually abort a channel load. r=kmag
https://hg.mozilla.org/integration/autoland/rev/a9b06feb5942
Try using response process selection more often for front-end loads. r=kmag

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/b4e8cff48119
https://hg.mozilla.org/mozilla-central/rev/9e48f539d4ef
https://hg.mozilla.org/mozilla-central/rev/a9b06feb5942