Crash in InvalidArrayIndex_CRASH | nsDocument::NotifyIntersectionObservers

RESOLVED FIXED in Firefox 53

Status

()

defect
P1
critical
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: alice0775, Assigned: tschneider)

Tracking

(4 keywords)

53 Branch
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox50 unaffected, firefox51 unaffected, firefox52 unaffected, firefox53+ fixed)

Details

(Whiteboard: [DUPEME], crash signature)

Attachments

(1 attachment, 3 obsolete attachments)

Reporter

Description

3 years ago
str
[Tracking Requested - why for this release]: Reproducible crash

This bug was filed from the Socorro interface and is 
report bp-9e669c44-c43d-41c7-a443-a1f482161217.
=============================================================

Reproducible: always

Steps to reproduce:
1. Open http://money.cnn.com/video/media/2016/10/13/rogue-one-trailer-good-morning-america.cnnmoney?iid=EL
2. Wait for a minute so that video will play back automatically.

3. Open http://money.cnn.com/video/media/2016/10/13/rogue-one-trailer-good-morning-america.cnnmoney?iid=EL in the same tab
4. Wait for a minute so that video will play back automatically.

5. Repeat from step1 if necessary

Actual Results:
Tab crashes
Reporter

Comment 1

3 years ago
oops typo.

3. Open http://money.cnn.com/2016/11/25/media/rogue-one-tickets/ in the same tab

Updated

3 years ago
Flags: needinfo?(tschneider)
Duplicate of this bug: 1324203
Another STR:
Go to this page, scroll up and down: http://abc7news.com/pets/hillsborough-mountain-lion-goes-viral/1658809/
Keywords: topcrash
I got a different crash one time with my STR, so I'll assume it is a similar issue.
Crash Signature: [@ InvalidArrayIndex_CRASH | nsDocument::NotifyIntersectionObservers] → [@ InvalidArrayIndex_CRASH | nsDocument::NotifyIntersectionObservers] [@ nsTHashtable<T>::Contains | mozilla::dom::DOMIntersectionObserver::UnlinkTarget ]

Comment 6

3 years ago
backoutbugherder
https://hg.mozilla.org/mozilla-central/rev/1016d96bdabb
https://hg.mozilla.org/mozilla-central/rev/f22c6dc53276
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Guess I formatted the commit message on that backout incorrectly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee

Comment 8

3 years ago
Clear mRegisteredIntersectionObservers properly when unlinking DOMSlots.
Flags: needinfo?(tschneider)
Assignee

Updated

3 years ago
Keywords: checkin-needed
Assignee

Updated

3 years ago
Keywords: checkin-needed
Assignee

Comment 9

3 years ago
Comment on attachment 8819693 [details] [diff] [review]
Clear mRegisteredIntersectionObservers when unlinking

Markus, this is a simple fix but I guess I still need an r+ from someone.
Attachment #8819693 - Flags: review?(mstange)
Priority: -- → P1
Assignee: nobody → tschneider
How is the crash happening, and how does the patch fix the bug?
Crash Signature: [@ InvalidArrayIndex_CRASH | nsDocument::NotifyIntersectionObservers] [@ nsTHashtable<T>::Contains | mozilla::dom::DOMIntersectionObserver::UnlinkTarget ] → [@ InvalidArrayIndex_CRASH | nsDocument::NotifyIntersectionObservers] [@ nsTHashtable<T>::Contains | mozilla::dom::DOMIntersectionObserver::UnlinkTarget ] [@ @0x0 | nsTHashtable<T>::Contains | mozilla::dom::DOMIntersectionObserver::UnlinkTarget] [@ xu…
Adding the Linux signature and changing to all since it affected all platforms.
Crash Signature: xul.dll@0x2562bd8 | nsTHashtable<T>::Contains | mozilla::dom::DOMIntersectionObserver::UnlinkTarget ] [@ @0x0 | mozilla::dom::DOMIntersectionObserver::UnlinkTarget] → xul.dll@0x2562bd8 | nsTHashtable<T>::Contains | mozilla::dom::DOMIntersectionObserver::UnlinkTarget ] [@ @0x0 | mozilla::dom::DOMIntersectionObserver::UnlinkTarget] [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | nsDocument::NotifyIntersecti…
OS: Windows 10 → All
Hardware: x86 → All
Tracking 53+ for this topcrash regression.
(In reply to Markus Stange [:mstange] from comment #10)
> How is the crash happening, and how does the patch fix the bug?

In this particular case (and seams in general whenever a new website is loaded and all objects of an old tab get collected at once) the order our unlink code for IntersectionObserver A observing a target B gets executed is:

1. unlink code for DOMSlots of B (holding a hash table with a pointer to A)
2. unlink code of A
3. unlink code of B

If we don't clear the hash table in DOMSlots, the unlink code in step 3 accesses a pointer into freed memory of A leading to the crash.
Comment on attachment 8819693 [details] [diff] [review]
Clear mRegisteredIntersectionObservers when unlinking

Ok, the patch makes sense to fix the Unlink crash.

As per our conversation on IRC, I'm not confident that this patch also fixes the NotifyIntersectionObservers crash. This might be the real bug that caused the NotifyIntersectionObservers, or there might be another lingering bug that just doesn't trigger any more with this patch. I think it would be a good idea to try to get to the bottom of the NotifyIntersectionObservers crash.
Attachment #8819693 - Flags: review?(mstange) → review+
Better commit message.
Attachment #8819693 - Attachment is obsolete: true

Comment 17

3 years ago
mozreview-review
Comment on attachment 8820459 [details]
Bug 1324209 - Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers.

https://reviewboard.mozilla.org/r/99960/#review100614
Attachment #8820459 - Flags: review?(mstange) → review+
(In reply to Tobias Schneider [:tobytailor] from comment #13)
> In this particular case (and seams in general whenever a new website is
> loaded and all objects of an old tab get collected at once) the order our
> unlink code for IntersectionObserver A observing a target B gets executed is:

For what it is worth, there's no guarantee about the order that Unlink methods will get called, so the code should be able to handle any ordering.
Assignee

Updated

3 years ago
Keywords: checkin-needed

Comment 19

3 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38ddff02f4db
Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers. r=mstange
Keywords: checkin-needed
Assignee

Updated

3 years ago
Crash Signature: [@ InvalidArrayIndex_CRASH | nsDocument::NotifyIntersectionObservers] [@ nsTHashtable<T>::Contains | mozilla::dom::DOMIntersectionObserver::UnlinkTarget ] [@ @0x0 | nsTHashtable<T>::Contains | mozilla::dom::DOMIntersectionObserver::UnlinkTarget] [@ xu… → [@ InvalidArrayIndex_CRASH | nsDocument::NotifyIntersectionObservers] [@ InvalidArrayIndex_CRASH | nsTArray_Impl<T>::ElementAt | nsDocument::NotifyIntersectionObservers ]
Assignee

Updated

3 years ago
Attachment #8820448 - Attachment is obsolete: true
The crashes in NotifyIntersectionObservers and UnlinkTarget are unrelated. Changed this bug to fix the NotifyIntersectionObservers and created bug 1325103 for UnlinkTarget.
Backed out for failing own crashtest:

https://hg.mozilla.org/integration/autoland/rev/e57adbf18a56427b75dce43d01209a717c0a75b9

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=38ddff02f4db8e9b73cc85f466625c59fbd730f7
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=8262348&repo=autoland

[task 2016-12-21T17:11:00.448987Z] 17:11:00     INFO - REFTEST TEST-START | file:///home/worker/workspace/build/tests/reftest/tests/dom/base/crashtests/1324209.html
[task 2016-12-21T17:11:00.449813Z] 17:11:00     INFO - REFTEST TEST-UNEXPECTED-FAIL | file:///home/worker/workspace/build/tests/reftest/tests/dom/base/crashtests/1324209.html | boolean preference 'dom.IntersectionObserver.enabled' not known or wrong type
Flags: needinfo?(tschneider)
Added dom.IntersectionObserver.enabled to modules/libpref/all.js
Attachment #8820459 - Attachment is obsolete: true
Flags: needinfo?(tschneider)
Assignee

Updated

3 years ago
Attachment #8820838 - Attachment description: Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers. → Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers. r=mstange
Assignee

Updated

3 years ago
Keywords: checkin-needed
Fixed the crash test issue. Can we please re-land?
Flags: needinfo?(aryx.bugmail)

Comment 24

3 years ago
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c693641a5ba
Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers. r=mstange
Keywords: checkin-needed

Comment 25

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9c693641a5ba
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
I'm assuming 51 is not affected.
Setting qe-verify- since this seems to have automated coverage.
Flags: qe-verify-
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.