Closed Bug 1541389 Opened 2 years ago Closed 10 months ago

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

Categories

(Core :: Networking, task, P2)

task

Tracking

()

RESOLVED FIXED
mozilla72
Fission Milestone M4.1
Tracking Status
firefox72 --- fixed

People

(Reporter: valentin, Assigned: valentin)

References

(Depends on 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file)

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

Flags: needinfo?(nika)

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.

Flags: needinfo?(nika)

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.

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

Flags: needinfo?(valentin.gosu)
Priority: -- → P3

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

Flags: needinfo?(valentin.gosu)
Fission Milestone: --- → M4
Depends on: document-channel
Blocks: 1490803

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

Fission Milestone: M4 → M4.1

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

Component: Session Restore → Networking
Flags: needinfo?(matt.woodrow)
Priority: P3 → P2
Product: Firefox → Core

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.

Flags: needinfo?(matt.woodrow)

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

Flags: needinfo?(valentin.gosu)

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.

Flags: needinfo?(valentin.gosu)
Assignee: nobody → valentin.gosu
Type: defect → task
Whiteboard: [necko-triaged]

(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.

Pushed by valentin.gosu@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a6c9464c5be1
Fix browser_httpCrossOriginOpenerPolicy.js to remove timeouts and not wait for SSTabRestored r=mattwoodrow
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.