640048 - Fix edge case for z-ordering for async scrollable elements

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benjamin Stover (:stechz)]

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)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=618975#c43.

Comment 2

[Benjamin Stover (:stechz)]

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)]

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)]

Attachment: Fix edge case for z-ordering for async scrollable elements

Comment 5

[Benjamin Stover (:stechz)]

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)]

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)]

> 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)]

(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)]

Attachment: 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)]

Attachment: Fix edge case for z-ordering for async scrollable elements

Comment 11

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

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)]

> 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)]

(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)]

Attachment: 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)]

Also, there was a small refactor of logic in BuildDisplayList for nsGfxScrollFrame.

Comment 16

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

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)]

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)]

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)]

(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)]

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)]

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)]

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?)]

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)]

Attachment: Crash fix: build nsDisplayScrollLayer with a non-null frame

I forgot that I needed an mFrame when wrapping lists.

Comment 25

[Benjamin Stover (:stechz)]

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)]

Comment on attachment 529316 [details] [diff] [review]
Crash fix: build nsDisplayScrollLayer with a non-null frame

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

Comment 27

[Benjamin Stover (:stechz)]

Pushed again:
http://hg.mozilla.org/mozilla-central/rev/c62fe949af60
http://hg.mozilla.org/mozilla-central/rev/6322e8f7cb2d

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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: [e-mail]]]

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)]

I agree we don't need to take this on a branch.

Comment 31

[Dão Gottwald [:dao]]

bugspam