1315837 - Crash in mozilla::dom::Element::UpdateIntersectionObservation

Status: RESOLVED FIXED

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

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-e76cc856-efe5-4ac5-a9e0-367da2161106.
=============================================================

Bug 1314032 fixed some of the crashes with this signature, but this is still a top crash on Nightly.

(In reply to Tobias Schneider [:tobytailor] from comment #16)
> Andrew, please file a new one, since these crashes seem to happen in
> mozilla::dom::Element::UpdateIntersectionObservation which I'm pretty sure
> is unrelated to this bug.

Comment 1

[Andrew McCreight [:mccr8]]

There's possible STR in bug 1314032 comment 15:
(In reply to Kohei Yoshino [:kohei] from comment #15)
> Yeah, I'm encountering crashes and sending reports from Nightly while
> testing my app that I've just added some Intersection Observer API code [1].
> The crash is intermittent; sometimes it happens in a few seconds after the
> app launched, but not always.
> 
> [1] https://github.com/bzdeck/bzdeck/commit/6dc07fc

[Tracking Requested - why for this release]: top crash

Comment 2

[Kohei Yoshino [:kohei]]

One of my crashes is bp-419455fd-995b-4c85-8e25-10b9c2161108, happening on macOS.

Comment 3

[Marcia Knous [:marcia]]

Tracking 52+ so we can get to the bottom of why this top crash is still occurring.

Comment 5

[Alice0775 White]

Reproducible: 100%

Steps To Reproduce:
1. open https://soundcloud.com/ (no need logged in)
2. click [>] in first thumbnail
3. Type Madonna in input field, click 1st suggestion, click [>] in first thumbnail
4. Type Madonna in input field, click 2nd suggestion, click [>]
5. Type Madonna in input field, click 3rd suggestion, click [>]

Actual Results:
Tab crashes



Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=45b462d40300df32424e2b582eb3b491de7331de&tochange=72e781308ebf6a06c65a4400c28c4ad813bd1081

Regressed by: 72e781308ebf	Tobias Schneider — Bug 1243846 - Implement Intersection Observer API. r=mrbkap, r=mstange

Comment 6

[Tobias Schneider [:tobytailor]]

Alice0775, thanks for the STR. I was already able to find out whats wrong and your steps confirmed my findings. Working on a fix.

Comment 7

[Tobias Schneider [:tobytailor]]

Attachment: Fix Unobserve/Disconnect

Fixes crash by making sure we don't keep observing a target that got collected without telling an observing Intersection Observer. Also make sure we disconnect Intersection Observers properly when dropped.

Comment 8

[Tobias Schneider [:tobytailor]]

Attachment: Properly unobserve when collecting target

Small update. Calling Disconnect shouldn't be needed at deconstruction time, instead avoid accessing document of mOwner is already gone.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Why Disconnect isn't needed in dtor? What guarantees UnregisterIntersectionObserver is called on all the targets before the DOMIntersectionObserver is deleted?

I'm worried about the raw observer pointers 
http://searchfox.org/mozilla-central/rev/008fdd93290c83325de6629a1ae48dc2afaed868/dom/base/FragmentOrElement.h#351,355

Comment 10

[Tobias Schneider [:tobytailor]]

As long as its observing a target there is always a ref counted pointer on the documents (see mIntersectionObservers in nsDocument). Only hen the last target is gone, that ref gets dropped, which also means, that mObservationTargets is empty, so no need to disconnect in that case. Indeed we could add an assert to make sure mObservationTargets is really empty.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8809064 [details] [diff] [review]
Properly unobserve when collecting target

Review of attachment 8809064 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay. I got a bit lost in the hierarchy of classes and had to re-read a bunch of code to remember how this all ties together. This looks good to me.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

For future reference, please try to use commit messages that summarize what the patch is actually doing instead of restating the problem being solved. Thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63cafb0c2f0f
Fix crash in mozilla::dom::Element::UpdateIntersectionObservation. r=mrbkap

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/63cafb0c2f0f

Comment 15

[Kohei Yoshino [:kohei]]

My app no longer crashes on the latest Nightly. Thank you!

Comment 16

[Andrew McCreight [:mccr8]]

I backed this out for causing bug 1317415, which is happening far more frequently on Nightly that the crash this bug fixes.

Comment 17

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a768d3a1ea834a4898e3c6703f295089b67f3b

Comment 18

[Tobias Schneider [:tobytailor]]

I was wrong in comment 10 thinking we don't need to disconnect from observing targets since targets should be collected before the observer, due to the ref counted reference hold in the document, which should always outlive js objects. Where this might be true for the general case, it might not when the entire tab/window is closed and collected all at once. In this case the order of objects collected seams do be deterministic. Not sure if that's true in general, since I was only able to observe this behavior on Windows systems.

Comment 19

[Tobias Schneider [:tobytailor]]

Attachment: Part 1: Make sure to disconnect from all targets in case observer gets collected first.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7cb56d86cb7
Crash in mozilla::dom::Element::UpdateIntersectionObservation. r=mrbkap

Comment 21

[Chris Peterson [:cpeterson]]

Marking status-firefox52 and 53 as affected based on the backout in comment 16. I think we will need to uplift this fix to Aurora 52.

Comment 22

[Phil Ringnalda (:philor)]

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

Comment 23

[Kohei Yoshino [:kohei]]

Needs an uplift request, I guess.

Comment 24

[areya]

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

Comment 25

[[:philipp]]

these crashes are accounting for more than 60% of crashes in stability data from 52.0a2 (excluding crashes submitted through the infobar)!
can we look into a backout quickly...?

Comment 26

[Tobias Schneider [:tobytailor]]

Comment on attachment 8811069 [details] [diff] [review]
Part 1: Make sure to disconnect from all targets in case observer gets collected first.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Top crasher.
[Describe test coverage new/current, TreeHerder]: Successfully tested in 53a1.
[Risks and why]: High risk. Unfixed implementation source for top crashes in 52a2.
[String/UUID change made/needed]: No string or UUID changes.

Comment 27

[Tobias Schneider [:tobytailor]]

Attachment: Part 2: Avoid cycles when unlinking targets

Blake, please review the fixes for the issues we found during our debug session today.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8813509 [details] [diff] [review]
Part 2: Avoid cycles when unlinking targets

Review of attachment 8813509 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMIntersectionObserver.cpp
@@ +170,4 @@
>      if (!mObservationTargets.Contains(&aTarget)) {
>          return false;
>      }
> +    mObservationTargets.RemoveEntry(&aTarget);

Nit: add a blank line above this one.

@@ +195,4 @@
>    if (!mConnected) {
>      return;
>    }
> +  mConnected = false;

Here too.

@@ +277,5 @@
>        rootFrame = presShell->GetRootScrollFrame();
>        if (rootFrame) {
>          nsPresContext* presContext = rootFrame->PresContext();
> +        while (presContext && !presContext->IsRootContentDocument()) {
> +          presContext = presContext->GetParentPresContext();

We talked about this in person. The repeated null check of presContext reads oddly and should be unnecesary (since we're not calling GetPresContext).

Comment 29

[Tobias Schneider [:tobytailor]]

Attachment: Part 2: Avoid cycles when unlinking targets.

Addressed review comments.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8813853 [details] [diff] [review]
Part 2: Avoid cycles when unlinking targets.

Review of attachment 8813853 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the trailing whitespace removed.

Comment 31

[Tobias Schneider [:tobytailor]]

Attachment: Part 2: Avoid cycles when unlinking targets. r=mrbkap

Removed trailing space.

Comment 32

[Tobias Schneider [:tobytailor]]

Comment on attachment 8811069 [details] [diff] [review]
Part 1: Make sure to disconnect from all targets in case observer gets collected first.

[Feature/regressing bug #]: N/A
[User impact if declined]: Top crasher.
[Describe test coverage new/current, TreeHerder]: Successfully tested in 53a1.
[Risks and why]: High risk. Unfixed implementation source for top crashes in 52a2.
[String/UUID change made/needed]: No string or UUID changes.

Comment 33

[Tobias Schneider [:tobytailor]]

Comment on attachment 8813857 [details] [diff] [review]
Part 2: Avoid cycles when unlinking targets. r=mrbkap

[Feature/regressing bug #]: N/A
[User impact if declined]: Top crasher.
[Describe test coverage new/current, TreeHerder]: Successfully tested in 53a1.
[Risks and why]: High risk. Unfixed implementation source for top crashes in 52a2.
[String/UUID change made/needed]: No string or UUID changes.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9f20dbe8d8
Crash in mozilla::dom::Element::UpdateIntersectionObservation. r=mrbkap

Comment 35

[Nicholas Nethercote [inactive]]

This crash (covered by bug 1317415) is currently #1 and #2 topcrash on Aurora for the past three days, so it'll be good when the fix is uplifted.

Comment 36

[Carsten Book [:Tomcat]]

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

Comment 37

[Julien Cristau [:jcristau]]

(In reply to Tobias Schneider [:tobytailor] from comment #32)
> Comment on attachment 8811069 [details] [diff] [review]
> Part 1: Make sure to disconnect from all targets in case observer gets
> collected first.

As far as I can tell only the hunk touching DOMIntersectionObserver's destructor applies on aurora, as that's the only difference with 63cafb0c2f0f which hasn't been backed out of aurora, right?

Comment 38

[Julien Cristau [:jcristau]]

Comment on attachment 8811069 [details] [diff] [review]
Part 1: Make sure to disconnect from all targets in case observer gets collected first.

let's try and fix this top crash in aurora52

Comment 39

[Julien Cristau [:jcristau]]

Comment on attachment 8813857 [details] [diff] [review]
Part 2: Avoid cycles when unlinking targets. r=mrbkap

fix top crash in aurora52

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/853ce442353a

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8102d0d48f8d7ea0f6fa76710ca8c44c620190e3

Comment 42

[Gary [:streetwolf52]]

Bug 1315837 possibly causing Bug 1320367?

Comment 43

[Phil Ringnalda (:philor)]

Backed out Part 2 in https://hg.mozilla.org/releases/mozilla-aurora/rev/d87b17a47f60 for causing very frequent but not quite permanent assertion failures on Android, https://treeherder.mozilla.org/logviewer.html#?job_id=4291250&repo=mozilla-aurora

Comment 44

[Mats Palmgren (inactive)]

So, since part 2 is currently backed out on Aurora, I'm guessing it's not quite
fixed there yet?

Comment 45

[Randell Jesup [:jesup] (needinfo me)]

UAF signatures in a moderate selection of the crash reports, also this is believed to cause other related UAF signatures tracked in other bugs.

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

Note: given the history and a fix on inbound, I would not wait on sec-approval to reland part 2 on Aurora once the problem is resolved.

Philor - this is asserting on Aurora, but not on Nightly?  Interesting....

Comment 47

[Phil Ringnalda (:philor)]

Correct.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

Are we planning to attempt to re-enable IntersectionObservation on 52 or leave it to the ride the 52 train? In other words, is there anything left to do here for 52?

Comment 49

[Olli Pettay [:smaug][[e-mail]]]

I filed bug 1321865 to track enabling the API again. I assume this crash is some rather simple issue and could be fixed even in aurora. (but we just must not keep crashing new APIs enabled on any branch)