Closed Bug 1704697 Opened 1 month ago Closed 4 days ago

Hangs in "WebDriver:Back" and "WebDriver:Forward" with BFCache enabled in parent process (for Fission)

Categories

(Testing :: Marionette, defect, P1)

Firefox 89
defect

Tracking

(Fission Milestone:M7a, firefox90 fixed)

RESOLVED FIXED
90 Branch
Fission Milestone M7a
Tracking Status
firefox90 --- fixed

People

(Reporter: neha, Assigned: whimboo)

References

(Blocks 2 open bugs)

Details

Attachments

(5 files, 1 obsolete file)

These tests fail with BFCache+Fission
webdriver/tests/back/back.py
webdriver/tests/back/user_prompts.py
webdriver/tests/close_window/close.py
webdriver/tests/element_click/navigate.py
webdriver/tests/execute_script/execute.py
webdriver/tests/forward/forward.py
webdriver/tests/forward/user_prompts.py

failure log: https://treeherder.mozilla.org/logviewer?job_id=332690156&repo=try&lineNumber=2112

Prefs to use: fission.autostart and fission.bfcacheInParent

:whimboo, would you be able to debug these failures? If you see a DOM issue, please NI smaug or peterv. Thanks!

Blocks: webdriver
No longer depends on: 1703606
Flags: needinfo?(hskupin)
Version: Default → Firefox 89
Fission Milestone: --- → M7a

I'll have a look at those tests when I'm done with the new tab modal dialog support in Marionette.

Note that the WebDriver spec doesn't contain anything around bfcache yet. As such we won't have to block bug 721859.

No longer blocks: webdriver
Severity: -- → S3
Priority: -- → P1

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #2)

I'll have a look at those tests when I'm done with the new tab modal dialog support in Marionette.

In that case, I will tentatively assign this bug to you to determine the next action.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED

So I had a quick look at the problems with Wdspec tests and the problem here is that the top-level browsing context gets swapped when the target page gets loaded from BFCache. Due to this swap the following check fails, and events as reported by the correct JSWindowActor instance (but different browsing context id now) are simply ignored:

https://searchfox.org/mozilla-central/rev/72951aa826642f048da4c6b71b8b3e36a9606dcd/testing/marionette/navigate.js#283-285

So far when a navigation (e.g COOP) causes a process change we used the browsing-context-discarded notification and continued waiting for the navigation to complete if the browsing context has been replaced:

https://searchfox.org/mozilla-central/rev/72951aa826642f048da4c6b71b8b3e36a9606dcd/testing/marionette/navigate.js#316-327
https://searchfox.org/mozilla-central/source/testing/marionette/driver.js#639-664

I wonder if there could be a dedicated notification that informs about a top-level browsing context swap like browsing-context-swapped. Nika, would there anything like that doable?

If not we might be able to get it done by improving the check in onNavigation by also allowing events from another top-level browsing context that has the same browserId. The downside is still that we won't recognize that swap if some other code that might be executed via execute_script triggers a back/forward navigation.

So an event that we could listen for would be great to have.

Flags: needinfo?(hskupin) → needinfo?(nika)

You might be able to use the XULFrameLoaderCreated event which is fired by the <browser> element whenever a toplevel process switch of any kind has occurred: https://searchfox.org/mozilla-central/rev/72951aa826642f048da4c6b71b8b3e36a9606dcd/dom/base/nsFrameLoaderOwner.cpp#224. At that point, if the target browser is the one you're interested in, IIRC its .browsingContext attribute should be updated to the new one.

Flags: needinfo?(nika)

Thanks Nika! That is actually the same as Olli also mentioned to me recently. I quickly tried to register this event and it seems to work fine for us. By using that we should be able to replace the whole logic around browsing-context-attached.

As testing has shown there is also bug 1680479, which is now happening way more often and which needs to bee also fixed before BFCache can be enabled for Fission when running Marionette tests. Nevertheless I got most of the previously skipped tests re-enabled (except the certificate error test from test_navigation.py). So navigation related issues should be (hopefully) all fixed once this bug is fixed.

Due to bug 1680479 we should not enable BFCache by default yet for Marionette.

Summary: Fix webdriver tests that fail with BFCache+Fission → Hangs in "WebDriver:Back" and "WebDriver:Forward" with BFCache enabled in parent process (for Fission)

I hope bug 1703607 helps with the about:certerror case.

When there is a remoteness change the order of page load events
is not guaranteed. As such a "pagehide" event from the current
page might be received after a "pageshow" event from the target
page.

Depends on D113767

Especially with BFCache enabled for Fission there is no new
browsing context created when a page comes out of the cache.
As such the current logic to detect remoteness changes doesn't
work. Instead the "XULFrameLoaderCreated" event on the tabbrowser
object can be used instead, which is always fired.

Depends on D113768

(In reply to Olli Pettay [:smaug] (away May 1 - 8) from comment #8)

I hope bug 1703607 helps with the about:certerror case.

It doesn't help at least with those changes that already landed. As such I will keep this single test disabled.

Attachment #9219162 - Attachment description: WIP: Bug 1704697 - [marionette] Remove unused frameRegsPending property from Context class. → Bug 1704697 - [marionette] Remove unused frameRegsPending property from Context class.
Attachment #9219163 - Attachment description: WIP: Bug 1704697 - [marionette] Enable "fission.bfcacheInParent" by default. → Bug 1704697 - [marionette] Enable "fission.bfcacheInParent" by default.
Attachment #9219164 - Attachment description: WIP: Bug 1704697 - [marionette] Don't require a page unload event for navigation when a remoteness change occurred. → Bug 1704697 - [marionette] Don't require a page unload event for navigation when a remoteness change occurred.
Attachment #9219165 - Attachment description: WIP: Bug 1704697 - [marionette] Use the XULFrameLoaderCreated event to detect remoteness changes. → Bug 1704697 - [marionette] Use the XULFrameLoaderCreated event to detect remoteness changes.
Attachment #9219163 - Attachment is obsolete: true
See Also: → 1706353
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2a390ce81a9
[marionette] Remove unused frameRegsPending property from Context class. r=marionette-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/64e2c34d5cbb
[wdspec] Add cross-origin navigation tests. r=webdriver-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/079b8863727b
[marionette] Don't require a page unload event for navigation when a remoteness change occurred. r=marionette-reviewers,smaug,jdescottes
https://hg.mozilla.org/integration/autoland/rev/6f587a056ef3
[marionette] Use the XULFrameLoaderCreated event to detect remoteness changes. r=marionette-reviewers,smaug,jdescottes
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28934 for changes under testing/web-platform/tests
Upstream PR was closed without merging

Reason here is the following failure: TypeError: tabBrowser.addEventListener is not a function.

It's because the mobile browser implementation adds a shim around the tab browser:
https://searchfox.org/mozilla-central/rev/716d7cd80b3dcd81c005ad13b51d3e6a85bd7e73/testing/marionette/browser.js#71-104

I'll have to figure out where I have to register for the event, and if it is available at all.

Flags: needinfo?(hskupin)
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d8615fb91eed
[marionette] Remove unused frameRegsPending property from Context class. r=marionette-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/7a9ad3123c6f
[wdspec] Add cross-origin navigation tests. r=webdriver-reviewers,jgraham
https://hg.mozilla.org/integration/autoland/rev/07367d3fdd97
[marionette] Don't require a page unload event for navigation when a remoteness change occurred. r=marionette-reviewers,smaug,jdescottes
https://hg.mozilla.org/integration/autoland/rev/78a4c80b04a7
[marionette] Add addEventListener and removeEventListener to MobileTabBrowser shim. r=marionette-reviewers,agi,jgraham
https://hg.mozilla.org/integration/autoland/rev/a381d0b29463
[marionette] Use the XULFrameLoaderCreated event to detect remoteness changes. r=marionette-reviewers,smaug,jdescottes
Upstream PR merged by moz-wptsync-bot
You need to log in before you can comment on or make changes to this bug.