Closed
Bug 1315837
Opened 8 years ago
Closed 8 years ago
Crash in mozilla::dom::Element::UpdateIntersectionObservation
Categories
(Core :: DOM: Core & HTML, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox49 | --- | unaffected |
firefox-esr45 | --- | unaffected |
firefox50 | --- | unaffected |
firefox51 | --- | unaffected |
firefox52 | + | disabled |
firefox-esr52 | --- | disabled |
firefox53 | --- | fixed |
People
(Reporter: mccr8, Assigned: tschneider)
References
Details
(6 keywords)
Crash Data
Attachments
(2 files, 4 obsolete files)
3.38 KB,
patch
|
mrbkap
:
review+
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.03 KB,
patch
|
jcristau
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•8 years 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
Comment 2•8 years ago
|
||
One of my crashes is bp-419455fd-995b-4c85-8e25-10b9c2161108, happening on macOS.
OS: Windows 8 → All
Updated•8 years ago
|
Flags: needinfo?(tschneider)
Comment 3•8 years ago
|
||
Tracking 52+ so we can get to the bottom of why this top crash is still occurring.
Comment 5•8 years 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•8 years ago
|
Has STR: --- → yes
Keywords: reproducible
Assignee | ||
Comment 6•8 years 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•8 years ago
|
||
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•8 years ago
|
Flags: needinfo?(tschneider)
status-firefox49:
--- → unaffected
status-firefox50:
--- → unaffected
Version: unspecified → 52 Branch
Assignee | ||
Comment 8•8 years ago
|
||
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•8 years 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•8 years 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•8 years ago
|
Crash Signature: [@ mozilla::dom::Element::UpdateIntersectionObservation] → [@ mozilla::dom::Element::UpdateIntersectionObservation] [@ mozilla::dom::DOMIntersectionObserver::Update]
Updated•8 years ago
|
Crash Signature: [@ mozilla::dom::Element::UpdateIntersectionObservation] [@ mozilla::dom::DOMIntersectionObserver::Update] → [@ mozilla::dom::Element::UpdateIntersectionObservation]
[@ mozilla::dom::DOMIntersectionObserver::Update]
[@ <name omitted> | mozilla::dom::DOMIntersectionObserver::Update]
Updated•8 years ago
|
Assignee: nobody → tschneider
Priority: -- → P1
Comment 11•8 years 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•8 years ago
|
Keywords: checkin-needed
Comment 12•8 years ago
|
||
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•8 years 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/63cafb0c2f0f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Comment 15•8 years ago
|
||
My app no longer crashes on the latest Nightly. Thank you!
Status: RESOLVED → VERIFIED
status-firefox53:
--- → verified
Reporter | ||
Comment 16•8 years 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•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1a768d3a1ea834a4898e3c6703f295089b67f3b
Assignee | ||
Comment 18•8 years 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•8 years ago
|
||
Attachment #8809064 -
Attachment is obsolete: true
Attachment #8811069 -
Flags: review?(mrbkap)
Assignee | ||
Updated•8 years ago
|
Updated•8 years ago
|
Attachment #8811069 -
Flags: review?(mrbkap) → review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 20•8 years 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
Comment 21•8 years ago
|
||
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.
Updated•8 years ago
|
Target Milestone: mozilla52 → ---
Comment 22•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a7cb56d86cb7
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 25•8 years 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•8 years 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•8 years ago
|
Attachment #8811069 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•8 years 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•8 years ago
|
||
Blake, please review the fixes for the issues we found during our debug session today.
Attachment #8813509 -
Flags: review?(mrbkap)
Comment 28•8 years 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•8 years ago
|
||
Addressed review comments.
Attachment #8813509 -
Attachment is obsolete: true
Comment 30•8 years 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•8 years ago
|
||
Removed trailing space.
Attachment #8813853 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 32•8 years 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•8 years 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•8 years ago
|
Comment 34•8 years 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
Comment 35•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bf9f20dbe8d8
Comment 37•8 years ago
|
||
(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 38•8 years ago
|
||
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 39•8 years ago
|
||
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•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/853ce442353a
Comment 41•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/8102d0d48f8d7ea0f6fa76710ca8c44c620190e3
Comment 42•8 years ago
|
||
Bug 1315837 possibly causing Bug 1320367?
Comment 43•8 years ago
|
||
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•8 years ago
|
||
So, since part 2 is currently backed out on Aurora, I'm guessing it's not quite fixed there yet?
Comment 45•8 years 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•8 years 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)
Updated•8 years ago
|
Group: core-security → core-security-release
Comment 48•8 years ago
|
||
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•8 years 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)
Updated•8 years ago
|
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(tschneider)
Updated•7 years ago
|
status-firefox-esr45:
--- → unaffected
Updated•7 years ago
|
status-firefox-esr52:
--- → disabled
Updated•7 years ago
|
Group: core-security-release
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•