1413073 - Need to be clipped wrt aASR assertion on gmail.

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Getting the assertion "Need to be clipped wrt aASR. Do not call this function with an ASR that our child items don't have finite bounds wrt." from GetClippedBoundsWithRespectToASR.

I can reproduce this fairly reliably by opening a new compose popup, and then selecting the 'To' field.

Looks like we fail to retrieve the OutOfFlowDisplayData for the position:fixed content when doing a partial build, and end up clipping it to the scroll frame.

When we try merge the new content (clipped) and the old content (unclipped), we hit the assertion.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Don't check dirty rect when deciding if we should skip OutOfFlow frames

The current dirty rect isn't overly meaningful right now, since we can override it later with the 'override dirty rect'.

We could check for the presence of that, but this condition is really hard to understand already, and I don't think storing OOF data for a few extra frames (the same ones we'd do in a full build) will hurt.

I don't have a reduced testcase for this unfortunately.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9632977cebf
Don't check the dirty rect when deciding if we need to store OutOfFlowDisplayData for a frame. r=mstange

Comment 3

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/f9632977cebf

Comment 4

[Matt Woodrow (:mattwoodrow)]

Unfortunately this broke some test with retained-dl enabled (that I wrote specifically to catch this sort of thing.

Storing the OutOfFlowDisplayData also sets NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO on all the frames, so we end up building display items for all out-of-flow frames (though not descendants of them), even when trying to do partial builds. Not ideal.

A simple fix is to always set the OutOfFlowDisplayData, but only set NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO when we intersect the dirty rect. This handles the case where we have nested out-of-flow frames and only the inner is dirty.

This doesn't fix gmail though, so I spent some time trying to figure out what's happening there.

It appears that the problem is the full build creates an nsDisplayWrapList that contains both scrolled and fixed content, and thus AutoContainerASRTracker decides that the asr for the wrap list is nullptr (to accommodate the nsDisplayFixedPosition).

We then do a partial build that only rebuilds the scrolled content, and the matching wrap list gets an asr for the scrollframe. We merge the two lists together using the new wrap list as the container, and end up with a final list that has an nsDisplayFixedPosition (with no asr) inside a wrap list with an asr. We hit the assert mentioned initially when we call UpdateBounds to finalize the merge.

We have code that almost fixes that for handling async scrolling. Whenever we do partial building for a frame, we build all items that currently intersect it, as well as re-building frames that have a different async-agr (ASR + async-transform), since those items could intersect it on the compositor, and we need to get ordering correct. Whenever we get changes to more than one async-agr, we give up and do a full build.

This however is restricted to within the scope of the current stacking context (since that is sufficient for the APZ/async animations case), and in the gmail case, the nsDisplayFixedPosition is within a sub-stacking context away from the invalidated frame, and we don't rebuild it.

Possible solutions:

* Remove the stacking context limitation for the different-agr marking. This should make sure the nsDisplayFixedPosition gets built, but it reduces the effectiveness of partial building (makes us build more things, and increases the chance of us failing entirely. I don't have a good feel for exactly how bad this is.

* Stick with the current patch, accept that we always rebuild out-of-flows, and change the tests to not care. Again, reducing the effectiveness of retained-dl. Is it only out-of-flows that can hit this?

* Teach merging about the logic that AutoContainerASRTracker uses, and pick which of the two wrap lists (new or old) to use as the destination for merging. Always picking the outermost one is easy, being smart enough to move to an inner one if all outer items got removed would be better/more complex.

* Find a way to not pick the ASR for a list based on the current contents, so that it's constant even with partial builds. It seems like we could remember what the ASR was at the nearest containing block ancestor, basically pretending that we had fixed position content always. I'm not sure what the implications of this are, how bad do we want the innermost asr possible?

Any ideas Markus? I'm leaning towards the third option right now.

Will attach testcase in a second.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: Testcase for mismatched wrap list ASRs

This is a reduced testcase for the main bug, and the problem encountered on gmail.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Testcase for NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO

This is a testcase for the first problem mentioned, us setting NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO lots.

With the current patch backed out, this fails.

We first visit the outer out-of-flow frame, and decide it's not dirty, so don't store the data.

We then visit then inner out-of-flow frame, decide it is, store the data, and also set NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO on all ancestors (including the outer).

When we do actual building, we now build the outer (since it has NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO), but we don't have data cached. We clip it wrong, and assert about it.

As mentioned above, always storing the data, but only setting NS_FRAME_FORCE_DISPLAY_LIST_DESCEND_INTO if we're actually dirty fixes this bit.

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add new crashtests

Comment 8

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Only mark frames for display if dirty

This fixes the issue with the first crashtest.

We always add the OutOfFlowDisplayData, even when not dirty, since a nested out-of-flow frame might force us to be drawn, and we need to have it around.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Recompute ASR and clip-chains on wrap lists when merging

This fixes the second crashtest (and the initial issue with gmail).

Instead of clearing the clipstate before creating items, we create the items with the full clip, and then do the clear on the item (such that the full clip is cached in the saved state).

When we merge wrap lists, we recompute an appropriate ASR for each list, and then call UpdateBounds which re-clears the clip up to the current asr.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b96c108eec627bb0c41b6271a2b4736c70c9335

Comment 10

[Matt Woodrow (:mattwoodrow)]

Oh, there's still a few failures with that :(

Example failure - https://pastebin.mozilla.org/9072272

The WrapList on line 9 has a scrolled ASR, but isn't clipped (at all, even ignoring ClearUpToASR).

I think not taking the frame's ASR into account is the problem here, since the frame's ASR should be nullptr.

This is the WrapList created for the results of BuildDisplayListForTopLayer, and this last issue might be specific to that (haven't looked into all the failures yet), so it's possible we can just do something special there.

Comment 11

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Recompute ASR and clip-chains on wrap lists when merging v2

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8926650 [details] [diff] [review]
Part 2: Recompute ASR and clip-chains on wrap lists when merging v2

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

::: layout/painting/RetainedDisplayListBuilder.cpp
@@ +419,5 @@
>          // new item. This solves example 2 from above.
>  
>          if (oldItem->GetChildren()) {
>            MOZ_ASSERT(newItem->GetChildren());
> +          const ActiveScrolledRoot* containerASR;

These inner containerASR variables shadow the one in the root scope of this method. That's a bit confusing. Can you rename them to something else?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/08e3bd4e82de
Part 1: Add new crashtests for complex ASR cases. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5a89d7d8163
Part 2: Only force frames to have display items built if they intersect the dirty rectangle. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d9e227c3a5e
Part 3: Recompute ASR and clip-chains on wrap lists when merging. r=mstange

Comment 14

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/08e3bd4e82de
https://hg.mozilla.org/mozilla-central/rev/d5a89d7d8163
https://hg.mozilla.org/mozilla-central/rev/9d9e227c3a5e

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/559f4487dc65
Fix rebase issue that caused us to only update the container asr for reused items.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/559f4487dc65

Comment 17

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix rebase issue

This was a fix to a mistake made during rebasing, but unfortunately it ended up missing the merge to beta.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1352499. This is code that is preffed off, but we want to run a shield study enabling the pref.
[User impact if declined]: None, preffed off code.
[Is this code covered by automated tests?]: Yes, when the pref is enabled.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Code is preffed off.
[String changes made/needed]: None

Comment 18

[Cornel Ionce [:noni]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> [Is this code covered by automated tests?]: Yes, when the pref is enabled.
> [Needs manual test from QE? If yes, steps to reproduce]: No

Due to the fact that this change has automated coverage and manual testing is not required, marking as qe-verify-.

Comment 19

[Gerry Chang [:gchang]]

Comment on attachment 8929938 [details] [diff] [review]
Fix rebase issue

Take this to support the shield study in 58. Beta58+.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/409df5c82bea