Closed
Bug 1337794
Opened 7 years ago
Closed 6 years ago
Remove obsolete pagehide event listening from browser.js
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: Gijs, Assigned: Gijs)
References
Details
Attachments
(1 file, 1 obsolete file)
Followup from bug 1332595 because it seems that removing the pagehide listener as well as the click listener in this block of code: https://dxr.mozilla.org/mozilla-central/rev/f4f374622111022d41dd8d5eb9220624135c534a/browser/base/content/browser.js#4936-4967 causes focus-related failures in a number of tests that are hard to debug / trace. Because we need to remove the click handling for more reasons than just "this is unnecessary code" and those changes need uplift, splitting the final cleanup off from bug 1332595
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=96b872cf0348171b82de22f07f78b50a55516ec8
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 2•7 years ago
|
||
Olli, I'm confused by some of the test breakage here. Specifically, applying this patch: https://hg.mozilla.org/try/rev/00e70562391283ff7a31fb5e3c31c1166f95fe27 and then running: ./mach mochitest browser/base/content/test/popups/browser_popup_frames.js --disable-e10s fails, because the test's waiting for a pagehide event to fire from an iframe (using a capturing event listener), and that never happens: await ContentTask.spawn(tab.linkedBrowser, baseURL, async function(uri) { let iframe = content.document.createElement("iframe"); iframe.src = uri; let pageHideHappened = ContentTaskUtils.waitForEvent(this, "pagehide", true); content.document.body.appendChild(iframe); await pageHideHappened; }); which causes the test to time out. I can fix this by applying the following patch: diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js --- a/browser/base/content/browser.js +++ b/browser/base/content/browser.js @@ -5003,16 +5003,18 @@ var TabsProgressListener = { TelemetryStopwatch.finish("FX_PAGE_LOAD_MS", aBrowser); } } else if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP && aStatus == Cr.NS_BINDING_ABORTED && stopwatchRunning /* we won't see STATE_START events for pre-rendered tabs */) { TelemetryStopwatch.cancel("FX_PAGE_LOAD_MS", aBrowser); } } + + let doc = aBrowser.isRemoteBrowser ? null : aWebProgress.DOMWindow.document; }, onLocationChange(aBrowser, aWebProgress, aRequest, aLocationURI, aFlags) { // Filter out location changes caused by anchor navigation // or history.push/pop/replaceState. if (aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_SAME_DOCUMENT) { // Reader mode actually cares about these: Why does requesting the document associated with the request in non-e10s make a difference as to whether the pagehide event fires here (and why only in non-e10s)?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bugs)
Comment 3•7 years ago
|
||
You get pagehide for the initial about:blank, right? And that isn't created for some reason in non-e10s. We try to not create initial about:blank if possible. That is my guess here.
Flags: needinfo?(bugs)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Assignee | ||
Comment 5•6 years ago
|
||
So I found a fix for the test in comment #2 / comment 3 that's in the patch here. However, this is still orange on try for some subtests of layout/base/tests/test_reftests_with_caret.html on linux32 only. It seems to have to do with how focus is applied to the test frame (the first reftest failure is because there's a focus outline on the entire frame - the contents are otherwise fine). At the moment I'm not sure how to fix. I'll continue to poke at this, but if anyone has suggestions that would be helpful...
Assignee | ||
Updated•6 years ago
|
Attachment #8971700 -
Flags: review?(dtownsend)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 7•6 years ago
|
||
The pagehide handler has been as dead as a doornail for a while now, but the side-effect of forcing the creation of an about:blank document in non-remote windows was implicitly relied upon by some tests. This removes the dead code and fixes up some tests.
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Updated•6 years ago
|
Attachment #8971700 -
Attachment is obsolete: true
Assignee | ||
Comment 8•6 years ago
|
||
Latest trypush: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0faba30ac17cbc47675ecb04fdd97052616643cf
Comment 9•6 years ago
|
||
Comment on attachment 9006317 [details] Bug 1337794 - remove obsolete pagehide handling hacks from browser.js Mike Conley (:mconley) (:⚙️) has approved the revision.
Attachment #9006317 -
Flags: review+
Comment 10•6 years ago
|
||
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0a74d4a7afd9 remove obsolete pagehide handling hacks from browser.js r=mconley
Comment 11•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0a74d4a7afd9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in
before you can comment on or make changes to this bug.
Description
•