878747 - browser.stop() call in addTab() is expensive and causes reflows

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

While profiling gBrowser.addTab() calls b.stop() turned out to be quite expensive and does also cause an uninterruptible reflow:

http://hg.mozilla.org/mozilla-central/file/876d89111acb/browser/base/content/tabbrowser.xml#l1396

I think it would be great if we could just prevent the frame from loading 'about:blank' in the first place if that's not the URL we want. We would then save time loading, stopping and reflowing.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 1 - implement 'noloadsrc' attribute for XUL frames

This patch adds the 'noloadsrc' attribute for XUL frames that prevents LoadSrc() from being called when it's set before the element is injected into the DOM.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 2 - use the new 'noloadsrc' attribute for browsers

This patch uses the new 'noloadsrc' attribute to prevent the newly created <browser> from loading 'about:blank' if that's not the URL we want.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 1 - implement 'nodefaultsrc' attribute for XUL frames

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 2 - use the new 'nodefaultsrc' attribute for browsers

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 757589 [details] [diff] [review]
part 1 - implement 'nodefaultsrc' attribute for XUL frames

if (expr) {
  stmt;
}

and eCaseMatters
(XUL is case-sensitive)

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 757590 [details] [diff] [review]
part 2 - use the new 'nodefaultsrc' attribute for browsers

>+            // If the URI to load is not 'about:blank' then let's prevent the
>+            // initial load of a blank document. We're going to kick off the
>+            // actual page load after we know that no preloaded newtab page
>+            // has been swapped in.
>+            if (!uriIsAboutBlank) {
>+              b.setAttribute("nodefaultsrc", "true");
>+            }

The preload stuff isn't really of interest here. I'd just have the comment say: "Prevent the superfluous initial load of a blank document if we're going to load something other than about:blank."

>+  // TabView causes reflows. Need to find out why.
>+  "iQClass_height@chrome://browser/content/tabview.js|" +
>+    "GroupItem_getContentBounds@chrome://browser/content/tabview.js|" +
>+    "GroupItem_shouldStack@chrome://browser/content/tabview.js|" +
>+    "GroupItem_arrange@chrome://browser/content/tabview.js|" +
>+    "GroupItem_add@chrome://browser/content/tabview.js|" +
>+    "GroupItems_newTab@chrome://browser/content/tabview.js|" +
>+    "TabItem__reconnect@chrome://browser/content/tabview.js|" +
>+    "TabItem@chrome://browser/content/tabview.js|" +
>+    "TabItems_link@chrome://browser/content/tabview.js|" +
>+    "@chrome://browser/content/tabview.js|" +
>+    "addTab@chrome://browser/content/tabbrowser.xml|"

What does this have to do with this bug?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #6)
> The preload stuff isn't really of interest here. I'd just have the comment
> say: "Prevent the superfluous initial load of a blank document if we're
> going to load something other than about:blank."

Ok, will correct.

> What does this have to do with this bug?

Yeah... I forgot to remove that before uploading the patch. It's related to (or actually the reason for) bug 879745. Panorama is active in the background and causes an uninterruptible reflow because iframes can do that :/

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 3 - kill Panorama's iframe before testing tabopen reflows

Instead of landing bug 879745 and losing actual test coverage for Panorama, I think we should just kill the TabView iframe before testing reflows when opening new tabs. That change is a little more self-contained and we can just remove it when Panorama goes away. We can thus test tabopen reflows that the majority of people not using Panorama will see.

FTR, I looked into why Panorama is causing reflows and without refactoring Panorama to not update its UI while it's hidden, there is not much we can do about this, unfortunately.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Attachment: part 3 - kill Panorama's iframe before testing tabopen reflows, v2

We should include the one fix for tabitems.js that removes the "._tabViewTabItem" from tabs.

Comment 10

[Dão Gottwald [:dao]]

Comment on attachment 759028 [details] [diff] [review]
part 3 - kill Panorama's iframe before testing tabopen reflows, v2

I'd rather just add it to the list of known reflows.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #10)
> I'd rather just add it to the list of known reflows.

The problem with adding it to the list of known reflows is that it actually always reflows. So if we're trying to get rid of more reflows in the future we can test that locally but if we run tests on our test servers there will always be the TabView reflow overriding all other achievements :/

Comment 12

[Avi Halachmi (:avih)]

As I understand from Tim, panorams causes reflows on new tab only if panorama was used during the current browser session, and it happens that the panorama test is executed before the reflows test.

Maybe we could just make sure they run in opposite order?

Comment 13

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #11)
> (In reply to Dão Gottwald [:dao] from comment #10)
> > I'd rather just add it to the list of known reflows.
> 
> The problem with adding it to the list of known reflows is that it actually
> always reflows. So if we're trying to get rid of more reflows in the future
> we can test that locally but if we run tests on our test servers there will
> always be the TabView reflow overriding all other achievements :/

Sorry, I don't understand what this means.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

The reflow test does only fail if Panorama has been loaded before. So if the reflow test would be run before the tabview test suite, we'd be fine. That doesn't sound like a great solution, though.

I think I'll go with adding it to the list of known reflows to get this landed. We can figure out how to deal with tabview fallout later.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/fa60aab7fa57
https://hg.mozilla.org/integration/fx-team/rev/73483f4906a7

Pushed including a small test fix for browser_bug678392.js that turned OS X orange.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fa60aab7fa57
https://hg.mozilla.org/mozilla-central/rev/73483f4906a7

Comment 17

[Jeremie Patonnier :Jeremie]

Not sure to understand what needs to be documented here. 
Tetsuharu, could you be a bit more specific?

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

We should document the new 'nodefaultsrc' attribute for <xul:browser> elements.

Comment 19

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Jeremie Patonnier :Jeremie from comment #17)
> Not sure to understand what needs to be documented here. 
> Tetsuharu, could you be a bit more specific?

I think ttaubert's comments too.