1382357 - stylo: Crash in GetInnerText/IsOrHasAncestorWithDisplayNone with AdBlock Plus enabled

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Bobby Holley (:bholley)]

I crashed on this.

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

Fix is straightforward, will work it up.

Comment 1

[Bobby Holley (:bholley)]

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.

Comment 2

[Bobby Holley (:bholley)]

(This seems to only happen with ABP, but the cause might be a deeper issue, we'll see).

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Bring back concise logging for ElementData. v1

This was removed recently in the ComputedValues patch, and makes traversal
logging rather unusable.

MozReview-Commit-ID: 5Jisfj9QxBM

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Wait to destroy frames until after we've successfully fetched the binding. v1

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

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 3 - Crashtest. v1

MozReview-Commit-ID: IUqzytg3SqL

Comment 6

[Cameron McCormack (:heycam)]

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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5a43e98b2000
https://hg.mozilla.org/mozilla-central/rev/747035414236