1418722 - Crash in IsAnyAncestorModified

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-7ace78f1-14fd-4cdb-940f-d14680171114.
=============================================================

Seen while looking at nightly crash stats: http://bit.ly/2AThC7J. 11-18 build shows 29 crashes. ni on :mattwoodrow since he worked in this area

Top 10 frames of crashing thread:

0 xul.dll IsAnyAncestorModified layout/painting/RetainedDisplayListBuilder.cpp:71
1 xul.dll MergeFrameRects layout/painting/RetainedDisplayListBuilder.cpp:207
2 xul.dll MergeLayerEventRegions layout/painting/RetainedDisplayListBuilder.cpp:254
3 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:502
4 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:438
5 xul.dll RetainedDisplayListBuilder::MergeDisplayLists layout/painting/RetainedDisplayListBuilder.cpp:468
6 xul.dll RetainedDisplayListBuilder::AttemptPartialUpdate layout/painting/RetainedDisplayListBuilder.cpp:853
7 xul.dll nsLayoutUtils::PaintFrame layout/base/nsLayoutUtils.cpp:3803
8 xul.dll mozilla::PresShell::Paint layout/base/PresShell.cpp:6468
9 xul.dll nsViewManager::ProcessPendingUpdatesPaint view/nsViewManager.cpp:480

=============================================================

Comment 1

[Andrew McCreight [:mccr8]]

This is currently the most common non-shutdownhang crash on Windows Nightly, making up something like 20% of all non-shutdown hang crashes. Matt, is there something you can do in the near term to improve this?

[Tracking Requested - why for this release]: windows top crash

Comment 2

[Andrew McCreight [:mccr8]]

Almost all of these crashes are on the specific value 0xfffffffff0de8042 which seems weird to me. The most common crash URL seems to be various YouTube things, though there are many others. I've actually hit this crash twice on my Windows machine, though reloading the pages I was on didn't retrigger the crash.

Comment 3

[Nicholas Nethercote [inactive]]

This is the #2 Windows topcrash on Nightly 20171124100500, with 204 occurrences, which is pretty high for Nightly.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Make sure we don't reuse nsDisplayPerspective after the inner transform is gone

nsDisplayPerspective uses the outer perspective frame for mFrame, and stores the inner, transformed frame in mTransformFrame.

In the case where the inner frame gets removed (but the outer remains), we don't mark the display item (since it's registered on the outer frame), leaving mTransformFrame pointing to the removed frame.

When we do display list merging, we look for invalidations using FrameForInvalidation() (which is mTransformFrame for perspective), and end up dereferencing the poison value.

On Windows, the poison value is (usually?) 0xf0de7fff, which happens to set nsIFrame::mFrameIsModified to false. We check this, it's false, so we then grab mParent (posion value), add 0x43 (offset to mFrameIsModified) and then crash trying to dereference that (0xf0de8042).

On mac the poison value sets mFrameIsModified to true, so we invalidate the item and get rid of it safely without any issues.

I don't think this is really a security issue, since the memory hasn't really been free'd, just poisoned, and retained for re-use (for another nsIFrame*) within the arena.

The fix tracks the display item on both frames, and makes sure it gets removed if either of them have been deleted.

Comment 5

[Marcia Knous [:marcia]]

Tracking 59+ for this top crash.

Comment 6

[Markus Stange [:mstange]]

Comment on attachment 8931994 [details] [diff] [review]
Make sure we don't reuse nsDisplayPerspective after the inner transform is gone

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

::: layout/painting/nsDisplayList.h
@@ +6249,5 @@
>      mList.GetChildren()->DeleteAll(aBuilder);
>      nsDisplayItem::Destroy(aBuilder);
>    }
>  
> +  virtual bool HasDeletedFrame() const override { return !mTransformFrame || nsDisplayItem::HasDeletedFrame(); }

Do we need to do something similar for nsDisplayBackgroundImage::mDependentFrame?

Comment 7

[Matt Woodrow (:mattwoodrow)]

(In reply to Markus Stange [:mstange] from comment #6)
> Comment on attachment 8931994 [details] [diff] [review]
> Make sure we don't reuse nsDisplayPerspective after the inner transform is
> gone
> 
> Review of attachment 8931994 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/painting/nsDisplayList.h
> @@ +6249,5 @@
> >      mList.GetChildren()->DeleteAll(aBuilder);
> >      nsDisplayItem::Destroy(aBuilder);
> >    }
> >  
> > +  virtual bool HasDeletedFrame() const override { return !mTransformFrame || nsDisplayItem::HasDeletedFrame(); }
> 
> Do we need to do something similar for
> nsDisplayBackgroundImage::mDependentFrame?

Nope, nsIFrame::RemoveDisplayItemDataForDeletion() makes sure that when we deleted a dependent frame, we mark the display item owner frame as invalid too.

We could do that here, but that would trigger rebuilding the outer frame when we delete the inner one, which is more than we need.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a16f714be5a2
Mark nsDisplayPerspective items for removal when the inner transform frame gets removed from the frame tree. r=mstange

Comment 9

[Cristian Brindusan [:cbrindusan]]

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

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8931994 [details] [diff] [review]
Make sure we don't reuse nsDisplayPerspective after the inner transform is gone

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 11

[Gerry Chang [:gchang]]

Comment on attachment 8931994 [details] [diff] [review]
Make sure we don't reuse nsDisplayPerspective after the inner transform is gone

Fix a crash and support retain display lists shield study. Beta58+.

Comment 12

[Calixte Denizet (:calixte)]

No more crashes after the patch landed.

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8e3201300324

Comment 14

[:Harald Kirschner :digitarald]

:gchang given how close this landed to the Beta 8 go-to-build; will this be in Beta 8?

Comment 15

[Gerry Chang [:gchang]]

:digitarald, yes. It's in 58.0b8.