Last Comment Bug 640048 - Fix edge case for z-ordering for async scrollable elements
: Fix edge case for z-ordering for async scrollable elements
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Benjamin Stover (:stechz)
:
Mentors:
Depends on: 618975 653889 681421
Blocks: 647192
  Show dependency treegraph
 
Reported: 2011-03-08 17:36 PST by Benjamin Stover (:stechz)
Modified: 2011-08-24 06:52 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Fix edge case for z-ordering for async scrollable elements (25.59 KB, patch)
2011-04-27 20:01 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
Fix edge case for z-ordering for async scrollable elements (26.42 KB, patch)
2011-04-28 10:58 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
Fix edge case for z-ordering for async scrollable elements (26.42 KB, patch)
2011-04-28 11:00 PDT, Benjamin Stover (:stechz)
no flags Details | Diff | Splinter Review
Fix edge case for z-ordering for async scrollable elements (27.25 KB, patch)
2011-04-28 20:06 PDT, Benjamin Stover (:stechz)
roc: review+
Details | Diff | Splinter Review
Crash fix: build nsDisplayScrollLayer with a non-null frame (1.18 KB, patch)
2011-04-30 14:55 PDT, Benjamin Stover (:stechz)
roc: review+
Details | Diff | Splinter Review

Description Benjamin Stover (:stechz) 2011-03-08 17:36:22 PST
Bug 618975 introduces a nasty edge case where scrollable HTML elements with content that has z-order can be incorrectly rendered. Instead of using BuildDisplayForStackingContext, we can build many nsDisplayScrollLayers and flatten them out later.
Comment 1 Benjamin Stover (:stechz) 2011-03-08 17:42:26 PST
See https://bugzilla.mozilla.org/show_bug.cgi?id=618975#c43.
Comment 2 Benjamin Stover (:stechz) 2011-04-15 11:41:36 PDT
Roc or Timothy,

I'm not sure how to begin. Can you give me some pointers?
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-17 16:50:07 PDT
a) don't call BuildDisplayListForStackingContext in nsGfxScrollFrame::BuildLayer; instead call BuildDisplayListForChild.
b) Create a wrapper-helper like nsOverflowClipWrapper that wraps everything in nsDisplayScrollLayers.
c) Use that wrapper-helper to wrap everything returned by BuildDisplayListForChild
d) In nsDisplayList::ComputeVisibilityForSublist, make sure nsDisplayScrollLayers get merged together when possible (by overriding TryMerge)
e) if you called TryMerge on an nsDisplayScrollLayer and it didn't merge into the element below it, then check to see whether this is the only nsDisplayScrollLayer for the scrollframe (this will require adding some kind of bookkeeping; you may find it easiest to temporarily set a property on the scrollframe via FrameProperties). If it is NOT, then make it disappear by replacing that item in the elements array with its (flattened) children and resume processing them. Also make sure that all other nsDisplayScrollLayers for the same nsGfxScrollFrame also disappear (more bookkeeping).
Comment 4 Benjamin Stover (:stechz) 2011-04-27 20:01:56 PDT
Created attachment 528779 [details] [diff] [review]
Fix edge case for z-ordering for async scrollable elements
Comment 5 Benjamin Stover (:stechz) 2011-04-27 20:06:43 PDT
Comment on attachment 528779 [details] [diff] [review]
Fix edge case for z-ordering for async scrollable elements

OK, this seems to work.

I added a new display item method called TryFlatten which is called after TryMerge, because this code needs a way to flatten after the regular flattening has been done. Does this sit well with you? Is there a better way?

Anything else about this approach that could be better?
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-27 20:18:29 PDT
Generally, looks great!

Instead of adding your own loop, I thought you could just Flatten the elements of the child list directly onto the end of the 'elements' array and adjust i so that the outer loop does the work for you. We shouldn't need ComputeVisibilityForSubitem.

TryFlatten looks OK but I think we should name it a little better ... perhaps "ShouldFlattenAway"? Need to be carefully documented that it gets called during the ComputeVisibility pass.

I'm confused why nsDisplayScrollInfoLayer::TryFlatten returns GetCount() == 1.

nsCreateLayerWrapper should be ScrollLayerWrapper.setCount should be SetCount.

Why remove the comment "// Furthermore, it is not worth the memory tradeoff to allow asynchronous"?
Comment 7 Benjamin Stover (:stechz) 2011-04-27 20:29:45 PDT
> Instead of adding your own loop, I thought you could just Flatten the elements
> of the child list directly onto the end of the 'elements' array and adjust i so
> that the outer loop does the work for you. We shouldn't need
> ComputeVisibilityForSubitem.

Do you mean flatten the elements of the child list during the loop? Won't that mess up the order of the list? Oh, is every item removed from the array after it is processed? Then I'd just have to adjust i, and that makes sense to me.

> 
> I'm confused why nsDisplayScrollInfoLayer::TryFlatten returns GetCount() == 1.
> 

Hm, I'll have to add a comment. InfoLayer is now always created in nsGfxScrollFrame, but InfoLayer is flattened away if there's exactly one nsDisplayScrollLayer so that there aren't two layers (one empty, one not) with the scroll metadata.

> Why remove the comment "// Furthermore, it is not worth the memory tradeoff to
> allow asynchronous"?

Well, because now we allow asynchronous scrolling of small frames. We no longer check for <= 20 scroll ranges because the memory is only allocated if there is a displayport set.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-27 20:39:47 PDT
(In reply to comment #7)
> > Instead of adding your own loop, I thought you could just Flatten the elements
> > of the child list directly onto the end of the 'elements' array and adjust i so
> > that the outer loop does the work for you. We shouldn't need
> > ComputeVisibilityForSubitem.
> 
> Do you mean flatten the elements of the child list during the loop? Won't that
> mess up the order of the list? Oh, is every item removed from the array after
> it is processed? Then I'd just have to adjust i, and that makes sense to me.

We just don't use the elements at and after index i. So you can call elements.SetLength(i) and then FlattenTo will append the elements starting at index i.

> > I'm confused why nsDisplayScrollInfoLayer::TryFlatten returns GetCount() == 1.
> > 
> 
> Hm, I'll have to add a comment. InfoLayer is now always created in
> nsGfxScrollFrame, but InfoLayer is flattened away if there's exactly one
> nsDisplayScrollLayer so that there aren't two layers (one empty, one not) with
> the scroll metadata.

I see.

> Well, because now we allow asynchronous scrolling of small frames. We no longer
> check for <= 20 scroll ranges because the memory is only allocated if there is
> a displayport set.

I see, thanks.
Comment 9 Benjamin Stover (:stechz) 2011-04-28 10:58:38 PDT
Created attachment 528906 [details] [diff] [review]
Fix edge case for z-ordering for async scrollable elements

Comments above addressed. I think this is ready for review now. Roc, do you
have time? If not, perhaps Timothy could.
Comment 10 Benjamin Stover (:stechz) 2011-04-28 11:00:58 PDT
Created attachment 528907 [details] [diff] [review]
Fix edge case for z-ordering for async scrollable elements
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-28 17:00:46 PDT
Comment on attachment 528907 [details] [diff] [review]
Fix edge case for z-ordering for async scrollable elements

Review of attachment 528907 [details] [diff] [review]:

::: layout/base/nsDisplayList.cpp
@@ +468,5 @@
+    if (list && item->ShouldFlattenAway(aBuilder)) {
+      // The elements on the list >= i no longer serve any use.
+      elements.SetLength(i);
+      list->FlattenTo(&elements);
+      i = elements.Length() - 1;

Shouldn't this be elements.Length()? 'i' is going to be decremented before the next iteration of the loop.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2032,5 @@
+    // flattened during visibility computation, we still need to export the
+    // metadata about this scroll box to the compositor process.
+    nsDisplayScrollInfoLayer* layerItem = new (aBuilder) nsDisplayScrollInfoLayer(
+      aBuilder, mScrolledFrame, mOuter);
+    set.Content()->AppendNewToBottom(layerItem);

Shouldn't we only build this nsDisplayScrollInfoLayer if usingDisplayport?

::: layout/generic/nsIFrame.h
@@ +894,5 @@
   NS_DECLARE_FRAME_PROPERTY(UsedPaddingProperty, DestroyMargin)
   NS_DECLARE_FRAME_PROPERTY(UsedBorderProperty, DestroyMargin)
 
+  typedef PRWord Count;
+  NS_DECLARE_FRAME_PROPERTY(ScrollLayerCount, nsnull)

I don't think we need this typedef. We can just use PRUint32 everywhere.

Also, do we remove the ScrollLayerCount property anywhere? It seems like we should, somewhere, maybe when we delete the last nsDisplayScrollInfoLayer?
Comment 12 Benjamin Stover (:stechz) 2011-04-28 19:20:18 PDT
> Shouldn't this be elements.Length()? 'i' is going to be decremented before the
> next iteration of the loop.

Good catch.

> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +2032,5 @@
> +    // flattened during visibility computation, we still need to export the
> +    // metadata about this scroll box to the compositor process.
> +    nsDisplayScrollInfoLayer* layerItem = new (aBuilder)
> nsDisplayScrollInfoLayer(
> +      aBuilder, mScrolledFrame, mOuter);
> +    set.Content()->AppendNewToBottom(layerItem);
> 
> Shouldn't we only build this nsDisplayScrollInfoLayer if usingDisplayport?

No, because if there is no displayport then there will be no nsDisplayScrollLayers. This is exactly the time where we want an info layer that contains the metadata, so that we know that we should create a displayport. This is equivalent to how the code worked before this patch.

> +  typedef PRWord Count;
> +  NS_DECLARE_FRAME_PROPERTY(ScrollLayerCount, nsnull)
> 
> I don't think we need this typedef. We can just use PRUint32 everywhere.

Hm, I used PRWord because we are storing the count of nsDisplayScrollLayers in the space that would normally be a void* pointer (thus, the re-interpret cast). For 64-bit systems, PRUint32 would then be incorrect, but a downcast should be alright I think.

If this is fine with you, I think PRWord makes more sense but I'm happy to get rid of the typedef.

> Also, do we remove the ScrollLayerCount property anywhere? It seems like we
> should, somewhere, maybe when we delete the last nsDisplayScrollInfoLayer?

Makes sense, done.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-28 19:34:49 PDT
(In reply to comment #12)
> If this is fine with you, I think PRWord makes more sense but I'm happy to get
> rid of the typedef.

OK, I don't mind either way.
Comment 14 Benjamin Stover (:stechz) 2011-04-28 20:06:31 PDT
Created attachment 529026 [details] [diff] [review]
Fix edge case for z-ordering for async scrollable elements

This addresses the above comments. Since the count property is now removed for
info layers, I thought it would be a good idea to abort in debug builds if the
count wasn't defined before visibility computation, since this code depends on
that. This also ensures that info layer items are considered last of all.
Comment 15 Benjamin Stover (:stechz) 2011-04-28 20:07:33 PDT
Also, there was a small refactor of logic in BuildDisplayList for nsGfxScrollFrame.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-28 20:15:01 PDT
Comment on attachment 529026 [details] [diff] [review]
Fix edge case for z-ordering for async scrollable elements

Review of attachment 529026 [details] [diff] [review]:
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-28 20:15:34 PDT
Do you know if there are reftests fixed by this patch? I assume there are, but I'm not sure. We should definitely test it somehow.
Comment 18 Benjamin Stover (:stechz) 2011-04-28 20:21:38 PDT
There is a reftest in the dependency bug, but unfortunately now that reftest somehow needs to set the displayport for the element that is scrolled. I can update the harness so it is capable of that I think.

Can this be done in the dependency bug or does it need to be merged here?

Try run: http://tbpl.mozilla.org/?tree=Try&rev=297975b3fc02
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-28 20:23:29 PDT
(In reply to comment #18)
> There is a reftest in the dependency bug, but unfortunately now that reftest
> somehow needs to set the displayport for the element that is scrolled. I can
> update the harness so it is capable of that I think.
> 
> Can this be done in the dependency bug or does it need to be merged here?

I don't mind.
Comment 20 Benjamin Stover (:stechz) 2011-04-29 08:39:13 PDT
Successful try run here, it seems:
http://tbpl.mozilla.org/?tree=Try&rev=9742de2f1ead

There's a weird Jetpack error with no data (infrastructure issue?) and a few known random oranges, but the "Ru" on Win opt concerned me the most.

Do you know what Ru is and if those failures are random?
Comment 21 Benjamin Stover (:stechz) 2011-04-29 08:45:51 PDT
dholbert thinks Ru is reference test that are using a different configuration, and he had failures on it last night too. So I think this is ready to land. I'm told on #developers the JP failures are not a concern either.

I think this is ready to land! There will be a followup for the reftest in bug 647192.
Comment 22 Benjamin Stover (:stechz) 2011-04-29 10:22:57 PDT
Pushed http://hg.mozilla.org/mozilla-central/rev/c7f62d818af2

Marking as a candidate for tracking-fx5 because it fixes rendering bugs.
Comment 23 Mark Finkle (:mfinkle) (use needinfo?) 2011-04-29 21:09:56 PDT
This patch was causing content crashes. See bug 653889.

I backed it out:
http://hg.mozilla.org/mozilla-central/rev/44f2b2c318b0
Comment 24 Benjamin Stover (:stechz) 2011-04-30 14:55:42 PDT
Created attachment 529316 [details] [diff] [review]
Crash fix: build nsDisplayScrollLayer with a non-null frame

I forgot that I needed an mFrame when wrapping lists.
Comment 25 Benjamin Stover (:stechz) 2011-04-30 15:00:02 PDT
Mark, thanks for backing this out by the way! Sorry for the trouble.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-01 02:38:07 PDT
Comment on attachment 529316 [details] [diff] [review]
Crash fix: build nsDisplayScrollLayer with a non-null frame

Review of attachment 529316 [details] [diff] [review]:
Comment 28 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2011-05-03 14:41:22 PDT
(In reply to comment #22)
> Marking as a candidate for tracking-fx5 because it fixes rendering bugs.

[ in triage meeting ]

How serious are these bugs?
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-03 14:44:26 PDT
Not tracking this - it sounds like what you actually want is to request approval on a rollup patch. If you do that, please include additional justification as to why this is necessary to fix prior to Firefox 6 - "edge case" makes it seem like we don't need to take this immediately.
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-03 15:29:17 PDT
I agree we don't need to take this on a branch.
Comment 31 Dão Gottwald [:dao] 2011-06-12 02:17:09 PDT
bugspam

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