Don't trigger a process switch before loading when DocumentChannel could do it in the response.
Categories
(Core :: DOM: Navigation, task, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox74 | --- | fixed |
People
(Reporter: mattwoodrow, Assigned: mattwoodrow)
References
Details
Attachments
(3 files)
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.
Assignee | ||
Comment 1•5 years ago
|
||
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.
Assignee | ||
Comment 2•5 years ago
|
||
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•5 years ago
|
||
(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?
Assignee | ||
Comment 4•5 years ago
|
||
(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•5 years ago
|
||
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•5 years ago
|
||
(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•5 years ago
|
||
(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.
Assignee | ||
Comment 8•5 years ago
|
||
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.
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 9•5 years ago
|
||
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
Assignee | ||
Comment 10•5 years ago
|
||
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
Assignee | ||
Comment 11•5 years ago
|
||
Depends on D58889
Comment 12•5 years ago
|
||
Comment 13•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b4e8cff48119
https://hg.mozilla.org/mozilla-central/rev/9e48f539d4ef
https://hg.mozilla.org/mozilla-central/rev/a9b06feb5942
Description
•