Closed Bug 1324209 Opened 4 years ago Closed 4 years ago

Crash in InvalidArrayIndex_CRASH | nsDocument::NotifyIntersectionObservers

Categories

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

53 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 + fixed

People

(Reporter: alice0775, Assigned: tschneider)

References

Details

(4 keywords, Whiteboard: [DUPEME])

Crash Data

Attachments

(1 file, 3 obsolete files)

[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
oops typo.

3. Open http://money.cnn.com/2016/11/25/media/rogue-one-tickets/ in the same tab
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 ]
https://hg.mozilla.org/mozilla-central/rev/1016d96bdabb
https://hg.mozilla.org/mozilla-central/rev/f22c6dc53276
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Guess I formatted the commit message on that backout incorrectly.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Clear mRegisteredIntersectionObservers properly when unlinking DOMSlots.
Flags: needinfo?(tschneider)
Keywords: checkin-needed
Keywords: checkin-needed
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 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.
Keywords: checkin-needed
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
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 ]
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)
Attachment #8820838 - Attachment description: Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers. → Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers. r=mstange
Keywords: checkin-needed
Fixed the crash test issue. Can we please re-land?
Flags: needinfo?(aryx.bugmail)
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
https://hg.mozilla.org/mozilla-central/rev/9c693641a5ba
Status: REOPENED → RESOLVED
Closed: 4 years ago4 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.