Closed Bug 1672758 Opened 4 years ago Closed 4 years ago

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

Categories

(Remote Protocol :: Marionette, defect, P1)

Firefox 82
defect

Tracking

(Fission Milestone:M7, firefox-esr78 unaffected, firefox82 wontfix, firefox83 fixed, firefox84 fixed)

RESOLVED FIXED
84 Branch
Fission Milestone M7
Tracking Status
firefox-esr78 --- unaffected
firefox82 --- wontfix
firefox83 --- fixed
firefox84 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

(Blocks 1 open bug, Regression, )

Details

(Keywords: regression, regressionwindow-wanted, Whiteboard: [marionette-fission-mvp][simple], [wptsync upstream])

Attachments

(2 files)

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.

 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.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P1
Regressed by: 1612831
Whiteboard: [marionette-fission-mvp]

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

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

Fission Milestone: --- → M7

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?

Flags: needinfo?(james)

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.

Depends on: 1666204
Blocks: 1673059

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.

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.

Whiteboard: [marionette-fission-mvp] → [marionette-fission-mvp][simple]

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.

Pushed by hskupin@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/26291 for changes under testing/web-platform/tests
Whiteboard: [marionette-fission-mvp][simple] → [marionette-fission-mvp][simple], [wptsync upstream]
Summary: "WebDriver:Back" hangs triggered from within an iframe → "WebDriver:Back" hangs when navigation is triggered from within an iframe and, and frameset is unloaded (no bfcache)
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 84 Branch

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
Attachment #9183554 - Flags: approval-mozilla-beta?
Attachment #9183736 - Flags: approval-mozilla-beta?
Upstream PR merged by moz-wptsync-bot
Flags: needinfo?(james)

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

Flags: needinfo?(james)

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.

Flags: needinfo?(james)

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.

Attachment #9183554 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9183736 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

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

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.

Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: