Closed Bug 950301 Opened 11 years ago Closed 10 years ago

Crash while panning in UI Tests -> Scrollbars

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29
Project Flags:
blocking-b2g 1.3+
Tracking Flags:
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: vingtetun, Assigned: tnikkel)

References

Details

(Keywords: crash, regression, reproducible, Whiteboard: [b2g-crash])

Crash Data

Attachments

(7 files, 2 obsolete files)

Call to RepositionChild with aChild == aAfter
7.87 KB, text/plain
Details
Screenshot of what happens before the crash
37.01 KB, image/png
Details
Fix display list dump code to using one printf per line
3.39 KB, patch
MatsPalmgren_bugz
: review+
kats
: feedback+
kats
: checkin-
Details | Diff | Splinter Review
Log of UI tests before crashing
1.99 MB, application/x-gzip
Details
ownLayer and item
5.44 KB, text/plain
Details
Add an assertion
1.01 KB, patch
BenWa
: review+
kats
: checkin+
Details | Diff | Splinter Review
fix
1.09 KB, patch
mattwoodrow
: review+
kats
: checkin+
Details | Diff | Splinter Review 
Steps to reproduce:
 - Open UI tests
 - Go to the Scroll bar section
 - Pan for a little while the 2 scrollable div
 - You should first see that the rendering seems wrong
 - Now pan a little bit the 1 scrollable div

Expected result:
 - It pans

Actual result:
 - It crash
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 4548.4673]
mozilla::layers::Layer::GetTransform (this=0x46eb4600) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/Layers.cpp:619
619	{
(gdb) bt
#0  mozilla::layers::Layer::GetTransform (this=0x46eb4600)
    at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/Layers.cpp:619
#1  0x40f17638 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=0x46eb4600, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:223
#2  0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#3  0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#4  0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#5  0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#6  0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#7  0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#8  0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#9  0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#10 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#11 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#12 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
---Type <return> to continue, or q <return> to quit---
#13 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#14 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#15 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#16 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#17 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#18 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#19 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#20 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#21 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#22 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#23 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#24 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4, 
    aApzcsToDestroy=0x45a3999c) at /home/vivien/mozilla/code/firefoxos/devices/geekphones/B2G/gecko/gfx/layers/composite/APZCTreeManager.cpp:232
#25 0x40f176e0 in mozilla::layers::APZCTreeManager::UpdatePanZoomControllerTree (this=<value optimized out>, aCompositor=<value optimized out>, 
    aLayer=<value optimized out>, aLayersId=4, aTransform=..., aParent=0x46474800, aNextSibling=0x0, aIsFirstPaint=false, aFirstPaintLayersId=4,
I think it is important to note that the crash does not happen if the second scrollable div has never been scrolled. And the second div have a direction="rtl" which should be the root cause of the issue since I'm not sure how much rtl is supported in the apzc code.
Gaia has a lot of issues with RTL in general. But I'm not sure if we should let a crash that can happens on web content in 1.3 ?
Flags: needinfo?(milan)
We should block on this. Should be an easy fix.
blocking-b2g: --- → 1.3?
Assignee: nobody → chrislord.net
blocking-b2g: 1.3? → 1.3+
Flags: needinfo?(milan)
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #3)
> Gaia has a lot of issues with RTL in general. But I'm not sure if we should
> let a crash that can happens on web content in 1.3 ?

I just tried to removed the RTL flag on the div and the crash still reproduce.
My understanding of UpdatePanZoomControllerTree is pretty limited but from what I see we're saying to a APZC (a) that it is the parent of APZC(b), then in the next run we are saying to APZC(b) that it is the parent of APZC(a).
That should be fine; the tree pointers are cleared and the tree is rebuilt every time UpdatePanZoomControllerTree is run. The APZC instances can be rearranged in the tree without any problem.
I can't reproduce this on a Peak, does this still happen using current m-c, and if so, what device and configuration are you testing on?
Flags: needinfo?(21)
(In reply to Chris Lord [:cwiiis] from comment #8)
> I can't reproduce this on a Peak, does this still happen using current m-c,
> and if so, what device and configuration are you testing on?

It does. It reproduce on a Keon. I can also crash a Peak on the latest m-c. For me it it easy to reproduce once the second scrollable has been scrolled and then ir starts to render funilly. Then scrolling the first scrollable crash the phone.
Flags: needinfo?(21)
Severity: normal → critical
Keywords: crash, regression, reproducible
Whiteboard: [b2g-crash]
Crash Signature: [@ mozilla::layers::Layer::GetTransform]
Do we have an STR outside of the tests?
Assignee: chrislord.net → bgirard
I can't reproduce on nexus 7 or the flatfish with a trunk build after quite a bit of scrolling.
We got it under Kats' Keon. Kats said he would look at it a bit. Kats if you give up I'll borrow a Keon and take it back.

We have a bad layer tree with bad children/sibbling links leading to an ifinite recursion. We're going to add some assertions to check that the layer tree structure is well form to catch the error on the tree mutation instead of when working out the transform which is likely after the error is introduced.
(I have a Peak, not a Keon). But yeah, the specific problem with the layer tree when I hit it was that the mFirstChild and the mLastChild pointed to ends of different linked lists. The mFirstChild list was fine, but the mLastChild list was bad. After following a few mPrevSibling pointers from mLastChild I ended up back at the parent node (i.e. foo->mLastChild->mPrevSibling->mPrevSibling == foo). The APZC code uses the mLastChild pointer to iterate the tree whereas I think most other code uses mFirstChild. This could explain why it manifests in the APZC code but not elsewhere.
That's a good theory
Just eyeballing the code I see that ContainerLayer::RepositionChild will behave badly if aChild == aAfter. I'm not sure if that ever happens but I'll add an assertion for it and try to see if it gets hit.
And that is in fact what is happening.
Attached patch Bandaid patch (obsolete) — — Splinter Review 
This is just a band-aid so we don't crash. In this case the assertion that is failing (and where we insert the same layer twice) is the one at http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=722d9c9944c1#2432 - I'm not sure what the proper fix for that is.
Assignee: bgirard → bugmail.mozilla
Attachment #8357596 - Flags: review?(roc)
Component: Panning and Zooming → Layout
Comment on attachment 8357596 [details] [diff] [review]
Bandaid patch

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

::: layout/base/FrameLayerBuilder.cpp
@@ +2851,5 @@
>      layer = mNewChildLayers[i];
> +    if (prevChild == layer) {
> +      // This should never happen; one of the assertions guarding the insertions to
> +      // mNewChildLayers must have been tripped. But for release builds we need
> +      // to handle this in case it happens.

No we don't. We should actually fix the bug.

Let's land the assertion, definitely, but we should fix this for real. This code is actually harmful since it hides real bugs that the assertion would otherwise catch.
Attachment #8357596 - Flags: review?(roc) → review-
Hmm, was one of the assertions guarding the insertions to mNewChildLayers actually tripped? If so, the stack for that assertion or better still a debugger parked at that assertion would be very helpful!
I can reproduce this pretty easily on my Peak, so I can in fact park gdb at the assertion failure. If you send me a public key I can do that in a screen session and you can poke around. Alternatively if there's something particular I can look for or logging I can add let me know and we can do it that way. I don't know what the SSH latency will be like going all the way around the world and back :p
I have a debug-enabled no-opt build on my Peak and reproduced the assertion failure. The debugger is parked and waiting. Email me a pubkey and I can send you instructions on how to connect.
Blocks: gaia-apzc-2
No longer blocks: gaia-apzc
Flags: needinfo?(roc)
Incidentally, I always see something like this (where one of the scrollable subframes gets flattened onto the page) when I'm trying to reproduce this. I'm not sure if it's related or not. I need to spend some more time panning the other scrollable subframes after this happens before it hits the assertion and crash.
Oops, sorry for the delay here. I'd be happy to connect to a screen session tomorrow morning my time I guess. I'll send you a public key.
Flags: needinfo?(roc)
OK, so the offending layer was used to render an nsDisplayScrollLayer for one frame and then has been chosen again to render an nsDisplayScrollLayer for another frame. That's obviously bad. Then gdb kinda died while I (perhaps unwisely) attempted to evaluate some expressions. The two frames have different elements so it's not immediately obvious why we might have tried to use the same layer for both.

A couple of things that would probably help:
-- Enable MOZ_DUMP_PAINT_LIST=1 and get the display-list dump and layer tree up to the point of the crash.
-- Enabling DEBUG_DISPLAY_ITEM_DATA in FrameLayerBuilder. If that log output doesn't make the device unusable, the data would be helpful.
Attached file Log of UI tests before crashing (obsolete) — 
Here is the log output with MOZ_DUMP_PAINT_LIST=1 and DEBUG_DISPLAY_ITEM_DATA defined. Note that the gzip expands to a 29MB file because I got the whole logcat from starting the UI tests app to the point it crashed. Also unfortunately the logging code doesn't account for the fact that android logging automatically inserts a newline after each logging call, so the indentation in the logging gets broken up across multiple log lines. If you want I can write a script to try to piece it back together.
Flags: needinfo?(roc)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25)
> Created attachment 8359835 [details]
> Log of UI tests before crashing
> 
> Here is the log output with MOZ_DUMP_PAINT_LIST=1 and
> DEBUG_DISPLAY_ITEM_DATA defined. Note that the gzip expands to a 29MB file
> because I got the whole logcat from starting the UI tests app to the point
> it crashed. Also unfortunately the logging code doesn't account for the fact
> that android logging automatically inserts a newline after each logging
> call, so the indentation in the logging gets broken up across multiple log
> lines. If you want I can write a script to try to piece it back together.

Yeah I've seen that before. We should just fix that in the logging code.
Flags: needinfo?(roc)
Kats, could you apply this patch and then rerun the test? Also, at the crash point it would be helpful to know the values of 'item' and 'ownLayer'. Thanks!!!
Assignee: bugmail.mozilla → roc
Attachment #8360058 - Flags: review?(matspal)
Attachment #8360058 - Flags: feedback?(bugmail.mozilla)
Comment on attachment 8360058 [details] [diff] [review]
Fix display list dump code to using one printf per line

r=mats, with a minor nit:  s/BeginReading/get/ because BeginReading
is a nsACString API that I think we want to get rid of.  Also, most
other code use get() so it's more familiar, and get() *isn't*
available on nsACString since that's not guaranteed to be flat.
Attachment #8360058 - Flags: review?(matspal) → review+
I rebased and applied the above patch but am having trouble reproducing the crash now. I'll try rolling back to the version of m-c I was using on the weekend and see if I have better luck there.
Ok, here's the log. The assertion failure is near the end, just search for "ASSERTION". All the output after that is from the root process. It seems much easier to reproduce on the older build; maybe something landed in the last couple of days that fixed it?
Attachment #8359835 - Attachment is obsolete: true
Attached file ownLayer and item — 
These are the values of item and ownLayer that go with the previous log. I don't see these values in the log so gdb might be lying to me. I'll leave gdb parked at the assertion for now, let me know if you need more info. Unfortunately this time I didn't run it inside a screen session.
Attachment #8360058 - Flags: feedback?(bugmail.mozilla) → feedback+
I don't know why the display item doesn't show up in the log. That seems wrong. Are you sure that item value is associated with the right log?

But I do see this:
01-15 15:41:21.909 I/Gecko   (  645):         ScrollLayer 442edef8(Block(div)(561)) bounds(0,2880,21600,34320) visible(0,0,0,0) componentAlpha(0,0,0,0) 
clip(0,2880,21600,34320)  layer=466e3c00
01-15 15:41:21.909 I/Gecko   (  645):           ScrollLayer 442edef8(Block(div)(561)) bounds(1720,22280,12000,12000) visible(0,0,0,0) componentAlpha(0,0
,0,0) clip(1720,22280,12000,12000)  layer=466e3c00

Nested ScrollLayer items for the same frame seems very bad. Do you think you could track that down? Instrumentation or a conditional breakpoint in nsDisplayScrollLayer::nsDisplayScrollLayer should easily detect the case where we're wrapping one nsDisplayScrollWrapper inside another. As far as I can tell, we only construct an nsDisplayScrollLayer from ScrollLayerWrapper in ScrollFrameHelper::BuildDisplayList, which should never do this :-(.
PM Triaged, blocker.
I can reproduce this easily, so if pinging back and forth between kats and roc gets to be too annoying I can look at this.
That would be good, yeah. I've been busy with other work and I don't really know what I should be looking for to figure out what goes wrong here.
(tn is looking into this)
Assignee: roc → tnikkel
Attached patch Add an assertion — — Splinter Review 
This is just the assertion from the bandaid patch that we should keep
Attachment #8357596 - Attachment is obsolete: true
Comment on attachment 8363173 [details] [diff] [review]
Add an assertion

We'll want this patch regardless of what the underlying problem turns out to be.
Attachment #8363173 - Flags: review?(bgirard)
Attachment #8363173 - Flags: review?(bgirard) → review+
So the nested scroll layer items are actually from different scroll frames. For floated elements we wrap each item individually and give it the same frame as the item we are wrapping (whereas when we wrap eg backgrounds we wrap the list as a whole and give the scroll layer item the scrolled frame as its frames). I think this is actually correct. But having two layers creating based on display items of the same type with the same frame confuses frame layer building, I'm guessing.

We actually want the underlying frame to be different for display list construction from the one we use for painting. So maybe we can just switch the frame of scroll layer items after merging and flattening during compute visibility to the scrolled frame.
Attached patch fix — — Splinter Review 
This does indeed fix the problem for me.
Attachment #8365511 - Flags: review?(matt.woodrow)
If all scroll layer items have the scrolled frame as their underlying frame (as far as frame layer builder knows), do we even need to keep track of merged frames for them? I think we can probably simplify that then.
Comment on attachment 8365511 [details] [diff] [review]
fix

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

Changing the underlying frame of a display item seems a bit weird. Normally to solve this we override GetPerFrameKey() for the display item type to make sure it returns a unique identifier, see http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.h#3040
(In reply to Matt Woodrow (:mattwoodrow) from comment #42)
> Comment on attachment 8365511 [details] [diff] [review]
> fix
> 
> Review of attachment 8365511 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Changing the underlying frame of a display item seems a bit weird. Normally
> to solve this we override GetPerFrameKey() for the display item type to make
> sure it returns a unique identifier, see
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> h#3040

I view this more as a hack around the problem that scroll layer items use one random frame contained in the subtree under the scroll frame as an underlying frame. Before we merge scroll layer items, each one might be required to have the underlying frame of it's enclosing item so that we get the painting order correct. But once we merge all these items into one it doesn't make sense to use the underlying frame of the last merged item, we change the underlying frame of scroll layer items every time we merge them (which is the common case). Since the scroll layer is for all of the content contained by the scroll frame the most logical frame to use is the scrolled frame.
Comment on attachment 8365511 [details] [diff] [review]
fix

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

Ok, I'm convinced.
Attachment #8365511 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8360058 [details] [diff] [review]
Fix display list dump code to using one printf per line

Bug 962505 does what this patch does and more and has landed, so we don't need this anymore.
https://hg.mozilla.org/mozilla-central/rev/c7d862c7116d
https://hg.mozilla.org/mozilla-central/rev/f785310ab034
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: