1597154 - [Fission] URLs that require external helper apps and/or prompting the user reload content processes forever

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[:Gijs (he/him)]

I'm running into this in the automated test I'm adding in bug 1496380. "Manual" STR:

1. open the browser with fission enabled
2. load data:text/html,<a href="fooprotocol:test">Click me</a>
3. click the link

ER:
an error page ("The address wasn't understood"), or a prompt to ask what app we should open the protocol with (try substituting fooprotocol with itunes on a machine that has itunes installed, or a zoom link on a machine that has zoom installed).

AR:
the tab spins forever (in the background, it is basically spawning and killing content processes with the null principal as quickly as it can manage)

What's happening here is the same as in bug 1586148. That is, my fix specifically checked for web handler apps, but didn't deal with the general case of not being able to handle protocols within the browser, and it turns out that case has the same problem...

Comment 1

[Sam Foster [:sfoster] (he/him)]

Moving to backlog.

Comment 2

[Matt Woodrow (:mattwoodrow)]

I think what's happening is that ShouldLoadURI being called from the docshell, and onMayChangeProcess being called by DocumentChannel are disagreeing on what process we should do the load in.

ShouldLoadURI causes us to cancel the current load, switch process and try again. onMayChangeProcess redirects the current load into a new process.

It looks like we redirect the load into a new process, and then ShouldLoadURI rejects it and starts it in another new process. Repeat.

We should probably do a few things:

• Not check ShouldLoadURI when we're picking up a redirected load (ResumeRedirectedLoad calls into InternalLoad in that case), since the decision has already been made.
• Not check ShouldLoadURI for protocols that DocumentChannel makes a decision for (we have some checks in E10SUtils, but they're a subset of DC's protocols).

Comment 3

[Matt Woodrow (:mattwoodrow)]

I guess adding logging to the two decision making paths in E10SUtils to understand why we come up with different results would be good too.

Comment 4

[Chris Peterson [:cpeterson]]

Tracking for Fission dogfooding (M5)

Comment 5

[Paul Bone [:pbone]]

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

I think what's happening is that ShouldLoadURI being called from the docshell, and onMayChangeProcess being called by DocumentChannel are disagreeing on what process we should do the load in.

ShouldLoadURI causes us to cancel the current load, switch process and try again. onMayChangeProcess redirects the current load into a new process.

I had some debugging printfs in onMayChangeProcess but I didn't see it as part of the loop. But ShouldLoadURI is.

It looks like we redirect the load into a new process, and then ShouldLoadURI rejects it and starts it in another new process. Repeat.

We should probably do a few things:

• Not check ShouldLoadURI when we're picking up a redirected load (ResumeRedirectedLoad calls into InternalLoad in that case), since the decision has already been made.

I'm uploading a patch that does this and fixes the problem.

• Not check ShouldLoadURI for protocols that DocumentChannel makes a decision for (we have some checks in E10SUtils, but they're a subset of DC's protocols).

I also fixed the problem by making the list in E10SUtils.jsm match the one in nsDocShell.cpp Is that what you meant with this bullet point.

EIther patch applied by itself fixes the issue. My guess is that we should definitely apply the first and probably also apply the second.

Comment 6

[Paul Bone [:pbone]]

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

I guess adding logging to the two decision making paths in E10SUtils to understand why we come up with different results would be good too.

Two paths in E10SUtils? There's a big of spagetti in there so I thought it was one tangly path with multiple entry points. Where is the second?

Comment 7

[Paul Bone [:pbone]]

Attachment: Bug 1597154 - pt 1. Add a comment about checking GetPendingRedirectedChannel r=mattwoodrow

Comment 8

[Paul Bone [:pbone]]

Attachment: Bug 1597154 - Make the E10SUtils protocol tests match the nsDocShell ones r=mattwoodrow

Depends on D57597

Comment 9

[Paul Bone [:pbone]]

Attachment: Bug 1597154 - pt 4. Enable browser_protocolhandler_loop.js for fission r=mattwoodrow

Depends on D57598

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:pbone, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 11

[Paul Bone [:pbone]]

Attachment: Bug 1597154 - pt 2. Fix out-of-date comment r=mattwoodrow

Depends on D57597

Comment 12

[Paul Bone [:pbone]]

Attachment: Bug 1597154 - pt 3. Defer to documentchannel for process switching for all schemes r=mattwoodrow

Depends on D58898

Comment 13

[Paul Bone [:pbone]]

Attachment: Bug 1597154 - pt 4. Rename SchemeUsesDocChannel

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9580b0a08782
pt 1. Avoid redirecting loads between processes more than once r=mattwoodrow,kmag
https://hg.mozilla.org/integration/autoland/rev/cc9c332c22a6
pt 2. Fix out-of-date comment r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/1638767d4977
pt 3. Defer to documentchannel for process switching for all schemes r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/8f8a663e3118
pt 4. Enable browser_protocolhandler_loop.js for fission r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/051d6f3a237c
pt 5. Rename SchemeUsesDocChannel r=mattwoodrow

Comment 15

[Dorel Luca [:dluca]]

Backed out 5 changesets (bug 1597154) for Mochitest error in docshell/test/mochitest/test_bug529119-2.html. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285146897&repo=autoland&lineNumber=2053

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedJob=285150279&revision=051d6f3a237c14a883a3712360dafddeaf23b6c2

Backout:
https://hg.mozilla.org/integration/autoland/rev/f41766fb940707b210530250d9017acca09ca40f

Comment 16

[Andreea Pavel [:apavel]]

There are also bc failures such as https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285146900&repo=autoland&lineNumber=19988
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285153354&repo=autoland&lineNumber=13858 and https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285154323&repo=autoland&lineNumber=22256

and devtools failures https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285147873&repo=autoland&lineNumber=23833

other mochitest failures: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=285153796&repo=autoland&lineNumber=23578
wpt failure: https://treeherder.mozilla.org/logviewer.html#?job_id=285163264&repo=autoland

Comment 17

[Paul Bone [:pbone]]

Attachment: Bug 1597154 - Fixes for part 3 r=mattwoodrow,nika

Either of you can take a look, I'm testing these fixes now to see if I can
land this patch series and would also appreciate a review before I commit to
landing them.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c34ca1e2079
pt 1. Avoid redirecting loads between processes more than once r=mattwoodrow,kmag
https://hg.mozilla.org/integration/autoland/rev/e8dccb59bf2a
pt 2. Fix out-of-date comment r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/9857504c26e6
pt 3. Defer to documentchannel for process switching for all schemes r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/6e82c600d52f
pt 4. Enable browser_protocolhandler_loop.js for fission r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/dc78c6d3d737
pt 5. Rename SchemeUsesDocChannel r=mattwoodrow

Comment 20

[Noemi Erli[:noemi_erli]]

Backed out 5 changesets (Bug 1597154) for causing browser-chrome failures in browser_UITour_showNewTab.js CLOSED TREE

Range of failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=289800757&resultStatus=testfailed%2Cbusted%2Cexception&tochange=7302618003983e94d9bc5b91930e96aa0deb7ff4&fromchange=dc78c6d3d7373db528c4ae4eb047f8bb87c73466

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=289800757&repo=autoland&lineNumber=21758

Backout: https://hg.mozilla.org/integration/autoland/rev/7e080cd2c698345c0b697f91c9956ff7924fed67

Comment 21

[Paul Bone [:pbone]]

:-(

I want to break this bug up into seperate tasks. One of the troubles getting it to land is that it flips the DC whitelist into a blacklist and that's catching all sorts of problems and therefore also worthy of being it's own unit of work. I'll break other tasks off from this bug also.

Comment 22

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Once this is fixed, the test in bug 1528305 should be enabled for Fission.

Comment 26

[Paul Bone [:pbone]]

Attachment: Bug 1597154 - pt 2. Refactor shouldLoadURI r=mattwoodrow

Refactoring in shouldLoadURI to compute remoteType and wantRemoteType once
and share them on two code paths. This also requires us to change
validatedRemoteType so it can handle more cases where the URI is provided.

Depends on D57597

Comment 27

[Paul Bone [:pbone]]

Attachment: Bug 1597154 - pt 3. Flip documentchannel whitelist to blacklist r=mattwoodrow

This fixes 1597154 because the lists of URIs here and in nsDocShell now
match.

Depends on D69135

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/853feeec896d
pt 1. Add a comment about checking GetPendingRedirectedChannel r=mattwoodrow,kmag
https://hg.mozilla.org/integration/autoland/rev/bed6c14b61df
pt 2. Refactor shouldLoadURI r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/73c783db555a
pt 3. Flip documentchannel whitelist to blacklist r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/08b92b1c769e
pt 4. Rename SchemeUsesDocChannel r=mattwoodrow

Comment 29

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/853feeec896d
https://hg.mozilla.org/mozilla-central/rev/bed6c14b61df
https://hg.mozilla.org/mozilla-central/rev/73c783db555a
https://hg.mozilla.org/mozilla-central/rev/08b92b1c769e