Crash in IsAnyAncestorModified

RESOLVED FIXED in Firefox 58

Status

()

defect
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: marcia, Assigned: mattwoodrow)

Tracking

({crash, topcrash, topcrash-win})

57 Branch
mozilla59
x86
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 fixed, firefox59+ verified)

Details

(crash signature)

Attachments

(1 attachment)

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

=============================================================
Flags: needinfo?(matt.woodrow)
Blocks: 1352499
Flags: needinfo?(matt.woodrow)
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
Flags: needinfo?(matt.woodrow)
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.
This is the #2 Windows topcrash on Nightly 20171124100500, with 204 occurrences, which is pretty high for Nightly.
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.
Assignee: nobody → matt.woodrow
Flags: needinfo?(matt.woodrow)
Attachment #8931994 - Flags: review?(mstange)
Tracking 59+ for this top crash.
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?
Attachment #8931994 - Flags: review?(mstange) → review+
(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.
Pushed by mwoodrow@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/a16f714be5a2
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
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
Attachment #8931994 - Flags: approval-mozilla-beta?
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+.
Attachment #8931994 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
No more crashes after the patch landed.
:gchang given how close this landed to the Beta 8 go-to-build; will this be in Beta 8?
Flags: needinfo?(gchang)
:digitarald, yes. It's in 58.0b8.
Flags: needinfo?(gchang)
You need to log in before you can comment on or make changes to this bug.