If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Crash in mozilla::dom::Element::UpdateIntersectionObservation

RESOLVED FIXED in Firefox 53

Status

()

Core
DOM
P1
critical
RESOLVED FIXED
11 months ago
27 days ago

People

(Reporter: mccr8, Assigned: tobytailor)

Tracking

(6 keywords)

52 Branch
mozilla53
Unspecified
All
crash, csectype-uaf, regression, reproducible, sec-high, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox49 unaffected, firefox-esr45 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52+ disabled, firefox-esr52 disabled, firefox53 fixed)

Details

(crash signature)

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

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

Comment 1

11 months ago
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
status-firefox51: --- → unaffected
status-firefox52: --- → affected
tracking-firefox52: --- → ?

Comment 2

11 months ago
One of my crashes is bp-419455fd-995b-4c85-8e25-10b9c2161108, happening on macOS.
OS: Windows 8 → All
Flags: needinfo?(tschneider)
Tracking 52+ so we can get to the bottom of why this top crash is still occurring.
tracking-firefox52: ? → +

Updated

11 months ago
Duplicate of this bug: 1315945

Comment 5

11 months ago
str
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

Updated

11 months ago
Has STR: --- → yes
Keywords: reproducible
(Assignee)

Comment 6

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

Comment 7

11 months ago
Created attachment 8808860 [details] [diff] [review]
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.
Attachment #8808860 - Flags: review?(mrbkap)
(Assignee)

Updated

11 months ago
Flags: needinfo?(tschneider)
status-firefox49: --- → unaffected
status-firefox50: --- → unaffected
Version: unspecified → 52 Branch
(Assignee)

Comment 8

11 months ago
Created attachment 8809064 [details] [diff] [review]
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.
Attachment #8808860 - Attachment is obsolete: true
Attachment #8808860 - Flags: review?(mrbkap)
Attachment #8809064 - Flags: review?(mrbkap)

Comment 9

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

Comment 10

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

Updated

11 months ago
Crash Signature: [@ mozilla::dom::Element::UpdateIntersectionObservation] → [@ mozilla::dom::Element::UpdateIntersectionObservation] [@ mozilla::dom::DOMIntersectionObserver::Update]
Crash Signature: [@ mozilla::dom::Element::UpdateIntersectionObservation] [@ mozilla::dom::DOMIntersectionObserver::Update] → [@ mozilla::dom::Element::UpdateIntersectionObservation] [@ mozilla::dom::DOMIntersectionObserver::Update] [@ <name omitted> | mozilla::dom::DOMIntersectionObserver::Update]
Assignee: nobody → tschneider
Priority: -- → P1

Comment 11

11 months ago
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.
Attachment #8809064 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
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

11 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63cafb0c2f0f
Fix crash in mozilla::dom::Element::UpdateIntersectionObservation. r=mrbkap
Keywords: checkin-needed

Comment 14

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/63cafb0c2f0f
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52

Comment 15

10 months ago
My app no longer crashes on the latest Nightly. Thank you!
Status: RESOLVED → VERIFIED
status-firefox53: --- → verified
(Reporter)

Updated

10 months ago
Depends on: 1317415
(Reporter)

Comment 16

10 months ago
I backed this out for causing bug 1317415, which is happening far more frequently on Nightly that the crash this bug fixes.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 17

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a768d3a1ea834a4898e3c6703f295089b67f3b
(Assignee)

Comment 18

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

Comment 19

10 months ago
Created attachment 8811069 [details] [diff] [review]
Part 1: Make sure to disconnect from all targets in case observer gets collected first.
Attachment #8809064 - Attachment is obsolete: true
Attachment #8811069 - Flags: review?(mrbkap)
(Assignee)

Updated

10 months ago
Blocks: 1317415
No longer depends on: 1317415

Updated

10 months ago
Attachment #8811069 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 20

10 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7cb56d86cb7
Crash in mozilla::dom::Element::UpdateIntersectionObservation. r=mrbkap
Keywords: checkin-needed
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.
status-firefox52: fixed → affected
status-firefox53: verified → affected
Target Milestone: mozilla52 → ---

Comment 22

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a7cb56d86cb7
Status: REOPENED → RESOLVED
Last Resolved: 11 months ago10 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 23

10 months ago
Needs an uplift request, I guess.
Flags: needinfo?(tschneider)

Comment 24

10 months ago
https://hg.mozilla.org/mozilla-central/rev/a7cb56d86cb7
Depends on: 1318998

Comment 25

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

Comment 26

10 months ago
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.
Flags: needinfo?(tschneider)
Attachment #8811069 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

10 months ago
Attachment #8811069 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

10 months ago
Attachment #8811069 - Attachment description: Make sure to disconnect from all targets in case observer gets collected first. → Part 1: Make sure to disconnect from all targets in case observer gets collected first.
(Assignee)

Comment 27

10 months ago
Created attachment 8813509 [details] [diff] [review]
Part 2: Avoid cycles when unlinking targets

Blake, please review the fixes for the issues we found during our debug session today.
Attachment #8813509 - Flags: review?(mrbkap)

Comment 28

10 months ago
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).
Attachment #8813509 - Flags: review?(mrbkap) → feedback+
(Assignee)

Comment 29

10 months ago
Created attachment 8813853 [details] [diff] [review]
Part 2: Avoid cycles when unlinking targets.

Addressed review comments.
Attachment #8813509 - Attachment is obsolete: true

Comment 30

10 months ago
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.
Attachment #8813853 - Flags: review+
(Assignee)

Comment 31

10 months ago
Created attachment 8813857 [details] [diff] [review]
Part 2: Avoid cycles when unlinking targets. r=mrbkap

Removed trailing space.
Attachment #8813853 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Keywords: checkin-needed
(Assignee)

Comment 32

10 months ago
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.
Attachment #8811069 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 33

10 months ago
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.
Attachment #8813857 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

10 months ago
Blocks: 1318998
No longer depends on: 1318998

Comment 34

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9f20dbe8d8
Crash in mozilla::dom::Element::UpdateIntersectionObservation. r=mrbkap
Keywords: checkin-needed
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

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf9f20dbe8d8
(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?
Flags: needinfo?(tschneider)
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
Attachment #8811069 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8813857 [details] [diff] [review]
Part 2: Avoid cycles when unlinking targets. r=mrbkap

fix top crash in aurora52
Attachment #8813857 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 40

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/853ce442353a
status-firefox52: affected → fixed

Comment 41

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

Comment 42

10 months ago
Bug 1315837 possibly causing Bug 1320367?
Depends on: 1320367
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

10 months ago
So, since part 2 is currently backed out on Aurora, I'm guessing it's not quite
fixed there yet?
status-firefox52: fixed → affected

Comment 45

10 months ago
UAF signatures in a moderate selection of the crash reports, also this is believed to cause other related UAF signatures tracked in other bugs.
Group: core-security
Keywords: csectype-uaf, sec-high

Comment 46

10 months ago
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....
Flags: needinfo?(philringnalda)
Correct.
Flags: needinfo?(philringnalda)

Updated

10 months ago
Group: core-security → core-security-release
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?

Updated

10 months ago
Blocks: 1321865

Comment 49

10 months ago
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)
status-firefox52: affected → disabled
(Assignee)

Updated

10 months ago
Flags: needinfo?(tschneider)
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → disabled
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.