Last Comment Bug 769541 - After landing Bug 758620, Some CSS causes a problem of re-painting(Tabstrip, Favicon, Label, etc.)
: After landing Bug 758620, Some CSS causes a problem of re-painting(Tabstrip, ...
Status: RESOLVED FIXED
fixed by bug 539356
: regression
Product: Core
Classification: Components
Component: Graphics: Layers (show other bugs)
: 16 Branch
: All All
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Chris Lord [:cwiiis]
:
Mentors:
: 778001 (view as bug list)
Depends on: 772079 786740 802470 820031
Blocks: 758620 771529 776247 777371 793998
  Show dependency treegraph
 
Reported: 2012-06-28 22:37 PDT by Alice0775 White
Modified: 2012-12-10 13:18 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
userChrome.css: Reduced css from "Tree Style tab" (499 bytes, text/css)
2012-06-28 22:37 PDT, Alice0775 White
no flags Details
Fix positioned descendants of fixed-pos frames (6.27 KB, patch)
2012-07-18 11:34 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Fix positioned descendants of fixed-pos frames (6.61 KB, patch)
2012-07-18 12:24 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Fix positioned descendants of fixed-pos frames (6.73 KB, patch)
2012-07-18 12:28 PDT, Chris Lord [:cwiiis]
roc: review+
Details | Diff | Splinter Review
Fix positioned descendants of fixed-pos frames (v2) (6.92 KB, patch)
2012-07-23 08:15 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Fix positioned descendants of fixed-pos frames (v2, rebased) (6.92 KB, patch)
2012-07-23 10:53 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Splinter Review
Fix positioned descendants of fixed-pos frames (v3) (13.07 KB, patch)
2012-07-24 02:16 PDT, Chris Lord [:cwiiis]
roc: review+
Details | Diff | Splinter Review
Fix positioned descendants of fixed-pos frames (final) (13.10 KB, patch)
2012-07-24 17:06 PDT, Chris Lord [:cwiiis]
chrislord.net: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Alice0775 White 2012-06-28 22:37:52 PDT
Created attachment 637788 [details]
userChrome.css: Reduced css from "Tree Style tab"

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/9bf5e71c5746
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120628065213

After landing Bug 758620, Some CSS causes a problem of re-painting.

This problem affects "Tree Style Tab" add-on .

Steps to Reproduce:
1. Prepare a new profile
2. Save attached file as chrome\userChrome.css in the profile
3. Start Firefox with the profile
4. Open several tabs, open web page in existing tab
    --- You can see TabToolbar repaint fails
5. Try to blu rbrowser window  and re-focus
    --- Now, you can see re-painting is carried out

Actual Results:
 Repainting of the TabToolbar is not carried out.

Expected Results:
 Repaint properly
Comment 1 Alice0775 White 2012-06-28 22:39:51 PDT
Err:
s/5. Try to blu rbrowser/5. Try to blur browser/
Comment 2 Alice0775 White 2012-06-28 23:22:34 PDT
In addition to teh comment#0,

You need to enable MenuBar.
Comment 3 Chris Lord [:cwiiis] 2012-06-29 05:01:03 PDT
So this would be a bug in invalidation, I suppose - given that Firefox doesn't use scroll-ports, the layout hasn't changed here, just the display list (and resultant layers).

Can't immediately think what's wrong, but will take a look and ask around.
Comment 4 Chris Lord [:cwiiis] 2012-06-29 05:23:29 PDT
We're going to see if DLBI fixes this.
Comment 5 Chris Lord [:cwiiis] 2012-07-01 04:39:26 PDT
Now that bug 539356 has landed, does this bug still occur?
Comment 6 Alice0775 White 2012-07-01 06:14:50 PDT
I confirmed that this was fixed by bug 539356.

http://hg.mozilla.org/mozilla-central/rev/d9d61d199b11
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120701030537
Comment 7 Chris Lord [:cwiiis] 2012-07-05 03:42:01 PDT
DLBI was backed out and has no fixed schedule for relanding - will take a look at this.
Comment 8 Chris Lord [:cwiiis] 2012-07-05 03:59:05 PDT
It took me a while to figure out how to reproduce this, I don't think the given instructions are clear. Here are some altered steps:

1. Prepare a new profile
2. Save attached file as chrome\userChrome.css in the profile
3. Restart Firefox with the profile
4. Keep opening new tabs until the tab bar starts scrolling
5. Observe that once the tab bar starts scrolling, tabs to the left of the newly opened one aren't invalidated (they appear as if they're still selected)
6. Scrolling the tab bar corrects the drawing, as does blurring the window.
Comment 9 Chris Lord [:cwiiis] 2012-07-05 08:52:14 PDT
I've discovered another invalidation bug that I think may also be the cause of this one;

It seems if a fixed position frame's origin moves, it gets invalidated incorrectly..

This now matters as fixed position elements get their own layers, and so this wouldn't have occurred before (I don't think).

After speaking with ajuma, we think that frame-tree invalidation needs to take into account the origin shifting, or something along these lines.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-05 19:05:40 PDT
You should be hitting this path in nsAbsoluteContainingBlock::ReflowAbsoluteFrame:
  if (oldRect.TopLeft() != rect.TopLeft() || 
      (aDelegatingFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW)) {
    // The frame moved
    aKidFrame->GetParent()->Invalidate(oldOverflowRect);
    aKidFrame->InvalidateFrameSubtree();
Comment 11 Chris Lord [:cwiiis] 2012-07-06 01:16:16 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> You should be hitting this path in
> nsAbsoluteContainingBlock::ReflowAbsoluteFrame:
>   if (oldRect.TopLeft() != rect.TopLeft() || 
>       (aDelegatingFrame->GetStateBits() & NS_FRAME_FIRST_REFLOW)) {
>     // The frame moved
>     aKidFrame->GetParent()->Invalidate(oldOverflowRect);
>     aKidFrame->InvalidateFrameSubtree();

Right - we're hitting this path and checking all the numbers being passed around, everything looks correct... The next thing I need to verify is that these invalidations are actually hitting the right layers.

When you invalidate a frame, does it propagate towards its descendants, or only upwards? If the former, the Invalidate call isn't doing what we want, if the latter, then the InvalidateFrameSubtree call isn't.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-06 01:37:08 PDT
When aKidFrame has its own ContainerLayer, aKidFrame->InvalidateFrameSubtree() should be invalidating all ThebesLayers in the layer subtree rooted at that ContainerLayer. Isn't that what you need here?
Comment 13 Chris Lord [:cwiiis] 2012-07-06 01:43:33 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #12)
> When aKidFrame has its own ContainerLayer,
> aKidFrame->InvalidateFrameSubtree() should be invalidating all ThebesLayers
> in the layer subtree rooted at that ContainerLayer. Isn't that what you need
> here?

It is, but it doesn't appear to be what's happening - instead, only the newly visible area is invalidated.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-06 01:48:59 PDT
Does it show up with GPU compositing enabled? If so, then we're failing to repaint the whole ThebesLayer(s) somehow. If not, then we're probably failing to invalidate the right widget area for BasicLayers.
Comment 15 Chris Lord [:cwiiis] 2012-07-06 01:53:53 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #14)
> Does it show up with GPU compositing enabled? If so, then we're failing to
> repaint the whole ThebesLayer(s) somehow. If not, then we're probably
> failing to invalidate the right widget area for BasicLayers.

I'm testing this on mobile, so this is BasicLayers - I'll see if it's possible to put together a simpler test-case that will expose the problem on desktop.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-06 04:24:26 PDT
OK, so we must be failing to repaint the ThebesLayer(s) for the fixed-pos element. You can verify this with a little logging.
Comment 17 Chris Lord [:cwiiis] 2012-07-06 07:17:15 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #16)
> OK, so we must be failing to repaint the ThebesLayer(s) for the fixed-pos
> element. You can verify this with a little logging.

Stuck in some logging in layers InvalidateRegion - seems for the incorrect updates, the invalidation seems to get zeroed.

My current test is a page that has a very tall div in the background (to make the page scrollable) and a small fixed-pos div aligned at the bottom-right (http://chrislord.net/files/mozilla/fixed2.html). If I zoom out very slightly so that the contentWidth is smaller than the screenWidth and let go so it bounces back, two draws get triggered (one after letting go, the next after bouncing back).

These draws have mostly identical viewports, but a shorter scroll-port clamping size - So the fixed-pos item starts off lower, then moves up slightly when the clamping scroll-port shrinks in height.

Both of these paths take the 'frame moved' branch in nsAbsoluteContainer, as they should. I see these ThebesLayer invalidations on the first reflow:

I/Gecko   (24726): CCC InvalidateRegion bounds: 0, 0, 0x0
I/Gecko   (24726): CCC InvalidateRegion bounds: 0, 1536, 1280x1792
I/Gecko   (24726): CCC InvalidateRegion bounds: 1173, 611, 86x85

and these on the second:

I/Gecko   (24726): CCC InvalidateRegion bounds: 0, 0, 0x0
I/Gecko   (24726): CCC InvalidateRegion bounds: 0, 1536, 1280x1792
I/Gecko   (24726): CCC InvalidateRegion bounds: 0, 0, 0x0

So that last invalidation was almost certainly the invalidation of the fixed-pos layer, and is certainly wrong.

I'm tracing further to find out why.
Comment 18 Chris Lord [:cwiiis] 2012-07-06 07:29:38 PDT
I've put an easier test-case at http://chrislord.net/files/mozilla/fixed3.html - Just changing the 'bottom' style attribute exposes the incorrect invalidation. This test doesn't seem to break on desktop, unfortunately, so this may be a different problem.
Comment 19 Chris Lord [:cwiiis] 2012-07-06 07:49:24 PDT
(In reply to Chris Lord [:cwiiis] from comment #18)
> I've put an easier test-case at
> http://chrislord.net/files/mozilla/fixed3.html - Just changing the 'bottom'
> style attribute exposes the incorrect invalidation. This test doesn't seem
> to break on desktop, unfortunately, so this may be a different problem.

Another note, this doesn't trigger the move or resize block - I guess because the abs container isn't moving or resizing, but the fixed-pos child is the one moving... So I guess this is a problem with the invalidation done in the child's reflow(?)
Comment 20 Chris Lord [:cwiiis] 2012-07-06 08:59:44 PDT
And another note - if I disable the creation of nsDisplayFixedPosition, the thebes layer invalidations are correct (I don't get the 3rd, zero-sized invalidation).

Replacing nsDisplayFixedPosition with nsDisplayOwnLayer has no effect on the problem, as expected (thankfully).
Comment 21 Chris Lord [:cwiiis] 2012-07-06 09:58:44 PDT
Tracing further, in the situation where we don't use nsDisplayFixedPosition, we can't really compare - the invalidations happen 'correctly', as mInvalidateAllThebesContent ends up getting set, so it's actually just invalidating the entire layer.
Comment 22 Chris Lord [:cwiiis] 2012-07-06 11:20:02 PDT
On pressing the button in my test, I see two reflows happening on the absolute containing block that has the fixed-pos element - the first has the current coordinates, and seems to invalidate the area correctly, the second has the new coordinates and does no invalidation (I guess because the first phase did it?)

This seems to me like it ought to produce the correct results, but when it comes to applying the thebes layer invalidation, the region for the frame associated with the nsDisplayFixedPosition is empty... So I guess the BuildLayer implementation is handing the wrong frame to BuildContainerLayerFor(?)
Comment 23 Chris Lord [:cwiiis] 2012-07-06 11:35:13 PDT
Ok, so that last realisation seems to have been the breakthrough moment - Altering nsDisplayFixedPosition's BuildLayer function so that it builds a layer associated with the first principal child of its associated frame fixes the invalidation. Haven't tested to see if it fixes the title bug yet, but I expect it does.

I assume that this isn't the right solution though(?) - any suggestions roc? I'll continue to look at this on Monday.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-08 22:58:49 PDT
(In reply to Chris Lord [:cwiiis] from comment #19)
> Another note, this doesn't trigger the move or resize block - I guess
> because the abs container isn't moving or resizing, but the fixed-pos child
> is the one moving... So I guess this is a problem with the invalidation done
> in the child's reflow(?)

No, when 'bottom' changes nsAbsoluteContainingBlock::ReflowAbsoluteFrame should still be called and oldRect.TopLeft() != rect.TopLeft() should be true.

(In reply to Chris Lord [:cwiiis] from comment #23)
> Ok, so that last realisation seems to have been the breakthrough moment -
> Altering nsDisplayFixedPosition's BuildLayer function so that it builds a
> layer associated with the first principal child of its associated frame
> fixes the invalidation.

That wouldn't be right. The frame we're building the container layer frame seems to be the right one already.

The fixed-pos frame should get aKidFrame->InvalidateFrameSubtree called on it from nsAbsoluteContainingBlock. That calls FrameLayerBuilder::InvalidateThebesLayersInSubtree; InternalInvalidateThebesLayersInSubtree should find (aFrame->GetStateBits() & NS_FRAME_HAS_CONTAINER_LAYER_DESCENDANT) and (aFrame->GetStateBits() & NS_FRAME_HAS_CONTAINER_LAYER). Then we do aFrame->Properties().Delete(ThebesLayerInvalidRegionProperty()). Next time we do a layer transaction, ApplyThebesLayerInvalidation for that frame finds invalidThebesContent is null and does:
    // The region was deleted to indicate that everything should be
    // invalidated.
    aState.SetInvalidateAllThebesContent();
That's what should happen, anyway.
Comment 25 Chris Lord [:cwiiis] 2012-07-10 01:24:21 PDT
After debugging this with roc's help, we discovered this was happening due to bug #772079.

roc suggested checking IsProperAncestorFrame in ScrollLayerWrapper::WrapItem, but this means that fixed position layers end up as siblings of the root scroll layer, instead of children.

Really, this is correct, but all of our async scroll code assumes otherwise (both in native fennec, and xul-fennec), and so this isn't a simple fix for this bug.

This wouldn't be a problem if we set our transforms on the root layer instead of the root scroll layer (and that may solve some other problems too), but this causes odd problems where things start disappearing when you scroll. I've not looked into why this is yet, but it isn't immediately obvious to me.

I'm looking into a fix/work-around for this, but I've marked this as depending on bug #772079 in case we come up with a viable fix for that first.
Comment 26 Chris Lord [:cwiiis] 2012-07-10 11:06:15 PDT
Just to note, the patch in bug 772079 fixes this.
Comment 27 Alice0775 White 2012-07-14 04:06:05 PDT
The patch in bug 772079 landed on Inbound tinderbox build.
http://hg.mozilla.org/integration/mozilla-inbound/rev/a7f80f6408ed
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120714005020

However, This is still reproducible. this is not fixed...
Comment 28 Chris Lord [:cwiiis] 2012-07-14 06:35:30 PDT
(In reply to Alice0775 White from comment #27)
> The patch in bug 772079 landed on Inbound tinderbox build.
> http://hg.mozilla.org/integration/mozilla-inbound/rev/a7f80f6408ed
> Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0
> ID:20120714005020
> 
> However, This is still reproducible. this is not fixed...

ugh, unfortunately this is correct - I just double-checked and it does solve the separate bug I was seeing in Fennec, but I guess either the fix isn't complete or this was caused by another issue.

I'll take a look at this.
Comment 29 Chris Lord [:cwiiis] 2012-07-16 07:41:08 PDT
Just a quick note, the attached userChrome can just set position:fixed on TabsToolbar and do nothing else and the bug will manifest in the way I describe in comment #8. Keeping -moz-appearance makes it easier to look at, but isn't necessary to reproduce the bug.
Comment 30 Chris Lord [:cwiiis] 2012-07-16 08:10:05 PDT
Debugging, but not found anything obvious that fixes this yet - Seeing a lot of empty invalidations, that makes me wonder a bit...
Comment 31 Chris Lord [:cwiiis] 2012-07-16 09:47:31 PDT
More notes;

The invalidations are happening (via InvalidateThebesLayerContents), but somehow they aren't being propagated to the layer until some other change(?) triggers that to happen.

An interesting thing, when the bug is triggered, invalidations on all tabs except the new tab stop working altogether - the calls still happen, just they have no visible effect.

Continuing to debug.
Comment 32 Chris Lord [:cwiiis] 2012-07-16 10:00:04 PDT
More notes; this problem only occurs on active layers.
Comment 33 Chris Lord [:cwiiis] 2012-07-16 10:59:47 PDT
This is the layer tree when invalidations are working and the tabbar is innactive:

BasicLayerManager (0x7f0d72be4160)
  BasicContainerLayer (0x7f0d71ddec00) [visible=< (x=0, y=0, w=805, h=890); >] [opaqueContent] [metrics={ viewport=(x=0, y=0, w=805, h=890) viewportScroll=(x=0, y=0) displayport=(x=0, y=0, w=0, h=0) scrollId=0 }] [usesTmpSurf]
    BasicThebesLayer (0x7f0d71d29800) [visible=< (x=0, y=0, w=805, h=30); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=0, w=805, h=890); >]
    BasicContainerLayer (0x7f0d6e121600) [clip=(x=0, y=30, w=805, h=860)] [visible=< (x=0, y=30, w=805, h=860); >] [opaqueContent] [isFixedPosition]
      BasicThebesLayer (0x7f0d6e15f400) [visible=< (x=0, y=30, w=805, h=860); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=30, w=805, h=860); >]
    BasicContainerLayer (0x7f0d71ddf000) [visible=< (x=0, y=0, w=805, h=67); >] [isFixedPosition]
      BasicThebesLayer (0x7f0d71d2c400) [visible=< (x=0, y=0, w=805, h=67); >] [isFixedPosition] [valid=< (x=0, y=0, w=805, h=67); >]
    BasicThebesLayer (0x7f0d71d2c800) [visible=< (x=0, y=0, w=805, h=28); >] [componentAlpha] [isFixedPosition]


And this is the layer-tree when invalidations are not working and the tabbar is active:

BasicLayerManager (0x7f0d72be4160)
  BasicContainerLayer (0x7f0d71ddec00) [visible=< (x=0, y=0, w=805, h=890); >] [opaqueContent] [metrics={ viewport=(x=0, y=0, w=805, h=890) viewportScroll=(x=0, y=0) displayport=(x=0, y=0, w=0, h=0) scrollId=0 }] [usesTmpSurf]
    BasicThebesLayer (0x7f0d71d29800) [visible=< (x=0, y=0, w=805, h=30); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=0, w=805, h=890); >]
    BasicContainerLayer (0x7f0d6e121600) [clip=(x=0, y=30, w=805, h=860)] [visible=< (x=0, y=30, w=805, h=860); >] [opaqueContent] [isFixedPosition]
      BasicThebesLayer (0x7f0d6e15f400) [visible=< (x=0, y=30, w=805, h=860); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=30, w=805, h=860); >]
    BasicContainerLayer (0x7f0d71ddf000) [visible=< (x=0, y=0, w=805, h=67); >] [isFixedPosition]
      BasicThebesLayer (0x7f0d71d2c400) [visible=< (x=0, y=0, w=805, h=67); >] [isFixedPosition] [valid=< (x=0, y=0, w=805, h=67); >]
    BasicThebesLayer (0x7f0d71d2c800) [visible=< (x=0, y=0, w=805, h=28); >] [isFixedPosition] [valid=< (x=0, y=0, w=805, h=28); >]
    BasicThebesLayer (0x7f0d6d229400) [transform=[ 1 0; 0 1; -618 0; ]] [visible=< (x=641, y=0, w=759, h=28); >] [isFixedPosition] [valid=< (x=641, y=0, w=759, h=28); >]
    BasicThebesLayer (0x7f0d6d229c00) [transform=[ 1 0; 0 1; -618 0; ]] [visible=< (x=641, y=4, w=755, h=21); >] [componentAlpha] [isFixedPosition]


Although all the invalidations seem to be happening, and with the correct numbers, I don't believe in the active layers case that they're going to the correct layers. If I replace every non-empty invalidation with a full-layer invalidation in InvalidatePostTransformRegion, I still get the case where hovering over the tabs produces no visible effect until the layers are marked inactive (whereby everything redraws and looks correct).

I'll continue looking into this tomorrow.
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-16 20:43:04 PDT
In the active case you should be getting InvalidateThebesLayerContents that invalidates for the container layer 0x7f0d71ddf000 when you hover over the tabs.
Comment 35 Chris Lord [:cwiiis] 2012-07-17 05:57:44 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> In the active case you should be getting InvalidateThebesLayerContents that
> invalidates for the container layer 0x7f0d71ddf000 when you hover over the
> tabs.

In both the active and inactive case, the only non-zero invalidations go to layer 0x7f0d71d2c400.

This seems odd to mee. In the active case, the two extra layers are on the root container (where, quite correctly I suppose, there are no invalidations) - I think there are two as the tab-bar wraps with the userChrome I'm using to test it.

Should these two extra layers not have ended up on the 0x7f0d71ddf000 container layer?
Comment 36 Chris Lord [:cwiiis] 2012-07-17 07:02:29 PDT
(In reply to Chris Lord [:cwiiis] from comment #35)
> I think there are two as the tab-bar wraps with the
> userChrome I'm using to test it.

Just to correct myself, the two extra layers are the tab-bar and the menu-bar (which is displayed with a transparent background on top of the tab-bar).

I've stuck up a couple of logs and the userChrome at http://chrislord.net/files/mozilla/769541/
Comment 37 Chris Lord [:cwiiis] 2012-07-17 07:04:53 PDT
The display-list generated with this userChrome doesn't seem right to me - it appears that the tab-bar ends up outside of the FixedPosition display-item, but they really ought to be children to it?
Comment 38 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-17 15:03:51 PDT
Yes, that is odd.
Comment 39 Chris Lord [:cwiiis] 2012-07-17 15:39:50 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> Yes, that is odd.

I'll spend some time tomorrow correlating the frame-tree with the display-list and see if/where it's going wrong.

It'd be great to get a reduced test-case for this, I failed to put one together when I tried on Monday, but I'll give that another go too.
Comment 40 Chris Lord [:cwiiis] 2012-07-18 08:56:55 PDT
http://chrislord.net/files/mozilla/769541/frame-displaylist-layertree.txt - Here's a log (interspersed with a bit of debug output, but ignore that) of the frame-tree, display-list and resultant layer tree.

What seems wrong to me is that a child of the FixedList ends up outside of the FixedPosition item - this accounts for the weird resultant layer tree and problems - the invalidations are going to the layer of the FixedList, as they should, but when animating, layers on the items in the FixedList become active and end up creating layers on top of the layer of the FixedList.
As these active layers are only updated drawn when they're created, that explains why they don't respond to mouse movement.

Now we just need to figure out why a child of the FixedList is creating a display item that's out of tree...
Comment 41 Chris Lord [:cwiiis] 2012-07-18 09:10:04 PDT
The comment,

    // A pseudo-stacking context (e.g., a positioned element with z-index auto).
    // We allow positioned descendants of the child to escape to our parent
    // stacking context's positioned descendant list, because they might be
    // z-index:non-auto

in nsIFrame::BuildDisplayListForChild sounds like it may be the culprit? Is this just an optimisation? Could we just check if child is of type FIXED_POSITION and tell it not to do that? Continuing...
Comment 42 Chris Lord [:cwiiis] 2012-07-18 11:34:07 PDT
Created attachment 643480 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames

It was indeed the behaviour stated in the last comment - solution as discussed with roc on IRC.
Comment 43 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-18 11:46:13 PDT
Comment on attachment 643480 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames

Review of attachment 643480 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsFrame.cpp
@@ +2182,5 @@
> +          aLists.PositionedDescendants()->AppendNewToTop(
> +            new (aBuilder) nsDisplayFixedPosition(aBuilder, item->GetUnderlyingFrame(),
> +                                                  child, &fixedPosDescendantList));
> +        }
> +        item = new (aBuilder) nsDisplayFixedPosition(aBuilder, child, child, &list);

This puts the wrapped extraPositionedDescendents below the main fixed-pos item, instead of above it. That's bad.
Comment 44 Chris Lord [:cwiiis] 2012-07-18 12:24:44 PDT
Created attachment 643500 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames

Whoops, right you are.
Comment 45 Chris Lord [:cwiiis] 2012-07-18 12:28:01 PDT
Created attachment 643507 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames

Sorry, in my haste I let the scope of a variable creep a bit - fixed.
Comment 46 Chris Lord [:cwiiis] 2012-07-18 22:40:01 PDT
Try run looked green, pushed to inbound:

http://hg.mozilla.org/integration/mozilla-inbound/rev/e6068bdf8268

Will request for aurora, as bug 758620 shouldn't really be there without both this and bug 772079.
Comment 48 Chris Lord [:cwiiis] 2012-07-19 06:56:53 PDT
Just opening up the clipping-3.html in a Firefox with this patch makes the fault very obvious - it displays correctly initially, but very quickly afterwards, the red square disappears. Investigating.
Comment 49 Chris Lord [:cwiiis] 2012-07-19 07:01:23 PDT
Actually, to correct myself, it's not 'very quickly afterwards', it's as soon as you trigger another paint. Quite possibly another invalidation bug?
Comment 50 Chris Lord [:cwiiis] 2012-07-19 07:01:50 PDT
(and quite possibly another invalidation bug that I've introduced, to be clear :))
Comment 51 Chris Lord [:cwiiis] 2012-07-23 06:28:01 PDT
Looking at the differences between the frame-tree, display-list and layer tree when this is drawn, the bad draw has two missing layers (a parent and its child). The only difference in anything else between a bad and a good draw is that layers have been retained, the trees are otherwise identical. Also interestingly, the layer that gets lost is a container layer that's a single-child of a container layer.

I wonder if the parent of the lost container layer is being marked as being empty, as it only has another container layer as a child?

Either way, looks like the problem is with retaining layers, so that's where I'll be looking for now.
Comment 52 Chris Lord [:cwiiis] 2012-07-23 07:37:40 PDT
More weirdness, the items that disappear are associated with a layer that isn't in the final tree (I assume it's the layer that would've been the leaf of the missing container). Not figured it out yet, but hopefully getting there.
Comment 53 Chris Lord [:cwiiis] 2012-07-23 07:52:18 PDT
Stopping old layers from being reused in FrameLayerBuilder::BuildContainerLayerFor fixes the error, so that lends further credence that something is happening when retaining layers...
Comment 54 Chris Lord [:cwiiis] 2012-07-23 08:10:52 PDT
So the problem here is that the display-list for the div that disappears is

Clip
FixedPosition
FixedPosition
Background
Border

(the problem clearly being the FixedPosition/FixedPosition) - I guess we could fix this by checking if the item in the extra positioned descendants list is already TYPE_FIXED_POSITION in nsIFrame::BuildDisplayListForChild, or implementing a merge in nsDisplayFixedPosition that merges when the underlying frame is identical - I've gone with the former, as it seems simpler/cleaner/more correct.

Verified that this fixes the problem, updated patch incoming.
Comment 55 Chris Lord [:cwiiis] 2012-07-23 08:15:02 PDT
Created attachment 644934 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (v2)
Comment 56 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2012-07-23 10:17:27 PDT
(In reply to Chris Lord [:cwiiis] from comment #46)
> Try run looked green, pushed to inbound:
> 
> http://hg.mozilla.org/integration/mozilla-inbound/rev/e6068bdf8268
> 
> Will request for aurora, as bug 758620 shouldn't really be there without
> both this and bug 772079.

It seems worth nominating both for tracking as well in that case (with a brief explanation of why that's the case as well), so that they don't get lost.
Comment 57 Chris Lord [:cwiiis] 2012-07-23 10:39:50 PDT
Nominating for tracking, as per comment #56.
Comment 58 Chris Lord [:cwiiis] 2012-07-23 10:50:56 PDT
https://tbpl.mozilla.org/?tree=Try&rev=b9cad627572c - Try run is looking good so far.
Comment 59 Chris Lord [:cwiiis] 2012-07-23 10:53:05 PDT
Created attachment 644978 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (v2, rebased)

Rebase on current m-c.
Comment 60 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-23 16:50:33 PDT
Comment on attachment 644978 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (v2, rebased)

Review of attachment 644978 [details] [diff] [review]:
-----------------------------------------------------------------

I think this wouldn't work when there is a clipping display item between the two fixed-pos display items.

I think this approach isn't going to work when we have nested fixed-pos. But that should be rare. So how about tracking whether we're in a fixed-pos context with a flag in nsDisplayListBuilder, and when we are, refusing to create a nested fixed-pos display item?
Comment 61 Chris Lord [:cwiiis] 2012-07-24 02:16:07 PDT
Created attachment 645229 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (v3)

Do as suggested in comment #60. Verified fixes this bug and the behaviour desired in bug 758620 still works.
Comment 62 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-24 11:16:40 PDT
Comment on attachment 645229 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (v3)

Review of attachment 645229 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that

::: layout/generic/nsFrame.cpp
@@ +2107,5 @@
> +  // nsDisplayFixedPosition. We check if we're already building a fixed-pos
> +  // item and disallow nesting, to prevent the situation of bug #769541
> +  // occurring.
> +  bool isFixedPosition = !aBuilder->IsInFixedPosition() && !isSVG &&
> +    !child->GetParent()->GetParent() && disp->mPosition == NS_STYLE_POSITION_FIXED;

Call this buildFixedPositionItem. Also, put the simplest and most frequently false conditions first. So, I'd say
  disp->mPosition == NS_STYLE_POSITION_FIXED &&
  !child->GetParent()->GetParent() && !isSVG &&
  !aBuilder->IsInFixedPosition()
Comment 63 Chris Lord [:cwiiis] 2012-07-24 17:06:27 PDT
Created attachment 645595 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (final)

Uploading final patch and carrying r+.

Can't push to inbound at the moment, preemptively requesting aurora approval as we probably want to get this in asap and try runs are green.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Fixed position elements can sometimes fail to update correctly.
User impact if declined: Possibly UI and page drawing glitches (mostly manifesting in things either not drawing, or drawing and then failing to update)
Testing completed (on m-c, etc.): try run is green, tested locally - will make sure it lands on central before landing on aurora.
Risk to taking this patch (and alternatives if risky): Not particularly risky, this should only affect the broken situation. Slight risk that I have something wrong, but it's quite a small patch that's gone through several revisions now.
String or UUID changes made by this patch: None.
Comment 64 Chris Lord [:cwiiis] 2012-07-25 00:08:20 PDT
Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/79727970b987
Comment 65 Ed Morley [:emorley] 2012-07-25 08:09:20 PDT
https://hg.mozilla.org/mozilla-central/rev/79727970b987
Comment 66 Alex Keybl [:akeybl] 2012-07-26 16:20:02 PDT
Comment on attachment 645595 [details] [diff] [review]
Fix positioned descendants of fixed-pos frames (final)

[Triage Comment]
Given where we are in the cycle, and the fact that multiple devs have seen these issues in their own work, fast tracking to Aurora 16.
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-07-26 18:19:29 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/d0b7e9bf59fc
Comment 68 Mark Hammond [:markh] 2012-07-30 01:54:09 PDT
*** Bug 778001 has been marked as a duplicate of this bug. ***
Comment 69 YUKI "Piro" Hiroshi 2012-08-03 17:19:36 PDT
This bug seems to be not completely fixed yet, about animated throbber in pinned tabs.

Steps to reproduce:
1. In a new profile, install Tree Style Tab only.
2. Right-click on the vertical tab bar and check "Auto hide tab bar".
3. Go to TST's configuration dialog and choose the skin of tab bar
   "Default (specified by the Theme)".
4. Pin a tab.
5. Focus on a non-pinned tab, then right click on the pinned tab and
   select "Reload Tab".

Actual result: The throbber stops animating.
Expected result: The throbber is animated.

Note: This problem doesn't appear when "Mixed" and other built-in skins.
Comment 70 Chris Lord [:cwiiis] 2012-08-07 06:55:23 PDT
(In reply to SHIMODA Hiroshi from comment #69)
> This bug seems to be not completely fixed yet, about animated throbber in
> pinned tabs.
> 
> Steps to reproduce:
> 1. In a new profile, install Tree Style Tab only.
> 2. Right-click on the vertical tab bar and check "Auto hide tab bar".
> 3. Go to TST's configuration dialog and choose the skin of tab bar
>    "Default (specified by the Theme)".
> 4. Pin a tab.
> 5. Focus on a non-pinned tab, then right click on the pinned tab and
>    select "Reload Tab".
> 
> Actual result: The throbber stops animating.
> Expected result: The throbber is animated.
> 
> Note: This problem doesn't appear when "Mixed" and other built-in skins.

I'll have a look at this - in the meantime though, it'd be super-helpful if you could verify that this is a bug caused by bug 758620 and not something pre-existing (and even more helpful if you could find a simpler way of reproducing, or a simplified test-case).

Note You need to log in before you can comment on or make changes to this bug.