Closed Bug 1413073 Opened 5 years ago Closed 5 years ago

Need to be clipped wrt aASR assertion on gmail.


(Core :: Web Painting, defect)

Not set



Tracking Status
firefox58 --- fixed


(Reporter: mattwoodrow, Assigned: mattwoodrow)




(7 files, 1 obsolete file)

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.
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.
Assignee: nobody → matt.woodrow
Attachment #8923653 - Flags: review?(mstange)
Attachment #8923653 - Flags: review?(mstange) → review+
Pushed by
Don't check the dirty rect when deciding if we need to store OutOfFlowDisplayData for a frame. r=mstange
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.
Flags: needinfo?(mstange)
Resolution: FIXED → ---
This is a reduced testcase for the main bug, and the problem encountered on gmail.
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.
Attachment #8926212 - Flags: review?(mstange)
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.
Attachment #8926213 - Flags: review?(mstange)
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.
Attachment #8926214 - Flags: review?(mstange)
Oh, there's still a few failures with that :(

Example failure -

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.
Attachment #8926214 - Attachment is obsolete: true
Attachment #8926214 - Flags: review?(mstange)
Attachment #8926650 - Flags: review?(mstange)
Attachment #8926212 - Flags: review?(mstange) → review+
Attachment #8926213 - Flags: review?(mstange) → review+
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?
Attachment #8926650 - Flags: review?(mstange) → review+
Flags: needinfo?(mstange)
Pushed by
Part 1: Add new crashtests for complex ASR cases. r=mstange
Part 2: Only force frames to have display items built if they intersect the dirty rectangle. r=mstange
Part 3: Recompute ASR and clip-chains on wrap lists when merging. r=mstange
Pushed by
Fix rebase issue that caused us to only update the container asr for reused items.
Attached patch Fix rebase issueSplinter Review
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
Attachment #8929938 - Flags: approval-mozilla-beta?
(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-.
Flags: qe-verify-
Comment on attachment 8929938 [details] [diff] [review]
Fix rebase issue

Take this to support the shield study in 58. Beta58+.
Attachment #8929938 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.