Closed Bug 1337794 Opened 3 years ago Closed Last year

Remove obsolete pagehide event listening from browser.js


(Firefox :: General, defect)

Not set



Firefox 64
Tracking Status
firefox64 --- fixed


(Reporter: Gijs, Assigned: Gijs)




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

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
Depends on: 1332595
Olli, I'm confused by some of the test breakage here. Specifically, applying this patch:

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);
    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)
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)
Assignee: nobody → gijskruitbosch+bugs
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...
Attachment #8971700 - Flags: review?(dtownsend)
Flags: needinfo?(gijskruitbosch+bugs)
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.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8971700 - Attachment is obsolete: true
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+
Pushed by
remove obsolete pagehide handling hacks from browser.js r=mconley
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
You need to log in before you can comment on or make changes to this bug.