stylo: Crash in GetInnerText/IsOrHasAncestorWithDisplayNone with AdBlock Plus enabled

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

(Blocks 1 bug, {crash})

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 unaffected, firefox56 fixed)

Details

Attachments

(3 attachments)

I crashed on this.

https://crash-stats.mozilla.com/report/index/a041abc8-7b37-46ea-a749-52f0d0170719

Fix is straightforward, will work it up.
Actually, the simple failure mode that I thought we were hitting can't happen, so this is more complex. I have an (large) rr trace of it though, so it should be just a matter of time to work through it.
(This seems to only happen with ABP, but the cause might be a deeper issue, we'll see).
Summary: stylo: Crash in GetInnerText/IsOrHasAncestorWithDisplayNone → stylo: Crash in GetInnerText/IsOrHasAncestorWithDisplayNone with AdBlock Plus enabled
This was removed recently in the ComputedValues patch, and makes traversal
logging rather unusable.

MozReview-Commit-ID: 5Jisfj9QxBM
Attachment #8888151 - Flags: review?(cam)
The issue here is that the DestroyFramesFor call does a ClearServoDataFromSubtree,
and then we subsequently fail to load the bindings, leaving ourselves with an unstyled
subtree that isn't marked with descendant bits and isn't rooted at a display:none element,
which is forbidden. This can trigger crashes when we call the innerText getter on one of
the unstyled elements, since that will first flush layout (which won't find the unstyled
subtree), and then invoke IsOrHasAncestorWithDisplayNone, which passes LazyComputeBehavior::Assert.

MozReview-Commit-ID: 7roBkH9fTGZ
Attachment #8888152 - Flags: review?(cam)
MozReview-Commit-ID: IUqzytg3SqL
Attachment #8888153 - Flags: review?(cam)
Attachment #8888151 - Flags: review?(cam) → review+
Comment on attachment 8888152 [details] [diff] [review]
Part 2 - Wait to destroy frames until after we've successfully fetched the binding. v1

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

Looks reasonable.
Attachment #8888152 - Flags: review?(cam) → review+
Attachment #8888153 - Flags: review?(cam) → review+
Pushed by bholley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5a43e98b2000
Wait to destroy frames until after we've successfully fetched the binding. r=heycam
https://hg.mozilla.org/integration/autoland/rev/747035414236
Crashtest. r=heycam
Keywords: crash
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/5a43e98b2000
https://hg.mozilla.org/mozilla-central/rev/747035414236
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.