Closed Bug 878747 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 24

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 4 obsolete files)

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.
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.
Attachment #757443 - Flags: review?(bugs)
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.
Attachment #757445 - Flags: review?(dao)
Attachment #757443 - Flags: review?(bugs)
Attachment #757445 - Attachment is obsolete: true
Attachment #757445 - Flags: review?(dao)
Attachment #757590 - Flags: review?(dao)
Comment on attachment 757589 [details] [diff] [review]
part 1 - implement 'nodefaultsrc' attribute for XUL frames

if (expr) {
  stmt;
}

and eCaseMatters
(XUL is case-sensitive)
Attachment #757589 - Flags: review?(bugs) → review+
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?
Attachment #757590 - Flags: review?(dao) → review+
(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 :/
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.
Attachment #759027 - Flags: review?(dao)
We should include the one fix for tabitems.js that removes the "._tabViewTabItem" from tabs.
Attachment #759027 - Attachment is obsolete: true
Attachment #759027 - Flags: review?(dao)
Attachment #759028 - Flags: review?(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.
Attachment #759028 - Flags: review?(dao) → review-
(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 :/
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?
(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.
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.
Attachment #759028 - Attachment is obsolete: true
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.
https://hg.mozilla.org/mozilla-central/rev/fa60aab7fa57
https://hg.mozilla.org/mozilla-central/rev/73483f4906a7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Blocks: 881244
Blocks: 783064
Not sure to understand what needs to be documented here. 
Tetsuharu, could you be a bit more specific?
Flags: needinfo?(saneyuki.s.snyk)
We should document the new 'nodefaultsrc' attribute for <xul:browser> elements.
(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.
Flags: needinfo?(saneyuki.s.snyk)
Depends on: 908534
You need to log in before you can comment on or make changes to this bug.