click and page load aborts too early if target URL loads too slowly (delayed pagehide event)

NEW
Unassigned

Status

defect
P3
normal
a year ago
10 months ago

People

(Reporter: whimboo, Unassigned)

Tracking

(Depends on 1 bug, Blocks 2 bugs)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 wontfix, firefox62 affected, firefox63 affected)

Details

()

Attachments

(1 attachment)

Originally reported as: https://github.com/mozilla/geckodriver/issues/1236

If the target page is loading slowly and exceeds our hard-coded 5s timeout for `pagehide` the load listener will be canceled. This is unfortunate given that `pagehide` seem to get fired when the first data from the target page arrives. So our current logic doesn't make that much sense, and we should re-think in how to improve the situation. I might find some time this week to have a look at.

Here an example Marionette test which demonstrates the issue and is not covered by any unit test at the moment:

>     def test(self):
>         target = self.marionette.absolute_url("slow?delay=10")
>         self.marionette.navigate(self.inline("""<a id="link" href="{}">Slow</a>""".format(target)))
>         link = self.marionette.find_element(By.ID, "link")
>         link.click()
>         self.marionette.find_element(By.ID, "delay")
Summary: click and page load aborts too early if target URL loads too slowly → click and page load aborts too early if target URL loads too slowly (delayed pagehide event)
Sounds like a conformance issue.
Priority: P3 → P2
Comment hidden (offtopic)
Sorry, that this bug was lost in my queue. I just had a quick look and found the following...

Given that `beforeunload` handlers are not allowed by the WebDriver specification for all the navigation and close window commands, and we have that implemented since bug 1434872, it should be easier now to simplify the page load logic.

Means when we detect an `onbeforeunload` event we can assume that a page load is undergoing, and as such directly wait for the page being finished loading. There shouldn't be a need to wait for the `pagehide` event too anymore.

I will see if I can fix this today.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Depends on: 1434872
Priority: P2 → P1
Comment hidden (mozreview-request)
Here are the try results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=51b78995fb9751d828dc0cb5210b1a39a93a7c5d&selectedJob=191060385

It looks pretty good except that `test_click_link_install_addon` is failing now. Interestingly it was only working because we had this extra timeout of 5s included, but which should not have made it work at the first place. Basically installing an extension should return from the page load event with the same logic as for unknown MIME types which is bug 1366035.

Dave, I feel that fixing this bug has a high priority, even when we will have a regression on our side for clicking a link which installs an addon. I wonder if you could live with a regression in that area until bug 1366035 has been fixed?
Flags: needinfo?(dave.hunt)
I believe there's an AMO test that depends on installing an add-on from a link. I don't know which version of Firefox they target. Are you anticipating that this regression would only affect Nightly? Benny: Could you comment on the AMO test(s)?
Flags: needinfo?(dave.hunt) → needinfo?(bennyjr169)
We have 1 install test for AMO, but we also test installation on the Discovery Pane. We test both on nightly specifically too.
Flags: needinfo?(bennyjr169)
Ok, so it looks like we cannot fix this bug until bug 1366035 has been properly fixed.
Assignee: hskupin → nobody
Status: ASSIGNED → NEW
Depends on: 1366035
Priority: P1 → P3
You need to log in before you can comment on or make changes to this bug.