Closed
Bug 1255618
Opened 8 years ago
Closed 8 years ago
crash in mozilla::dom::AudioDestinationNode::DestroyAudioChannelAgent
Categories
(Core :: Web Audio, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla48
People
(Reporter: philipp, Assigned: karlt)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
ehsan.akhgari
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
|
Details |
58 bytes,
text/x-review-board-request
|
ehsan.akhgari
:
review+
|
Details |
This bug was filed from the Socorro interface and is report bp-6eae7b0c-9356-4aa4-a2c4-0ad0a2160310. ============================================================= Crashing Thread (0) Frame Module Signature Source 0 xul.dll mozilla::dom::AudioDestinationNode::DestroyAudioChannelAgent() dom/media/webaudio/AudioDestinationNode.cpp 1 xul.dll mozilla::dom::AudioContext::Suspend(mozilla::ErrorResult&) dom/media/webaudio/AudioContext.cpp 2 xul.dll nsGlobalWindow::SuspendTimeouts(unsigned int, bool, bool) dom/base/nsGlobalWindow.cpp 3 xul.dll nsGlobalWindow::SuspendTimeouts(unsigned int, bool, bool) dom/base/nsGlobalWindow.cpp 4 xul.dll nsGlobalWindow::SuspendTimeouts(unsigned int, bool, bool) dom/base/nsGlobalWindow.cpp 5 xul.dll nsXMLHttpRequest::Send(nsIVariant*, mozilla::dom::Nullable<nsXMLHttpRequest::RequestBody> const&) dom/base/nsXMLHttpRequest.cpp 6 xul.dll nsXMLHttpRequest::Send(JSContext*, mozilla::ErrorResult&) dom/base/nsXMLHttpRequest.h 7 xul.dll mozilla::dom::XMLHttpRequestBinding::send obj-firefox/dom/bindings/XMLHttpRequestBinding.cpp 8 xul.dll mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) dom/bindings/BindingUtils.cpp 9 xul.dll js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct) js/src/vm/Interpreter.cpp 10 xul.dll Interpret js/src/vm/Interpreter.cpp 11 xul.dll js::RunScript(JSContext*, js::RunState&) js/src/vm/Interpreter.cpp 12 xul.dll js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) js/src/vm/Interpreter.cpp 13 xul.dll ExecuteScript js/src/jsapi.cpp 14 xul.dll ExecuteScript js/src/jsapi.cpp 15 xul.dll JS_ExecuteScript(JSContext*, JS::AutoVectorRooter<JSObject*>&, JS::Handle<JSScript*>) js/src/jsapi.cpp 16 xul.dll nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, nsJSUtils::EvaluateOptions const&, JS::MutableHandle<JS::Value>, void**) dom/base/nsJSUtils.cpp 17 xul.dll nsJSUtils::EvaluateString(JSContext*, JS::SourceBufferHolder&, JS::Handle<JSObject*>, JS::CompileOptions&, void**) dom/base/nsJSUtils.cpp 18 xul.dll nsScriptLoader::EvaluateScript(nsScriptLoadRequest*, JS::SourceBufferHolder&) dom/base/nsScriptLoader.cpp this cross-platform crash seems to be regressing since firefox 42 builds, but its volume is visibly spiking recently (since 2016-02-25): https://crash-stats.mozilla.com/signature/?date=%3E2015-06-01&signature=mozilla%3A%3Adom%3A%3AAudioDestinationNode%3A%3ADestroyAudioChannelAgent#graphs on 44.0.2 it is on #24 of the top crash list with 0.5% of all crashes in this version (in 45 it's still too hard to quantify the "real" impact, since trusteer crashes are dominating the data).
Assignee | ||
Comment 1•8 years ago
|
||
Thanks for the report. Offset 0xb0 in amd64 reports is consistent with AudioDestinationNode::mAudioChannelAgent suggesting that mDestination is null at http://hg.mozilla.org/releases/mozilla-release/annotate/b6609650a911/dom/media/webaudio/AudioContext.cpp#l869 which is the situation after the AudioContext is unlinked. So this looks like a regression from http://hg.mozilla.org/releases/mozilla-release/rev/feb02fb51745#l6.12 The AudioContext is not removed from the nsGlobalWindow until ~AudioContext(). Perhaps, because the AudioContext is not in a usable state after Unlink(), the safest thing would be to remove from nsGlobalWindow in Unlink(). I don't know whether or not we have a convention re removing weak pointers from Unlink(). https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bb587880320
Reporter | ||
Comment 2•8 years ago
|
||
there's also a [@ mozilla::dom::AudioDestinationNode::CreateAudioChannelAgent ] signature spiking since the same date, like: https://crash-stats.mozilla.com/report/index/aa898ff5-d0d7-40e8-8852-6dcbe2160311 should i file a new bug for this one or are they interrelated?
Flags: needinfo?(karlt)
Assignee | ||
Comment 3•8 years ago
|
||
Thanks, philipp. Good to know that version exists too, but it's the same root cause, so no need for a new bug.
Flags: needinfo?(karlt)
Assignee | ||
Comment 4•8 years ago
|
||
mDestination is cleared during unlink, which means that after that point the window can't do much with the AudioContext, nor should need to do so. Review commit: https://reviewboard.mozilla.org/r/39563/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39563/
Attachment #8729695 -
Flags: review?(ehsan)
Reporter | ||
Updated•8 years ago
|
Crash Signature: [@ mozilla::dom::AudioDestinationNode::DestroyAudioChannelAgent] → [@ mozilla::dom::AudioDestinationNode::DestroyAudioChannelAgent]
[@ mozilla::dom::AudioDestinationNode::CreateAudioChannelAgent]
Comment 5•8 years ago
|
||
Comment on attachment 8729695 [details] MozReview Request: bug 1255618 remove AudioContext from global window at unlink r?ehsan https://reviewboard.mozilla.org/r/39563/#review36239
Attachment #8729695 -
Flags: review?(ehsan) → review+
Updated•8 years ago
|
Rank: 7
Comment 6•8 years ago
|
||
Karl -- Thanks for jumping on this. As a follow up, can we add a test? (I don't want to hold up landing this for a test.)
Flags: needinfo?(karlt)
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8729695 [details] MozReview Request: bug 1255618 remove AudioContext from global window at unlink r?ehsan Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39563/diff/1-2/
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8729695 [details] MozReview Request: bug 1255618 remove AudioContext from global window at unlink r?ehsan Sorry to throw this at you again, Ehsan. Are you happen with the disconnect from the destructor also?
Attachment #8729695 -
Flags: review+ → review?(ehsan)
Assignee | ||
Comment 9•8 years ago
|
||
https://reviewboard.mozilla.org/r/39563/#review36289 ::: dom/media/webaudio/AudioContext.cpp:152 (Diff revisions 1 - 2) > AudioContext::~AudioContext() > { > + DisconnectFromWindow(); This version keeps the disconnect in the destructor also, which is what I intended to do, but missed. It's actually unnecessary because unlink is also called because the context and destination always make a cycle, but I don't want to risk leaving a dangling pointer if that ever changes.
Assignee | ||
Comment 10•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/39651/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39651/
Assignee | ||
Updated•8 years ago
|
Attachment #8730071 -
Flags: review?(ehsan)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(karlt)
Comment 11•8 years ago
|
||
Comment on attachment 8729695 [details] MozReview Request: bug 1255618 remove AudioContext from global window at unlink r?ehsan https://reviewboard.mozilla.org/r/39563/#review36371 To be perfectly honest, I thought the original version of the patch did disconnect in the destructor as well, otherwise I'd have asked for it. Thanks for the due diligence and sorry I missed this earlier!
Attachment #8729695 -
Flags: review?(ehsan) → review+
Comment 12•8 years ago
|
||
Comment on attachment 8730071 [details] MozReview Request: mochitest for bug 1255618 r=ehsan https://reviewboard.mozilla.org/r/39651/#review36379 ::: dom/media/webaudio/test/test_bug1255618.html:19 (Diff revision 1) > +function collect_and_fetch() { > + SpecialPowers.forceGC(); > + SpecialPowers.forceCC(); > + > + var xhr = new XMLHttpRequest(); > + xhr.open("GET", filename, false); I assume you're doing the sync XHR to spin the event loop. Please add a comment to that effect.
Attachment #8730071 -
Flags: review?(ehsan) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8730071 -
Attachment description: MozReview Request: mochitest for bug 1255618 → MozReview Request: mochitest for bug 1255618 r=ehsan
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8730071 [details] MozReview Request: mochitest for bug 1255618 r=ehsan Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39651/diff/1-2/
Assignee | ||
Comment 14•8 years ago
|
||
https://reviewboard.mozilla.org/r/39651/#review36379 > I assume you're doing the sync XHR to spin the event loop. Please add a comment to that effect. I added a comment to explain. It's actually to suspend timeouts, as in the crash report stacks.
Comment 15•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/b22ef40863a0 https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6fec908401
Comment 16•8 years ago
|
||
Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/e782585f98ba
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b22ef40863a0
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 18•8 years ago
|
||
Hi Karl -- It looks like the test got backed out, but the fix landed. Can you re-land the test and ask for uplift to Aurora and Beta? (Like I said earlier, I don't want to hold up the fix for the test, especially since this is a regression.) Thanks.
Flags: needinfo?(karlt)
Assignee | ||
Comment 19•8 years ago
|
||
The test depends on bug 1257407. Waiting to check bug 1257090 is fixed before requesting uplift.
Depends on: 1257407
Flags: needinfo?(karlt)
Comment 20•8 years ago
|
||
(In reply to Karl Tomlinson (ni?:karlt) from comment #19) > The test depends on bug 1257407. > > Waiting to check bug 1257090 is fixed before requesting uplift. I'm not sure it makes sense to wait on a retest of bug 1257090 before uplifting this fix. We know you found a real bug here. If bug 1257090 still exists after testing with this fix, we can handle that separately. If you feel strongly we should wait on a retest of bug 1257090, I'll send Jesse email letting him know this is blocked on his re-testing of bug 1257090. Thanks!
Flags: needinfo?(karlt)
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8729695 [details] MozReview Request: bug 1255618 remove AudioContext from global window at unlink r?ehsan Approval Request Comment [Feature/regressing bug #]: bug 1180539 [User impact if declined]: safe crash when sync XHR is used soon after AudioContexts are CCed. #24 crash. [Describe test coverage new/current, TreeHerder]: New test cannot land due to bug 257407. There is however existing test coverage that would have been likely to catch problems with the patch. [Risks and why]: Things involving CC timing are not simple to analyse, but I've looked at the interactions between the objects being separated by this patch and they all should not be able to cause observable effects that would be influenced by this patch. [String/UUID change made/needed]: none. If beta is in a stage of complete risk aversion, then I'd understand rejecting this patch. I do however recommend uplift for stability improvement.
Flags: needinfo?(karlt)
Attachment #8729695 -
Flags: approval-mozilla-beta?
Attachment #8729695 -
Flags: approval-mozilla-aurora?
Wes, Karl, the MozReview request shows 86 test jobs failed on inbound. Is this still ok to uplift? Were those intermittent failures? Thanks!
Flags: needinfo?(wkocher)
Flags: needinfo?(karlt)
From my understanding, those failures in Mozreview were from the test that was added, which was then backed out before the merge to m-c. Sounds like that test depends on another patch landing before it lands. I can't speak to whether it'd be a good idea to uplift the patch without the test, but it seems to be doing okay on m-c.
Flags: needinfo?(wkocher)
Comment on attachment 8729695 [details] MozReview Request: bug 1255618 remove AudioContext from global window at unlink r?ehsan Crash fix, taking it in Aurora47 and Beta 46 (since we still have over half of beta cycle remaining).
Attachment #8729695 -
Flags: approval-mozilla-beta?
Attachment #8729695 -
Flags: approval-mozilla-beta+
Attachment #8729695 -
Flags: approval-mozilla-aurora?
Attachment #8729695 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 26•8 years ago
|
||
Wes described the situation accurately, thanks.
Flags: needinfo?(karlt)
Comment 27•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/4087c90f63ae
Comment 28•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/3377679924f5
Comment 29•7 years ago
|
||
Socorro shows 0 crashes over the past 4 weeks, in builds after this landed.
Status: RESOLVED → VERIFIED
Comment 30•7 years ago
|
||
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/96d810861d79 mochitest for bug 1255618 r=ehsan
Comment 31•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/96d810861d79
Assignee | ||
Updated•7 years ago
|
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•