790934 - Don't run BrowserOnAboutPageLoad twice for each about page load

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I had forgotten that Frank filed bug 764989 already - I guess we can dupe that here?

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Oh, actually I guess that is a different (related) bug.

Comment 3

[Frank Yan (:fryn)]

(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 [e-mail] 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.

Comment 5

[[e-mail]]

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 6

[Dão Gottwald [:dao]]

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.

Comment 7

[Dão Gottwald [:dao]]

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.

Comment 8

[Marco Bonardo [:mak]]

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

Comment 9

[Marco Bonardo [:mak]]

(In reply to [e-mail] 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?

Comment 10

[Marco Bonardo [:mak]]

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.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

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

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 13

[Marco Bonardo [:mak]]

(In reply to :Gavin Sharp (use [e-mail] 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.

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch v1.1

This is currently running on Try.

Gavin, could you please confirm we don't want the about:blank optimization?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

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

Comment 16

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d4df96a52cd

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5d4df96a52cd