1326194 - Crash in PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-48fd97f5-0c75-4a50-976a-aad812161229.
=============================================================

Seen while looking at Nightly: http://bit.ly/2ihn3Fq. Crashes started with 20161228030213 build.

There are some Mac crashes as well with similar signatures:

[@ PLDHashTable::Remove | mozilla::dom::DOMIntersectionObserver::Disconnect]
[@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation]

Comment 1

[Andrew McCreight [:mccr8]]

I'll just add the two signatures from comment 0 in here, and somebody can split them out into separate bugs if appropriate.

Comment 2

[Nicholas Nethercote [inactive]]

Intersection observer crashes are the #2, #4 and #43 topcrash in Nightly 20161230030205. I've asked for the intersection observer API to be disabled again ASAP in bug 1321865.

Comment 4

[Ting-Yu Chou [:ting] (away)]

Copy the crash signature from bug 1317627.

Comment 5

[Tobias Schneider [:tobytailor]]

What causes the crash here is that we clear the hash table of pointers to IntersectionObservers when unlinking DOMSlots but then later trying to iterate over it in nsNodeUtils::LastRelease to unregister an element from its observing IntersectionObservers. If DOMSlots happens to be unlinked before that, target elements are not properly unregistered, leaving dangling pointers in the observer's mObservingTargets hash table.

Comment 6

[Tobias Schneider [:tobytailor]]

What causes the crash here is that we clear the hash table of pointers to IntersectionObservers when unlinking DOMSlots but then later trying to iterate over it in nsNodeUtils::LastRelease to unregister an element from its observing IntersectionObservers. If DOMSlots happens to be unlinked before that, target elements are not properly unregistered, leaving dangling pointers in the observer's mObservingTargets hash table.

Comment 7

[Tobias Schneider [:tobytailor]]

Attachment: Bug 1326194 - Properly unlink observed targets. m=mrbkap

Review commit: https://reviewboard.mozilla.org/r/102846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102846/

Comment 8

[Tobias Schneider [:tobytailor]]

Btw, I was able to reproduce this by browsing on soundcloud.com for a while.

Comment 9

[Benjamin Smedberg]

This patch doesn't include any tests. Is it not possible to test for this kind of brokenness/crash?

Comment 10

[Tobias Schneider [:tobytailor]]

Unfortunately not, since the order in which things get collected seems to be nondeterministic in this case. See https://bugzilla.mozilla.org/show_bug.cgi?id=1324209#c18.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Tobias Schneider [:tobytailor] from comment #10)
> Unfortunately not, since the order in which things get collected seems to be
> nondeterministic in this case. See
> https://bugzilla.mozilla.org/show_bug.cgi?id=1324209#c18.

Even if you had a test that only sometimes crashed, it would be an improvement over nothing. (Also, you can use SpecialPowers.forceCC to make a CC happen, which might make things slightly more reproduceable.)

Comment 12

[Tobias Schneider [:tobytailor]]

Well, all tests we already have in place could crash cause of this issue. I couldn't reproduce a specific scenario to causes it. The reason why it is somehow reproducible on sites like Soundcloud is simply their excessive usage of IntersectionObservers which increases the odds of crashing.

Comment 13

[Tobias Schneider [:tobytailor]]

Comment on attachment 8824321 [details]
Bug 1326194 - Properly unlink observed targets. m=mrbkap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102846/diff/1-2/

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8824321 [details]
Bug 1326194 - Properly unlink observed targets. m=mrbkap

https://reviewboard.mozilla.org/r/102846/#review103566

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5af3fdc8dccf
Unlink observed targets as soon as DOMSlots gets unlinked. r=mrbkap

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5af3fdc8dccf

Comment 17

[Tobias Schneider [:tobytailor]]

Was able to reproduce this case exactly and create crashtests. Turned out there is another edge case which might lead to the same issue.

Comment 18

[Tobias Schneider [:tobytailor]]

Comment on attachment 8824321 [details]
Bug 1326194 - Properly unlink observed targets. m=mrbkap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102846/diff/2-3/

Comment 19

[Tobias Schneider [:tobytailor]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbe0e5af5cf292a29931a9548d672684d8779c52

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

mozreview seems to have carried over my review from the original patch to the latest one. r=me on both of them.

Comment 21

[Tobias Schneider [:tobytailor]]

Comment on attachment 8824321 [details]
Bug 1326194 - Properly unlink observed targets. m=mrbkap

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102846/diff/3-4/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cdce2c9a89c4
Properly unlink observed targets. m=mrbkap r=mrbkap

Comment 23

[Phil Ringnalda (:philor)]

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

Comment 24

[Marco Castelluccio [:marco]]

I'm copying the flags from the duplicate bug (bug 1317627).