1541389 - When performing a process switch BrowserTestUtils.browserStopped fires before the channel is reloaded in the new process

Status: RESOLVED FIXED

(Core :: Networking, task, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

If I comment out the wait for "SSTabRestored" event in [1] then I can see in the logging that Navigated to ... from the test is printed before OnStopRequest is called for the new channel in the new process.

This seems wrong and is partly the reason why I added timeouts to the test.

[1] https://searchfox.org/mozilla-central/rev/44a212460990ffffecf50a8e972d3cbde2e7216b/toolkit/components/remotebrowserutils/tests/browser/browser_httpCrossOriginOpenerPolicy.js#15-16,42-44

Comment 1

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

I agree that the behaviour right now in process switches isn't great. I believe right now you're supposed to use https://searchfox.org/mozilla-central/rev/9ee63566281365f26e7a4b06c9d4e2219e64c3e8/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm#299-323 to detect when a browser has finished loading, and I think it handles process switching "correctly". It does this by waiting for a load event to be fired in the content process, and a corresponding message sent, which shouldn't occur in the previous process which fails to finish its load before being killed.

In the future we'll definitely want to fix this, but I'm not sure it's a straightforward fix using our current process-switching architecture, so it may have to wait until we get our new system stood up.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

I remember having trouble with browserLoaded too, in terms of raciness. So I wonder if the failed load from the previous process sometimes happens before the process is destroyed.

Comment 3

[Mike de Boer [:mikedeboer]]

Valentin, do you need to keep this bug around? If so, is Session Restore really the best component, since you're talking about browserLoaded?

Comment 4

[Valentin Gosu [:valentin] (he/him)]

The process switching logic is implemented in SessionStore.jsm
If not in Session Restore, where would we put this bug? Testing:General?

Comment 5

[Chris Peterson [:cpeterson]]

Roll unfixed test bugs from Fission Milestone M4 to M4.1

Comment 6

[Neha Kochar [:neha]]

Matt, can you check if this is still an issue with DocumentChannel? We'll need this for re-SAB.

Comment 7

[Matt Woodrow (:mattwoodrow)]

The test is enabled with fission, so I assume it's fixed?

Nika did some work on top of DocumentChannel to make sure we delay notifying the old docshell that it had been cancelled until we'd setup the new one.

Comment 8

[Neha Kochar [:neha]]

Valentin, can you check if this is still an issue?

Comment 9

[Valentin Gosu [:valentin] (he/him)]

I've just checked, and this appears to be fixed in m-c.
I'll submit a patch shortly to remove the workarounds in the test.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1541389 - Fix browser_httpCrossOriginOpenerPolicy.js to remove timeouts and not wait for SSTabRestored r=mattwoodrow

(Matt Woodrow (:mattwoodrow) in bug 1541389 comment #7)

Nika did some work on top of DocumentChannel to make sure we delay
notifying the old docshell that it had been cancelled until we'd setup
the new one.

This patch removes the code that waits for the SSTabRestored event when a
process switch is expected.
It also removes the setTimeout in the test, and uses
browser.frameLoader.remoteTab.osPid instead of ContentTask.spawn to get the
PID of the content process.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6c9464c5be1
Fix browser_httpCrossOriginOpenerPolicy.js to remove timeouts and not wait for SSTabRestored r=mattwoodrow

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/a6c9464c5be1