Having opened panorama once in a window affects all rendering in that window, even in tabs that are opened later

VERIFIED FIXED in Firefox 4.0b10

Status

Firefox Graveyard
Panorama
P1
normal
VERIFIED FIXED
8 years ago
2 years ago

People

(Reporter: bz, Assigned: Sean Dunn)

Tracking

({perf})

unspecified
Firefox 4.0b10
x86
All
Dependency tree / graph

Details

(Whiteboard: [hardblocker])

Attachments

(1 attachment, 8 obsolete attachments)

BUILD: any build showing bug 601332

STEPS TO REPRODUCE:
1)  Start the browser
2)  Open tabcandy
3)  Close tabcandy
4)  In any tab (already open or newly opened) in the window you did #2 and #3
    in, load 
    http://www2.webkit.org/perf/sunspider-0.9.1/sunspider-0.9.1/driver.html

EXPECTED RESULTS: Subframe not painting

ACTUAL RESULTS: Subframe painting

Which means tabcandy having been opened sometime in the past is affecting web page behavior.... which is not so great, imo.
blocking2.0: --- → ?
blocking2.0: ? → betaN+
So the issue is that tabcandy is calling drawWindow() on every pageload.  Looking into the details.  This seems bad to me, at first blush.
We seem to be handling a TabAttrModified event.  The event is dispatched by setTabTitleLoading calling _tabAttrModified, in tabbrowser.xml.  And it seems to be triggering off the STATE_START notification that creating the new iframe triggers.

So in particular, tabcandy is disabling our various FOUC protections altogether, as far as I can see....
It's not clear to me, by the way, why changing a tab _title_ requires a drawWindow operation even if tabcandy is open.  But we definitely shouldn't do it when it's closed, no?

Comment 4

8 years ago
is this related to Bug 605928 ?
I don't think so, no.
Thanks for catching this!

So calling drawWindow changes the styling of the page being drawn? Or only at certain times?

Panorama calls drawWindow when the page changes, so our thumbnail is ready for the zoom down animation; otherwise the effect would be quite jarring.

We're looking into using MozAfterPaint instead of TabAttrModified to trigger paints (bug 598435); not sure if that would make this bug better or worse.
Calling drawWindow the way you call it (i.e. without the DRAWWINDOW_DO_NOT_FLUSH flag) flushes out layout.  That means it can (at least temporarily) change the look of the page, and can definitely cause flashes of unstyled content.

> Panorama calls drawWindow when the page change

Uh... not quite.  What panorama does is call drawWindow when any load in any toplevel window or subframe starts when no load is already in flight in that tab, and again when the load finishes.  It also calls it on any tab switch.

> so our thumbnail is ready for the zoom down animation

I really have a hard time believing that synchronous painting of the window and flushing of layout, all on the critical pageload path, is necessary for this.

And of course none of this happens until the user has opened panorama for the first time, so it doesn't show up as affecting our performance tests.  I'd be somewhat interested in seeing what our Tp times look like with this enabled during the Tp run.

What I would recommend is doing the paints async, when the browser is not in the middle of a pageload and not flushing layout as you do so, for a start...  The exact triggering mechanism is probably less important at that point.
Panorama also calls drawWindow any time the page sets document.title or changes title element DOMs or anything...
(In reply to comment #7)
> > so our thumbnail is ready for the zoom down animation
> 
> I really have a hard time believing that synchronous painting of the window and
> flushing of layout, all on the critical pageload path, is necessary for this.

If this is really the only reason we update when Panorama is not active, why don't we use -moz-element just on the large (zoomed in) tab thumbnail which we use for the zoom-out animation? Would that be better than using drawWindow even when we're not in Panorama?
Priority: -- → P1

Updated

8 years ago
Blocks: 598154
(In reply to comment #6)
> Panorama calls drawWindow when the page changes

bz already highlighted this, but TabAttrModified is not a good indicator of "page changes" - it can fire quickly in succession and for changes that have no impact on the preview thumbnail. Sharing infrastructure with the taskbar previews and using MozAfterPaint as suggested in bug 598435 suggest would be ideal (it already uses DO_NOT_FLUSH).

No matter what we do, though, frequently pre-emptively drawWindow()ing all tabs so that we can animate them quickly is not really acceptable. Hopefully moz-element works better for that (I'm not sure how it compares to drawWindow() in terms of performance).

We really need to make sure that opening TabCandy once and then immediately closing it does not introduce large differences in browser behavior or performance.
Assignee: nobody → seanedunn
Blocks: 604213
Keywords: perf
Boris, thank you for the info! We definitely want to be better about this. 

Sean, can you take this on with your perf work?

Mitcho, -moz-element for the zoom down animation is an interesting idea if we can get it faster than what we've seen in bug 597258. 

We already have a mechanism for deferring repaints (so even if we get a lot of TabAttrModified events in a row for a particular page we only repaint the thumbnail once), but we're currently bypassing that for the current page if you're not in the Panorama UI. Seems like that can/should be fixed independently of any of the other issues. Sean, seems like this would be a good use for the update prioritization mechanism we talked about.

We can also easily add DRAWWINDOW_DO_NOT_FLUSH. It looks like it should be: 

- ctx.drawWindow(win, x, y, w, h, color);
+ ctx.drawWindow(win, x, y, w, h, color,
+     Ci.nsIDOMCanvasRenderingContext2D.DRAWWINDOW_DO_NOT_FLUSH);

... though maybe we should cache that flag (WindowsPreviewPerTab.jsm does)?

Obviously we also want to follow up on triggering differently (bug 598435) and possibly using existing thumbnailing (bug 581038).

Sounds like we also want to look into being able to run Tp with Panorama enabled. I'm not sure how to proceed there.

Boris, thanks again for getting this thread rolling.
No problem.  Thanks for jumping on this!
(Assignee)

Comment 13

8 years ago
> Sean, can you take this on with your perf work?

Yup, sure thing.
(Assignee)

Comment 14

8 years ago
Created attachment 491030 [details] [diff] [review]
v1

Adding in DRAWWINDOW_DO_NOT_FLUSH solved the initial problem of not modifying how pages are drawn, but there was still the perf problem up the attrModified handlers (one in GroupItems and one in TabItems) being called and slowing things down.

Solution was to create a _delayedModUpdates to hold unique xultabs when TabView isn't active, and to remove them if the xultab is closed. When TabView is entered, these are flushed to their respective update call.

In the case of TabItems, the update() places this on its own setTimeout chain, so there is no waiting.
Attachment #491030 - Flags: feedback?(ian)
Comment on attachment 491030 [details] [diff] [review]
v1

(In reply to comment #14)
> Solution was to create a _delayedModUpdates to hold unique xultabs when TabView
> isn't active, and to remove them if the xultab is closed. When TabView is
> entered, these are flushed to their respective update call.

This seems like a good idea for the app tabs in GroupItems, but seems unnecessary for TabItems, where we already have the update queue which is paused whenever we exit the Panorama UI and resumed whenever we return to it. 

The one exception is the current tab: we want to update it when we're not in the Panorama UI, so it'll match when we zoom back out to the Panorama UI. However, it would be great if we could do this less aggressively... if we get, say, 15 update requests for the current tab (while not in the Panorama UI) during a minute, it would be nice to just update it once. Any thoughts on how to do that?
Attachment #491030 - Flags: feedback?(ian) → feedback-
(Assignee)

Comment 16

8 years ago
While I agree that in the TabItems case we could just append it to the update queue instead of making its own delay queue, I'm not ready to agree that it should ever update the current tab while not in the TabView, even less agressively. Any noticeable delay in JS performance while TabView is running is bad. During the spider perf test, for example, the constant calling of update() had increased test times by ~25%.

I'd like to try a setTimeout(0) update() of the current xul:tab when TabView is entered. That way, it won't block, and it may copy in time to be shown on the zoomout.
(Assignee)

Comment 17

8 years ago
Using setTimeout(0) causes a blinking hitch. BUT, just calling:
TabItems._update(gBrowser.selectedTab);
is extremely fast, even in debug. So fast, there's no hitching. I'll update the patch asap.
You do know that setTimeout(0) is the same thing as setTimeout(10), right?
(Assignee)

Comment 19

8 years ago
Well, no I didn't realize that the earliest resolution limit was exactly that. Thank you. But, I've only used it to mean "run this asynch, as soon as possible", expecting much worse delays.
(In reply to comment #17)
> Using setTimeout(0) causes a blinking hitch. BUT, just calling:
> TabItems._update(gBrowser.selectedTab);
> is extremely fast, even in debug. So fast, there's no hitching. I'll update the
> patch asap.

That's great to hear! Definitely preferable to updating speculatively.
(Assignee)

Comment 21

8 years ago
Created attachment 491685 [details] [diff] [review]
v2

Kept the GroupItems update delay queue, now using the built-in setTimeout() chain for TabItems. Force an _update() when showTabView() called.
Attachment #491030 - Attachment is obsolete: true
Attachment #491685 - Flags: feedback?(ian)
Comment on attachment 491685 [details] [diff] [review]
v2

Looks good, with a couple of exceptions: 

>+    function handleClose(xulTab) {
>+      let idx = this._delayedModUpdates.indexOf(xulTab);
>+      if (idx != -1)
>+        this._delayedModUpdates.splice(idx, 1);
>+    }
>+
>     AllTabs.register("attrModified", handleAttrModified);
>     this._cleanupFunctions.push(function() {
>       AllTabs.unregister("attrModified", handleAttrModified);
>+      AllTabs.unregister("close", handleClosed);
>     });

Need to register the close handler.

>+    // Update the tab we're looking at, so that the correct image is used 
>+    // to zoom out to the TabView.
>+    TabItems._update(gBrowser.selectedTab);

This _update call will assert if called with an app tab. Check gBrowser.selectedTab.pinned first.

r+ with those items addressed.
Attachment #491685 - Flags: feedback?(ian) → review+
(Assignee)

Comment 23

8 years ago
Created attachment 492903 [details] [diff] [review]
v3

Added changes. Sent to try: http://hg.mozilla.org/try/rev/aaf0f06635b2
Attachment #491685 - Attachment is obsolete: true
Attachment #492903 - Flags: review?(ian)
(Assignee)

Comment 24

8 years ago
Created attachment 493417 [details] [diff] [review]
v4

Try failed first attempt, fixed bug with windows being orphaned. 

When a tab is updated while in browser view (meaning, we want to defer it), and it's the focused tab, and it hasn't been reconnected, the code now reconnects it:

        if (!tab.tabItem.reconnected &&
          !UI._isTabViewVisible() &&
          tab == gBrowser.selectedTab)      
          this.reconnect(tab.tabItem);

Previously, this tab would have NOT been deferred. But, calling _update() on it is too course, and causes the aforementioned slowdowns. The part that matters is the reconnect on focused tabs.
Attachment #492903 - Attachment is obsolete: true
Attachment #493417 - Flags: review?(ian)
Attachment #492903 - Flags: review?(ian)
(Assignee)

Comment 25

8 years ago
Sent to try: http://hg.mozilla.org/try/rev/21d38566d241
(Assignee)

Comment 26

8 years ago
Try failed, but not because of my check-in (full mochitest suite is failing at the moment with the latest pull). Will push to try again later.
(Assignee)

Updated

8 years ago
Blocks: 604699
(Assignee)

Updated

8 years ago
Depends on: 597248
(Assignee)

Comment 27

8 years ago
Created attachment 493698 [details] [diff] [review]
v5
Attachment #493417 - Attachment is obsolete: true
Attachment #493698 - Flags: review?(ian)
Attachment #493417 - Flags: review?(ian)
Comment on attachment 493698 [details] [diff] [review]
v5

>     this._eventListeners["close"] = function(tab) {
>       if (tab.ownerDocument.defaultView != gWindow || tab.pinned)
>         return;
>+      
>+      let idx = self._tabsWaitingForUpdate.indexOf(tab);
>+      if (idx != -1) {
>+        self._tabsWaitingForUpdate.splice(idx, 1);
>+      }
> 
>       self.unlink(tab);
>     }

This splice already happens in .unlink, so we don't need it here. 

r+ with that. 

Please package once you've made that change. Note that the commit message for the patch needs to include "a=blocking" (or whatever's appropriate for the patch).
Attachment #493698 - Flags: review?(ian) → review+
(Assignee)

Comment 29

8 years ago
Created attachment 493857 [details] [diff] [review]
v6

Packaged for check-in
Attachment #493698 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(In reply to comment #26)
> Try failed, but not because of my check-in (full mochitest suite is failing at
> the moment with the latest pull). Will push to try again later. 

Did this ever happen? Was it successful?

Also, the latest patch has a "dump" statement that should be removed.
(Assignee)

Comment 31

8 years ago
Created attachment 494070 [details] [diff] [review]
v7

Unrotted, pushed to try: http://hg.mozilla.org/try/rev/1af83eb82bcb
Attachment #493857 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
No longer depends on: 597248
(Assignee)

Comment 32

8 years ago
Created attachment 494131 [details] [diff] [review]
v8

Unrotted, resubmitted to try: http://hg.mozilla.org/try/rev/b072b88595fb
Attachment #494070 - Attachment is obsolete: true

Comment 33

8 years ago
http://hg.mozilla.org/mozilla-central/rev/b7e28f448533
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b8
Blocks: 591425
Sean, where do we stand on this bug? If nothing else, we should find out if this fixes bug 591425, so we know if someone needs to be looking into that one separately.

Comment 36

7 years ago
bugspam (moving b9 to b10)
Blocks: 608028

Comment 37

7 years ago
bugspam (removing b9)
No longer blocks: 598154
Whiteboard: [hardblocker]

Updated

7 years ago
OS: Mac OS X → All
(Assignee)

Comment 38

7 years ago
Send to try: http://hg.mozilla.org/try/rev/597c55ce70c3
blocker=hardblocker
Severity: normal → blocker

Updated

7 years ago
Severity: blocker → normal
Michael, please see here for how to rate severity.

https://bugzilla.mozilla.org/page.cgi?id=fields.html#bug_severity

Yes, it's confusing that a bug can be a blocker for a release but still not have severity blocker.
(Assignee)

Updated

7 years ago
Blocks: 567029
(Assignee)

Comment 41

7 years ago
Created attachment 504208 [details] [diff] [review]
v9: Unrotted

Sent to try: http://hg.mozilla.org/try/pushloghtml?changeset=77459b91a563
Attachment #494131 - Attachment is obsolete: true

Comment 42

7 years ago
http://hg.mozilla.org/mozilla-central/rev/99bae22d8c8c
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 4.0b8 → Firefox 4.0b10
Duplicate of this bug: 625561
Blocks: 615704
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.