Last Comment Bug 728983 - Maple invalidation problems with scrollable layer and multiple <div>s
: Maple invalidation problems with scrollable layer and multiple <div>s
Status: VERIFIED FIXED
MAPLE [layout]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: P1 normal (vote)
: mozilla14
Assigned To: Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
:
Mentors:
: 728036 729581 745471 (view as bug list)
Depends on: 753329 765677
Blocks: 729951 735864
  Show dependency treegraph
 
Reported: 2012-02-20 17:17 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-07-18 11:19 PDT (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta+


Attachments
Testcase #1 (3.62 KB, text/html)
2012-03-04 14:50 PST, Mats Palmgren (:mats)
no flags Details
simple testcase (591 bytes, text/html)
2012-04-16 19:50 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
no flags Details
fix (19.45 KB, patch)
2012-04-16 21:35 PDT, Robert O'Callahan (:roc) (Exited; email my personal email if necessary)
matt.woodrow: review+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2012-02-20 17:17:52 PST
This is quite noticeable on http://people.mozilla.org/~jmuizelaar/click-test.html.

The blue boxes don't show up until after scrolling.
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-02-20 17:21:54 PST
My guess is that it was https://hg.mozilla.org/projects/maple/rev/0e7445af29d1 that caused the worseness.
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-02-20 17:28:12 PST
Confirmed.

https://hg.mozilla.org/projects/maple/rev/0e7445af29d1 is bad
https://hg.mozilla.org/projects/maple/rev/cf7c1f3d4c7f is good.
Comment 3 Ali Juma [:ajuma] 2012-02-20 17:59:48 PST
It's the change to nsGfxScrollFrame.cpp in that patch (forcing the creation of a scrollable layer) that's causing this.
Comment 4 Ali Juma [:ajuma] 2012-02-21 07:59:43 PST
The problem is indeed with invalidation when we have scrollable layers. I made a larger click-test page at http://people.mozilla.org/~ajuma/click-test.html, which is large enough to cause a scrollable layer to be created even without the patch applied, and the blue boxes don't get updated until after scrolling even without the patch applied. I'm now looking into what's wrong with invalidation for scrollable layers.
Comment 5 Ali Juma [:ajuma] 2012-02-21 13:36:21 PST
Looking into this further, it seems that Gecko is getting confused about visibility. If we click on the box where the click location div happens to be, the part of the box that overlaps the div turns purple, and when we click away that part turns white again. If we remove the div that displays the click location, everything works fine. If we move this div above the table, then the boxes in the table correctly turn blue when clicked, but the click location div doesn't get updated until a pan or zoom.
Comment 6 Brad Lassey [:blassey] (use needinfo?) 2012-02-23 10:39:06 PST
this was fixed
Comment 7 Ali Juma [:ajuma] 2012-02-23 10:53:27 PST
I'm still seeing this problem.
Comment 8 Ali Juma [:ajuma] 2012-02-29 07:14:10 PST
Interestingly, Attachment 601563 [details] [diff], bug 729534, fixes this problem.
Comment 9 Ali Juma [:ajuma] 2012-02-29 07:32:15 PST
(In reply to Ali Juma [:ajuma] from comment #8)
> Interestingly, Attachment 601563 [details] [diff], bug 729534, fixes this
> problem.

Actually, it fixes the problem of the clicked boxes not turning blue, but it also stops the click location div from updating.
Comment 10 Ali Juma [:ajuma] 2012-03-02 08:01:00 PST
This appears to be caused by the merging of nsDisplayScrollLayers. When we merge two nsDisplayScrollLayers, the frame associated with one of the nsDisplayScrollLayers is used to retain the layer; the problem seems to be that since the other frame is not being used to retain a layer, invalidation requests it receives are essentially having no effect (they just get passed on to the frame's parent).

This explains why the fix to Bug 729354 changes the behaviour we're seeing here -- since that fix changes which frame is used to retain a layer, it changes which frame correctly handles invalidation.

I believe Bug 729581 is caused by the same problem, but I've mostly been debugging this bug since it's easier to reproduce.

There may also be a relationship to the way we're creating new nsDisplayScrollLayers in nsGfxScrollFrameInner::BuildDisplayList on Maple, where we always set mShouldBuildLayer = true (which might explain why we're so frequently having to merge away nsDisplayScrollLayers).
Comment 11 Joe Drew (not getting mail) 2012-03-02 08:04:24 PST
Mats, if you have time today, can you look at this?

Failing that, I'd like Timothy to look at it once he gets up.
Comment 12 Timothy Nikkel (:tnikkel) 2012-03-02 15:54:52 PST
In nsGfxScrollFrame.cpp in ScrollLayerWrapper::WrapItem can we do

-return new (aBuilder) nsDisplayScrollLayer(aBuilder, aItem, aItem->GetUnderlyingFrame(), mScrolledFrame, mScrollFrame);
+return new (aBuilder) nsDisplayScrollLayer(aBuilder, aItem, mScrolledFrame, mScrolledFrame, mScrollFrame);

?
Comment 13 Mats Palmgren (:mats) 2012-03-04 13:00:54 PST
Unfortunately, that breaks z-index ordering.  I tried a few variations of your idea,
but something always breaks.

-> stechz
Comment 14 Timothy Nikkel (:tnikkel) 2012-03-04 14:26:27 PST
Hmm, how does it break z-index ordering? We don't change the order of the items in the display list, do we?
Comment 16 Mats Palmgren (:mats) 2012-03-04 14:50:21 PST
Created attachment 602768 [details]
Testcase #1

Here's the test I was using; the pink block is on top with your suggested fix.
Comment 17 Timothy Nikkel (:tnikkel) 2012-03-04 15:27:43 PST
Can we hack display scroll layer items to have a z-order frame and use that for comparison when we sort?

I'd try this out myself but I don't know where to begin to get a working build for this (do I use the maple branch? what platforms does it work on? etc).
Comment 18 Ali Juma [:ajuma] 2012-03-04 16:04:42 PST
(In reply to Timothy Nikkel (:tn) from comment #17)
> I'd try this out myself but I don't know where to begin to get a working
> build for this (do I use the maple branch? what platforms does it work on?
> etc).

The maple branch on Android is where we've been running into these problems, so it's best to use that. (The build instructions are the same as for Native Fennec -- see https://wiki.mozilla.org/Android)
Comment 19 Timothy Nikkel (:tnikkel) 2012-03-04 16:11:55 PST
(In reply to Ali Juma [:ajuma] from comment #18)
> The maple branch on Android is where we've been running into these problems,
> so it's best to use that. (The build instructions are the same as for Native
> Fennec -- see https://wiki.mozilla.org/Android)

Those make it sound like I need an Android device, which I do not have.
Comment 20 Joe Drew (not getting mail) 2012-03-05 13:32:40 PST
Ben doesn't work on much Mozilla stuff any more, tragically.
Comment 21 Joe Drew (not getting mail) 2012-03-09 12:08:30 PST
We will fix this as follow-up to landing.
Comment 22 Joe Drew (not getting mail) 2012-03-13 10:36:38 PDT
Timothy, Mats, Jet:

Can we get a confirmed owner for this bug? I'm not too concerned who that is, but right now it feels under-owned.
Comment 23 Joe Drew (not getting mail) 2012-03-13 10:37:04 PDT
*** Bug 729581 has been marked as a duplicate of this bug. ***
Comment 24 Joe Drew (not getting mail) 2012-03-13 12:36:07 PDT
*** Bug 728036 has been marked as a duplicate of this bug. ***
Comment 25 Jet Villegas (:jet) 2012-03-13 15:21:29 PDT
Mats: can you pick this one up?
Comment 26 Johnathan Nightingale [:johnath] 2012-03-27 11:56:13 PDT
Poking this - it's a fennec beta blocker and hasn't had activity in 2 weeks. Is there someone who should take this from Mats if he's over-committed?
Comment 27 Jet Villegas (:jet) 2012-03-27 12:37:44 PDT
tn: another scroller for ya...
Comment 28 Timothy Nikkel (:tnikkel) 2012-04-05 16:37:41 PDT
What are the specific steps to reproduce this? I haven't been successful in seeing this.
Comment 29 Ali Juma [:ajuma] 2012-04-05 18:16:50 PDT
(In reply to Timothy Nikkel (:tn) from comment #28)
> What are the specific steps to reproduce this? I haven't been successful in
> seeing this.

1) Visit http://people.mozilla.org/~jmuizelaar/click-test.html
2) Tap on squares.

Result:
The square tapped on will turn blue, but the red rectangle showing the position of the last tap won't update until you pan enough for there to be a repaint. (And, fwiw, before Bug 729534 landed, the problem was reversed: the red rectangle would update, but the square tapped on would not turn blue.)
Comment 30 Joe Drew (not getting mail) 2012-04-16 14:11:36 PDT
*** Bug 745471 has been marked as a duplicate of this bug. ***
Comment 31 Damon Sicore (:damons) 2012-04-16 17:18:05 PDT
Roc, are you on this?
Comment 32 JP Rosevear [:jpr] 2012-04-16 17:28:04 PDT
(In reply to Damon Sicore (:damons) from comment #31)
> Roc, are you on this?

Yes he is, we discussed it in a meeting today.
Comment 33 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-16 19:50:11 PDT
Created attachment 615596 [details]
simple testcase

This testcase reproduces a related problem on desktop. The red box should turn green almost immediately after loading, but you don't see that happen. The reason is exactly what Ali pointed out in comment #10, applied to nsDisplayOpacity; we've merged nsDisplayOpacity items together, so we have a single layer containing the content under #o, but #o's first-in-flow frame isn't marked as a container layer, only the last-in-flow. So when we invalidate #d, the correct ThebesLayer is not invalidated.
Comment 34 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-16 20:23:48 PDT
Interestingly, this testcase works with DLBI. Not sure why though, if the invalidation happens on the merged frame it won't be checked.

For DLBI I think we want merged display items to remember the list of frames that they correspond to. Then we can check each of the frames for the invalidated bit.
Comment 35 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-16 20:54:14 PDT
(In reply to Matt Woodrow (:mattwoodrow) from comment #34)
> For DLBI I think we want merged display items to remember the list of frames
> that they correspond to. Then we can check each of the frames for the
> invalidated bit.

I'm doing the first part to fix this on trunk.
Comment 36 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-16 21:35:11 PDT
Created attachment 615614 [details] [diff] [review]
fix
Comment 37 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-04-16 22:33:51 PDT
Comment on attachment 615614 [details] [diff] [review]
fix

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

Can I be a pain and ask that you land this as two separate pieces? I'd like to be able to backout the FrameLayerBuilder changes so they don't conflict with DLBI.
Comment 38 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-04-16 22:46:35 PDT
Sure.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9157e2b13913
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dbdb799fa6d
Comment 40 Adrian Tamas (:AdrianT) 2012-05-18 02:24:20 PDT
Verified fix on:
Build: Nightly 15.0a1 2012-05-17
Device: HTC Desire Z
OS: Android 2.3.3

The squares are displayed blue when the user taps them at http://people.mozilla.org/~jmuizelaar/click-test.html

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