Closed Bug 1453541 Opened 2 years ago Closed 2 years ago

Marking the frame's overflow area for rebuild doesn't include things that intersect out-of-flow descendants

Categories

(Core :: Web Painting, defect, P1, critical)

defect

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 + verified
firefox62 + verified

People

(Reporter: mattwoodrow, Assigned: mattwoodrow)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

Attachments

(3 files)

Similar to bug 1436189, but we also need to build items that intersect the OOF frames, not the just the OOF frames themselves.
Blocks: 1453942
Attachment #8967613 - Flags: review?(mikokm)
Attachment #8967614 - Flags: review?(mikokm)
Attachment #8967615 - Flags: review?(mikokm)
Leaving this for now, since bug 1453668 has made it unnecessary. Might re-visit this decision later.
Priority: -- → P5
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Leaving this for now, since bug 1453668 has made it unnecessary. Might
> re-visit this decision later.

I am unsure why I said this. The tests still fail without the patches applied, and this is the cause of bug 1458531.
Severity: normal → critical
Priority: P5 → P1
Duplicate of this bug: 1458531
Attachment #8967613 - Flags: review?(mstange)
Attachment #8967614 - Flags: review?(mstange)
Attachment #8967615 - Flags: review?(mstange)
We'll at least want to make sure that bug 1458531 is verified working again once this lands.
Flags: qe-verify+
Flags: in-testsuite?
Comment on attachment 8967613 [details]
Bug 1453541 - Part 1: Move more code into ProcessFrame so that we can call it from multiple places.

https://reviewboard.mozilla.org/r/236332/#review248644

::: commit-message-29418:1
(Diff revision 1)
> +Bug 1453541 - Part 1: Move more code into ProcessFrame so that we can call it from mulitple places. r?miko

typo: mulitple -> multiple
Attachment #8967613 - Flags: review?(mstange) → review+
Comment on attachment 8967614 [details]
Bug 1453541 - Part 2: Look for Out Of Flow frames with modified ancestors during ProcessFrame instead of during display list building.

https://reviewboard.mozilla.org/r/236334/#review248648

I can't say I 100% understand what's going on here, but the parts I do understand make sense, and we have tests.

::: layout/painting/RetainedDisplayListBuilder.cpp:913
(Diff revision 1)
> +    if (f->ForceDescendIntoIfVisible())
> +      return;
> +    f->SetForceDescendIntoIfVisible(true);

I don't really understand what this does. Why the early return?

::: layout/painting/RetainedDisplayListBuilder.cpp:976
(Diff revision 1)
> +    mBuilder.MarkFrameModifiedDuringBuilding(f);
>  
>      if (!ProcessFrame(f, mBuilder, mBuilder.RootReferenceFrame(),
>                        aOutFramesWithProps, true,
>                        aOutDirty, aOutModifiedAGR)) {
> -      return false;
> +       return false;

I think the old indent was correct
Attachment #8967614 - Flags: review?(mstange) → review+
Comment on attachment 8967615 [details]
Bug 1453541 - Part 3: Add reftests.

https://reviewboard.mozilla.org/r/236336/#review248656
Attachment #8967615 - Flags: review?(mstange) → review+
(In reply to Markus Stange [:mstange] from comment #9)
> Comment on attachment 8967614 [details]
> Bug 1453541 - Part 2: Look for Out Of Flow frames with modified ancestors
> during ProcessFrame instead of during display list building.
> 
> https://reviewboard.mozilla.org/r/236334/#review248648
> 
> I can't say I 100% understand what's going on here, but the parts I do
> understand make sense, and we have tests.

Sorry, I discussed this with miko (offline) when I first wrote it, but it's not explained enough here. I'll add some more comments.

The problem we're solving here is that invalidating a frame that has placeholder descendants gives us an overflow area that doesn't include the OOF frames. Without including their overflow, we don't build content infront/behind the OOFs, and can get incorrect sorting during the merge.

The solution is to walk up the frame tree looking for containing blocks that might have OOF frames with their placeholder as a descendant of the modified frame, and then mark the OOFs as modified if so.

To avoid duplicating work, we check for OOFs with their placeholder inside any invalidated frame (rather than just inside the current frame), and then set a flag on the containing block to note that we've already checked all OOF frames and don't need to do it again.

We also combine this process with the work required for MarkFrameForDisplayIfVisible (which walks up all ancestors, setting a flag, and has an early return if it finds an existing flag - since all further ancestors must have the flag too). We reuse the same flag for both purposes.
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e82d647e8335
Part 1: Move more code into ProcessFrame so that we can call it from multiple places. r=mstange
https://hg.mozilla.org/integration/autoland/rev/02442e2d0ccc
Part 2: Look for Out Of Flow frames with modified ancestors during ProcessFrame instead of during display list building. r=mstange
https://hg.mozilla.org/integration/autoland/rev/b52b2eb81d1e
Part 3: Add reftests. r=mstange
Flags: in-testsuite? → in-testsuite+
Depends on: 1461267
Depends on: 1461281
Comment on attachment 8967613 [details]
Bug 1453541 - Part 1: Move more code into ProcessFrame so that we can call it from multiple places.

Approval Request Comment
[Feature/Bug causing the regression]: Retained-dl
[User impact if declined]: Incorrect z-order sorting sometimes during dynamic changes (affects Netflix - see bug 1458531).
[Is this code covered by automated tests?]: Yes, new reftests added.
[Has the fix been verified in Nightly?]: Manually verified by me.
[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?]: Not overly.
[Why is the change risky/not risky?]: Just adds more frames to the set that we do display list building for. This bug is marked as the regressor for bug 1461281, but we can't reproduce it yet, and it's a much smaller issue than breaking netflix.
[String changes made/needed]: None.
Attachment #8967613 - Flags: approval-mozilla-beta?
Comment on attachment 8967613 [details]
Bug 1453541 - Part 1: Move more code into ProcessFrame so that we can call it from multiple places.

Retained display list fix needed for the feature to ship in Fx61. Approved for 61.0b6.
Attachment #8967613 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Based on comment 7 and comment 17, I have verified that https://bugzilla.mozilla.org/show_bug.cgi?id=1458531#c0 is not reproducible anymore with the fix as follows:

-reproduced on:
     61.0a1  20180421220102
     61.0b5  20180514150347
     
-verified on: win 8.1, ubuntu 16.04, osx 10.13
     62.0a1 20180516220130
Verified as well on 61.0b6 2018-05-17 on Ubuntu 16.04, Win 8 & OSX 10.12.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.