1324209 - Crash in InvalidArrayIndex_CRASH | nsDocument::NotifyIntersectionObservers

Status: RESOLVED FIXED

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

Description

[Alice0775 White]

[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

Comment 1

[Alice0775 White]

oops typo.

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

Comment 2

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7c828bc4d59ee94d181687fab56957bb8074c5ac&tochange=016b87fe9145354cbdcc1b2bfdeb2cc737962016

Suspect: Bug 1321865

Comment 4

[Andrew McCreight [:mccr8]]

Another STR:
Go to this page, scroll up and down: http://abc7news.com/pets/hillsborough-mountain-lion-goes-viral/1658809/

Comment 5

[Andrew McCreight [:mccr8]]

I got a different crash one time with my STR, so I'll assume it is a similar issue.

Comment 6

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1016d96bdabb
https://hg.mozilla.org/mozilla-central/rev/f22c6dc53276

Comment 7

[Andrew McCreight [:mccr8]]

Guess I formatted the commit message on that backout incorrectly.

Comment 8

[Tobias Schneider [:tobytailor]]

Attachment: Clear mRegisteredIntersectionObservers when unlinking

Clear mRegisteredIntersectionObservers properly when unlinking DOMSlots.

Comment 9

[Tobias Schneider [:tobytailor]]

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.

Comment 10

[Markus Stange [:mstange]]

How is the crash happening, and how does the patch fix the bug?

Comment 11

[Marcia Knous [:marcia]]

Adding the Linux signature and changing to all since it affected all platforms.

Comment 12

[Marcia Knous [:marcia]]

Tracking 53+ for this topcrash regression.

Comment 13

[Tobias Schneider [:tobytailor]]

(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 14

[Markus Stange [:mstange]]

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.

Comment 15

[Tobias Schneider [:tobytailor]]

Attachment: Clear mRegisteredIntersectionObservers when unlinking DOMSlots. r=mstange

Better commit message.

Comment 16

[Tobias Schneider [:tobytailor]]

Attachment: Bug 1324209 - Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers.

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

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8820459 [details]
Bug 1324209 - Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers.

https://reviewboard.mozilla.org/r/99960/#review100614

Comment 18

[Andrew McCreight [:mccr8]]

(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.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38ddff02f4db
Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers. r=mstange

Comment 20

[Tobias Schneider [:tobytailor]]

The crashes in NotifyIntersectionObservers and UnlinkTarget are unrelated. Changed this bug to fix the NotifyIntersectionObservers and created bug 1325103 for UnlinkTarget.

Comment 21

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

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

Comment 22

[Tobias Schneider [:tobytailor]]

Attachment: Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers. r=mstange

Added dom.IntersectionObserver.enabled to modules/libpref/all.js

Comment 23

[Tobias Schneider [:tobytailor]]

Fixed the crash test issue. Can we please re-land?

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c693641a5ba
Safely iterate over mIntersectionObservers in nsDocument::NotifyIntersectionObservers. r=mstange

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9c693641a5ba

Comment 26

[Marco Castelluccio [:marco]]

I'm assuming 51 is not affected.

Comment 27

[Andrei Vaida [:avaida]]

Setting qe-verify- since this seems to have automated coverage.