Don't run BrowserOnAboutPageLoad twice for each about page load

RESOLVED FIXED in Firefox 21

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
Firefox 21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

7 years ago
Posted patch patch v1.0 (obsolete) — Splinter Review
Most of the discussion is in bug 749477, to sum up:

- we add listeners to any about page, at least for about:blank this is an unwanted perf hog. We may evaluate to filter out about:newTab as well.
- we do that multiple times, cause some document load causes multiple STATE_STOP invokations, while others cause background loads that again cause multiple STATE_STOP (transitioning from about:blank to the actual error page)
- there is no easy way to distinguish STATE_STOP and know if the current one is also the latest one
- there is no easy way to detect load of error pages, only DOMContentLoad works, but comes before the last STATE_STOP
- error pages STATE_STOP invokations are wrongly filtered out by the isSuccessCode(aStatus) condition

This is a suggested patch that filters out about:blank and avoids invoking browserOnAboutPageLoad multiple times.
Assignee

Updated

7 years ago
Depends on: 749477
I had forgotten that Frank filed bug 764989 already - I guess we can dupe that here?
Oh, actually I guess that is a different (related) bug.

Comment 3

7 years ago
(In reply to Marco Bonardo [:mak] from comment #0)
> - we add listeners to any about page, at least for about:blank this is an
> unwanted perf hog. We may evaluate to filter out about:newTab as well.

(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #1)
> I had forgotten that Frank filed bug 764989 already - I guess we can dupe
> that here?

I filed that (bug 764989) and made a patch and requested review from you months ago to address the first point that Marco repeated above. I had that fix included in my fix for bug 682944, but you had asked that I spin it off into a separate bug, but then my spun-off patch got ignored. It's really a subset of this bug.

Updated

7 years ago
Duplicate of this bug: 764989
It wouldn't make sense to cut and paste SeaMonkey's offline error page DOMContentLoaded event handler code but it works something like this:

  if (/^about:neterror\?e=netOffline/.test(event.target.documentURI)) {
    event.target.addEventListener("click", function tryAgain(event) {
      if (event.target.id == "errorTryAgain") {
        Services.io.offline = false;
      }
    }, true);
  }
Comment on attachment 660816 [details] [diff] [review]
patch v1.0

>+    let document = aWebProgress.DOMWindow.document;
>     if (aStateFlags & Ci.nsIWebProgressListener.STATE_STOP &&
>-        Components.isSuccessCode(aStatus) &&
>-        /^about:/.test(aWebProgress.DOMWindow.document.documentURI)) {
>+        /^about:(?!blank)/.test(document.documentURI) &&
>+        !document.documentElement.hasAttribute("hasBrowserHandlers")) {
>+      // STATE_STOP may be received twice for documents, thus store an
>+      // attribute to ensure handling it just once.
>+      document.documentElement.setAttribute("hasBrowserHandlers", "true");
>       aBrowser.addEventListener("click", BrowserOnClick, true);
>       aBrowser.addEventListener("pagehide", function onPageHide(event) {
>         if (event.target.defaultView.frameElement)
>@@ -4490,7 +4506,7 @@ var TabsProgressListener = {
>       }, true);
> 
>       // We also want to make changes to page UI for unprivileged about pages.
>-      BrowserOnAboutPageLoad(aWebProgress.DOMWindow.document);
>+      BrowserOnAboutPageLoad(document);

The document variable doesn't save anything. In the common non-about: case you wouldn't need to touch it at all.
Err, I misread the patch. The document is accessed for testing the URI, so the variable may or may not be worthwhile depending on how often aStateFlags and aStatus would make you look at the URI.
Assignee

Comment 8

7 years ago
(In reply to Dão Gottwald [:dao] from comment #7)
> Err, I misread the patch. The document is accessed for testing the URI, so
> the variable may or may not be worthwhile depending on how often aStateFlags
> and aStatus would make you look at the URI.

basically for any document we get STATE_STOP here (fwiw note that I'm removing the aStatus check, but that was a minor filter regardless)
Assignee

Comment 9

7 years ago
(In reply to neil@parkwaycc.co.uk from comment #5)
> It wouldn't make sense to cut and paste SeaMonkey's offline error page
> DOMContentLoaded event handler code but it works something like this:

where do you add the listener?
Assignee

Comment 10

7 years ago
Comment on attachment 660816 [details] [diff] [review]
patch v1.0

after some thoughts, I think we should take this or evaluate a completely different approach for the whole thing.  Though the latter is far more risky so I'd more prone to begin with the former, unless someone has negatives to add.
Attachment #660816 - Flags: review?(gavin.sharp)
(In reply to Frank Yan (:fryn) from comment #3)
> I filed that (bug 764989) and made a patch and requested review from you
> months ago to address the first point that Marco repeated above. I had that
> fix included in my fix for bug 682944, but you had asked that I spin it off
> into a separate bug, but then my spun-off patch got ignored.

I apologize if you were frustrated by my lack of attention on bug 764989. I asked Marco to spin this off from his patch bug 749477 in much the same way I asked you to spin the work off, because it's not high-priority bug. That being said, as a member of fx-team, there are multiple options available to you when reviewers aren't responsive that you should take advantage of: pinging people on IRC, picking a different reviewer, etc.
Comment on attachment 660816 [details] [diff] [review]
patch v1.0

You shouldn't use "document" as a variable name in a window scope since that shadows window.document.

I think we should aim for simplicity here - adding the listener too broadly isn't a big deal, as long as we're reasonably confident that the listener only handles events for the right pages. The attribute guard against calling BrowserOnAboutPageLoad twice makes sense.
Attachment #660816 - Flags: review?(gavin.sharp) → review+
Assignee

Comment 13

7 years ago
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #12)
> I think we should aim for simplicity here - adding the listener too broadly
> isn't a big deal, as long as we're reasonably confident that the listener
> only handles events for the right pages.

Ok, I originally added the about:blank check cause in some cases we load about:blank before the actual content, avoiding even the smallest work there may end up in a win on page loading.  I can remove that part though.

I'm retesting the patch now, cause I think the aboutHome browser-chrome test is bogus, but if it passes I'm not going to fix it here, I'll have to fix it in bug 820834 regardless.
Assignee

Comment 14

7 years ago
Posted patch patch v1.1Splinter Review
This is currently running on Try.

Gavin, could you please confirm we don't want the about:blank optimization?
Attachment #660816 - Attachment is obsolete: true
Flags: needinfo?(gavin.sharp)
(In reply to Marco Bonardo [:mak] from comment #14)
> Gavin, could you please confirm we don't want the about:blank optimization?

The patch looks fine to land as-is (I assume that's what you mean by "about:blank optimization").
Flags: needinfo?(gavin.sharp)
https://hg.mozilla.org/mozilla-central/rev/5d4df96a52cd
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.