540247 - Faviconize Tab extension causes painting weirdness on session restore

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Timothy Nikkel (:tnikkel)]

1. Install Faviconize Tab extension https://addons.mozilla.org/en-US/firefox/addon/3780 (use nightly tester tools to override compatibility)
2. Open enough tabs so that they don't all fit in the tab bar.
3. Faviconize 3 or so of the first tabs (right click faviconize).
4. Select the last tab.
5. Make sure "Show my tabs from last time" is turned on.
6. Exit Firefox and start it again.

It looks like when the Faviconize Tab extension shrinks the width of those first few tabs, all the tabs shift to the left, but some of the old location of the tabs is not invalidated.

Regression range using nightlies includes bug 526394 which seems like the likely cause.

Comment 1

[Timothy Nikkel (:tnikkel)]

It's enough to just faviconize a tab when there are enough tabs to cause scrolling the in tab bar to see this bug.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Timothy, do you want to dig into this? If not, I'll take it.

Comment 3

[Timothy Nikkel (:tnikkel)]

I can look into it.

Comment 4

[Timothy Nikkel (:tnikkel)]

The problematic invalidate seems to come from nsIFrame::Redraw (in nsBox.cpp). In that call we have |this| as

Box(box)(-1)@0xb0762d00 {0,0,90600,1440} [state=80c01010] [content=0xb0774be0] [overflow=0,0,86760,1560] [sc=0xb0754d98]<

and 90600-86760 = 64 pixels is the space at the end of the tab bar not occupied by any tab (caused by the shrinking of one tab followed by no scrolling of the tabbar that would normally happen if say a tab was removed).

Comment 5

[Timothy Nikkel (:tnikkel)]

nsXULScrollFrame::LayoutScrollArea gets the minsize of its scrolled frame and sets it to that size. Then it calls Layout on the scrolled frame, SprocketLayout handles this and it calls Redraw to invalidate the frame which defaults to using the overflow rect, the overflow rect hasn't been updated from when nsXULScrollFrame::LayoutScrollArea changed the bounds yet. When Layout is finished, EndLayout -> SyncLayout gets called, and this updates the overflow rect and then resizes the view for the frame if it has one (it did before bug 526394) to the overflow rect and this invalidates the difference between the old and new rects of the view. Without a view we don't get this invalidate hence the painting weirdness.

In XUL Layout it seems that it is the parents job to set the size of a child and generate any invalidation needed. So I think we should get the overflow rect of the scrolled frame in nsXULScrollFrame::LayoutScrollArea before and after Layout and invalidate the difference. This fixes the problem for me.

Comment 6

[Timothy Nikkel (:tnikkel)]

Some other observations: the invalidation from nsViewManager::ResizeView goes straight to UpdateView and hence doesn't get passed to any MozAfterPaint listeners.

I can create a reftest for this bug that exhibits the problem on screen, but because the shrinking of the scrolled frame causes a scroll position change to happen we get a bitblit scroll, and the reftest harness updates bitblitted rects using drawWindow (ie a full paint, not by copying them) the problem isn't visible in the canvas the reftest harness is using. If we passed the scroll position change in the MozAfterPaint event with "scroll copy" rects then the reftest harness could emulate the bitblitting.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That's a great idea! Thanks!

Comment 8

[Timothy Nikkel (:tnikkel)]

Attachment: patch

I included the reftest even though it will always pass, maybe if the reftest harness gets updated to bitblit like in comment 6 it will actually test something.

Comment 9

[Timothy Nikkel (:tnikkel)]

Filed bug 542450 for the second paragraph in comment 6.

Comment 10

[Timothy Nikkel (:tnikkel)]

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

Comment 11

[Timothy Nikkel (:tnikkel)]

After I landed this I stumbled upon the fact that bug 543681 seems to fix this without the patch in this bug.

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to comment #11)
> After I landed this I stumbled upon the fact that bug 543681 seems to fix this
> without the patch in this bug.

Scratch that, bug 543681 fixes the simplified testcases that I had, but not the original bug.