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

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM
--
critical
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: marcia, Assigned: tobytailor)

Tracking

({crash, topcrash})

Trunk
mozilla53
Unspecified
Windows 8
crash, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 wontfix, firefox51 wontfix, firefox52 disabled, firefox53 fixed)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

a year ago
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]
Blocks: 1243846
Blocks: 1321865
status-firefox50: --- → unaffected
status-firefox51: --- → unaffected
status-firefox52: --- → unaffected
status-firefox53: --- → affected
Version: 50 Branch → Trunk
I'll just add the two signatures from comment 0 in here, and somebody can split them out into separate bugs if appropriate.
Crash Signature: [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] → [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] [@ PLDHashTable::Remove | mozilla::dom::DOMIntersectionObserver::Disconnect] [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation]
Flags: needinfo?(tschneider)
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.
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1317627
Copy the crash signature from bug 1317627.
Crash Signature: [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] [@ PLDHashTable::Remove | mozilla::dom::DOMIntersectionObserver::Disconnect] [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] → [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] [@ PLDHashTable::Remove | mozilla::dom::DOMIntersectionObserver::Disconnect] [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] [@ nsICont…
(Assignee)

Comment 5

11 months ago
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.
(Assignee)

Comment 6

11 months ago
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.
Crash Signature: [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] [@ PLDHashTable::Remove | mozilla::dom::DOMIntersectionObserver::Disconnect] [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] [@ nsICont… → [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] [@ PLDHashTable::Remove | mozilla::dom::DOMIntersectionObserver::Disconnect] [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation]
Flags: needinfo?(tschneider)
(Assignee)

Updated

11 months ago
Crash Signature: [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] [@ PLDHashTable::Remove | mozilla::dom::DOMIntersectionObserver::Disconnect] [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] → [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] [@ PLDHashTable::Remove | mozilla::dom::DOMIntersectionObserver::Disconnect] [@ PLDHashTable::Search | mozilla::dom::Element::UpdateIntersectionObservation] @ nsIConte…
Comment hidden (mozreview-request)
(Assignee)

Comment 8

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

Updated

11 months ago
Attachment #8824321 - Flags: review?(mstange) → review?(mrbkap)

Comment 9

11 months ago
This patch doesn't include any tests. Is it not possible to test for this kind of brokenness/crash?
Assignee: nobody → tschneider
Flags: needinfo?(tschneider)
(Assignee)

Comment 10

11 months ago
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.
Flags: needinfo?(tschneider)
(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.)
(Assignee)

Comment 12

11 months ago
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.
(Reporter)

Updated

11 months ago
Keywords: topcrash
Comment hidden (mozreview-request)

Comment 14

11 months ago
mozreview-review
Comment on attachment 8824321 [details]
Bug 1326194 - Properly unlink observed targets. m=mrbkap

https://reviewboard.mozilla.org/r/102846/#review103566
Attachment #8824321 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 15

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5af3fdc8dccf
Unlink observed targets as soon as DOMSlots gets unlinked. r=mrbkap
Keywords: checkin-needed

Comment 16

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5af3fdc8dccf
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Assignee)

Comment 17

11 months ago
Was able to reproduce this case exactly and create crashtests. Turned out there is another edge case which might lead to the same issue.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request)
(Assignee)

Comment 19

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

Comment 20

11 months ago
mozreview seems to have carried over my review from the original patch to the latest one. r=me on both of them.
Comment hidden (mozreview-request)

Comment 22

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cdce2c9a89c4
Properly unlink observed targets. m=mrbkap r=mrbkap

Comment 23

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cdce2c9a89c4
Status: REOPENED → RESOLVED
Last Resolved: 11 months ago11 months ago
Resolution: --- → FIXED
I'm copying the flags from the duplicate bug (bug 1317627).
status-firefox50: unaffected → wontfix
status-firefox51: unaffected → wontfix
status-firefox52: unaffected → fix-optional
status-firefox52: fix-optional → disabled
You need to log in before you can comment on or make changes to this bug.