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

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the issue is that tabcandy is calling drawWindow() on every pageload.  Looking into the details.  This seems bad to me, at first blush.

Comment 2

[Boris Zbarsky [:bzbarsky]]

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....

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

[Alice0775 White]

is this related to Bug 605928 ?

Comment 5

[Boris Zbarsky [:bzbarsky]]

I don't think so, no.

Comment 6

[Ian Gilman [:iangilman]]

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.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Panorama also calls drawWindow any time the page sets document.title or changes title element DOMs or anything...

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

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

Comment 10

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

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

Comment 11

[Ian Gilman [:iangilman]]

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.

Comment 12

[Boris Zbarsky [:bzbarsky]]

No problem.  Thanks for jumping on this!

Comment 13

[Sean Dunn]

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

Yup, sure thing.

Comment 14

[Sean Dunn]

Attachment: 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.

Comment 15

[Ian Gilman [:iangilman]]

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?

Comment 16

[Sean Dunn]

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.

Comment 17

[Sean Dunn]

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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

You do know that setTimeout(0) is the same thing as setTimeout(10), right?

Comment 19

[Sean Dunn]

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.

Comment 20

[Ian Gilman [:iangilman]]

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

Comment 21

[Sean Dunn]

Attachment: v2

Kept the GroupItems update delay queue, now using the built-in setTimeout() chain for TabItems. Force an _update() when showTabView() called.

Comment 22

[Ian Gilman [:iangilman]]

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.

Comment 23

[Sean Dunn]

Attachment: v3

Added changes. Sent to try: http://hg.mozilla.org/try/rev/aaf0f06635b2

Comment 24

[Sean Dunn]

Attachment: 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.

Comment 25

[Sean Dunn]

Sent to try: http://hg.mozilla.org/try/rev/21d38566d241

Comment 26

[Sean Dunn]

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.

Comment 27

[Sean Dunn]

Attachment: v5

Comment 28

[Ian Gilman [:iangilman]]

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

Comment 29

[Sean Dunn]

Attachment: v6

Packaged for check-in

Comment 30

[Ian Gilman [:iangilman]]

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

Comment 31

[Sean Dunn]

Attachment: v7

Unrotted, pushed to try: http://hg.mozilla.org/try/rev/1af83eb82bcb

Comment 32

[Sean Dunn]

Attachment: v8

Unrotted, resubmitted to try: http://hg.mozilla.org/try/rev/b072b88595fb

Comment 33

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/b7e28f448533

Comment 34

[(no longer active)]

Backed out because of test failures:

http://hg.mozilla.org/mozilla-central/rev/e2f49a210890

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1291397388.1291398925.13622.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1291399252.1291401945.28056.gz&fulltext=1
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1291398293.1291402027.28513.gz&fulltext=1

etc.

Comment 35

[Ian Gilman [:iangilman]]

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

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 37

[Kevin Hanes]

bugspam (removing b9)

Comment 38

[Sean Dunn]

Send to try: http://hg.mozilla.org/try/rev/597c55ce70c3

Comment 39

[Michael Yoshitaka Erlewine [:mitcho]]

blocker=hardblocker

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 41

[Sean Dunn]

Attachment: v9: Unrotted

Sent to try: http://hg.mozilla.org/try/pushloghtml?changeset=77459b91a563

Comment 42

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/99bae22d8c8c

Comment 44

[Tracy Walker [:tracy]]

verified with nightly build of minefield.