1672758 - "WebDriver:Back" hangs when navigation is triggered from within an iframe and frameset is unloaded (no bfcache)

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Reported via https://github.com/mozilla/geckodriver/issues/1796.

The following Marionette test hangs in WebDriver:Back:

        self.marionette.navigate("http://testsite.surfly.com/multisession.html")
        self.marionette.find_element(By.TAG_NAME, "a").click()
        frame = self.marionette.find_element(By.ID, "bing_frame")
        self.marionette.switch_to_frame(frame)
        self.marionette.go_back()

Here the log output from 83 beta and Nightly:

1603382848864	Marionette	DEBUG	2 -> [0,14,"WebDriver:Back",{}]
1603382848867	Marionette	TRACE	Received message beforeunload for https://www.bing.com/?surfly_switch_role=true&toWww=1&redig=9AB3F78B002D42CBBC68CAF9E45DD1F3
1603382848881	Marionette	TRACE	Received message pagehide for https://www.bing.com/?surfly_switch_role=true&toWww=1&redig=9AB3F78B002D42CBBC68CAF9E45DD1F3

In Firefox 82 it looks like:

1603382956071	Marionette	DEBUG	2 -> [0,14,"WebDriver:Back",{}]
1603382956072	Marionette	TRACE	Received message beforeunload for https://www.bing.com/?surfly_switch_role=true&toWww=1&redig=C89C77A57617413BA7272BD4CC4AEE31
1603382956083	Marionette	TRACE	[19] Frame with id 4294967297 got removed
1603382956083	Marionette	TRACE	Received message pagehide for https://www.bing.com/?surfly_switch_role=true&toWww=1&redig=C89C77A57617413BA7272BD4CC4AEE31

I'll have to check which change for Firefox 82.0 broke that back navigation. Interesting is that we have tests that should have covered that. So it would be good to know what's different here.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

 3:41.48 INFO: Last good revision: 24b917577203190f0e1c0b21f6ffaf4d07f1e8f6
 3:41.48 INFO: First bad revision: 4951412ca28ee33b50f4f8af722d01703feb7099
 3:41.48 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=24b917577203190f0e1c0b21f6ffaf4d07f1e8f6&tochange=4951412ca28ee33b50f4f8af722d01703feb7099

So this actually regressed by bug 1612831.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The test for navigating with frames including using bfcache to navigate back and forward is here:

https://searchfox.org/mozilla-central/rev/25d5a4443a7e13cfa58eff38f1faa5e69f0b170f/testing/marionette/harness/marionette_harness/tests/unit/test_navigation.py#488-532

The difference is that this test switches to the parent frame first before navigating backwards in history. Doing that with the above testcase will let it run successfully.

Interesting is that modifying the above testcase to the following allows me to see the same hang for even the 78 ESR release:

        self.marionette.navigate(inline("<p>foo"))
        self.marionette.navigate(inline(iframe("<p>bar")))
        frame = self.marionette.find_element(By.TAG_NAME, "iframe")
        self.marionette.switch_to_frame(frame)
        self.marionette.go_back()

In the working cases (with the original testcase) we received at least the following observer notification, which let us abort the backward navigation:

1603395783119 Marionette TRACE [19] Received observer notification outer-window-destroyed

Comment 3

[Chris Peterson [:cpeterson]]

Tracking marionette-fission-mvp bugs for Fission Beta milestone (M7).

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Checking the WebDriver spec for the back command shows that the command has to:

Traverse the history by a delta –1 for the current browsing context.

Note that the informative section above refers to the top-level browsing context. Also other navigation commands (Navigate To, and Reload) always switch to the top-level browsing context. But that's not done for back/forward, and as such there is no defined behavior in the spec, which covers the case when pagehide is received, but then the frame is removed, and no more events including pageshow are received. So even with it's failing I read it that we obey the current spec, and that it might need an update.

James, can you please weight in here?

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, so by moving the navigation related code to the parent process, the new code isn't able to make use of the outer-window-destroyed observer notification anymore, because it's not received by the parent process. As such and by the missing unit test this particular issue slipped through.

Good news are indeed that I have recently added checks for situations when the browsing context gets discarded. The work landed via bug 1666204. So we can actually make use of it.

And here comes the other problem. Both WebDriver:Back and WebDriver:Forward don't specify the current browsing context for waitForNavigationCompleted, so it actually uses the default which is the top-level browsing context. That means when the currently selected frame gets closed, it's not taken into account because the top-level one is still existent.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1672758 - [marionette] Observe the correct browsing context for navigation events.

While all navigation related commands trigger the navigation through
the top-level browsing context, observing the page load events still
has to happen in the current browsing context.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The attached patch will make it work for pages that don't end-up in bfcache like the original test case.

For bfcache related issues I filed bug 1673059. Those never worked.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1672758 - [wdspec] Add back/forward navigation tests when current iframe is removed.

Without using the browser's bfcache the currently selected iframe will
be destroyed. Make sure that the commands don't run into a timeout error.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/48e5e0c5a800
[wdspec] Add back/forward navigation tests when current iframe is removed. r=webdriver-reviewers,maja_zf
https://hg.mozilla.org/integration/autoland/rev/2841e55e8942
[marionette] Observe the correct browsing context for navigation events. r=marionette-reviewers,maja_zf

Comment 10

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/26291 for changes under testing/web-platform/tests

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/48e5e0c5a800
https://hg.mozilla.org/mozilla-central/rev/2841e55e8942

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 9183554 [details]
Bug 1672758 - [marionette] Observe the correct browsing context for navigation events.

Beta/Release Uplift Approval Request

• User impact if declined: Tests and automation scripts based on our Webdriver implementation will run into a timeout during back and forward navigation if the currently selected frame gets destroyed.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The patch just changes that by default the current browsing context and not the top-level browsing context is observed for being destroyed. By accident both back and forward navigation were doing the latter.
• String changes made/needed: No

Comment 13

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

James, your feedback for comment 4 would still be welcome.

Comment 15

[James Graham [:jgraham]]

Yes, I agree there's a spec bug in that it assumes the current browsing context is still open but that need not be the case. I think it should use "current top level browsing context" in step 3 and then be more specific about where the events might fire in step 4.

Comment 16

[Pascal Chevrel:pascalc]

Comment on attachment 9183554 [details]
Bug 1672758 - [marionette] Observe the correct browsing context for navigation events.

Fix for an 82 regression in Webdriver, safe as it only impacts Webdriver users, has tests and we still have several betas in the cycle. Uplift approved for 83 beta 5, thanks.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to James Graham [:jgraham] from comment #15)

Yes, I agree there's a spec bug in that it assumes the current browsing context is still open but that need not be the case. I think it should use "current top level browsing context" in step 3 and then be more specific about where the events might fire in step 4.

Thanks! I filed https://github.com/mozilla/geckodriver/issues/1801.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/14ef79593cfc
https://hg.mozilla.org/releases/mozilla-beta/rev/5cf2da87dad0

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Regarding Firefox 82, which is also affected, I'm not sure if we need / want an uplift. So far we have seen only a single person reporting the problem. As such it might not qualify as an important patch to have. I will continue to observe the feedback channels.