Null-deref in [@ HTMLMediaElement::StreamCaptureTrackSource::GetCORSMode]

RESOLVED FIXED in Firefox 51

Status

()

defect
P1
critical
Rank:
17
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: truber, Assigned: pehrsons)

Tracking

(Blocks 1 bug, {crash, csectype-nullptr, testcase})

Trunk
mozilla53
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox50 unaffected, firefox51 fixed, firefox52 fixed, firefox53 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Posted file testcase.html (obsolete) —
The attached testcase causes a null deref in mozilla-central rev f13abb8ba9f3

==25061==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f283092f3eb bp 0x7ffe756537b0 sp 0x7ffe756537b0 T0)
    #0 0x7f283092f3ea in mozilla::dom::HTMLMediaElement::StreamCaptureTrackSource::GetCORSMode() const /home/worker/workspace/build/src/dom/html/HTMLMediaElement.cpp:2930:12
    #1 0x7f28310f7e1c in GetCORSMode /home/worker/workspace/build/src/dom/media/MediaStreamTrack.h:338:41
    #2 0x7f28310f7e1c in mozilla::dom::MediaStreamAudioSourceNode::PrincipalChanged(mozilla::dom::MediaStreamTrack*) /home/worker/workspace/build/src/dom/media/webaudio/MediaStreamAudioSourceNode.cpp:217
    #3 0x7f28310f77df in mozilla::dom::MediaStreamAudioSourceNode::AttachToTrack(RefPtr<mozilla::dom::MediaStreamTrack> const&) /home/worker/workspace/build/src/dom/media/webaudio/MediaStreamAudioSourceNode.cpp:123:3
    #4 0x7f2830a9d49e in mozilla::DOMMediaStream::NotifyTrackAdded(RefPtr<mozilla::dom::MediaStreamTrack> const&) /home/worker/workspace/build/src/dom/media/DOMMediaStream.cpp:1331:5
    #5 0x7f2830aa2ec7 in mozilla::DOMMediaStream::AddTrackInternal(mozilla::dom::MediaStreamTrack*) /home/worker/workspace/build/src/dom/media/DOMMediaStream.cpp:1057:3
    #6 0x7f283093af6e in applyImpl<mozilla::DOMMediaStream, void (mozilla::DOMMediaStream::*)(mozilla::dom::MediaStreamTrack *), StorensRefPtrPassByPtr<mozilla::dom::MediaStreamTrack> , 0> /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:791:12
    #7 0x7f283093af6e in apply<mozilla::DOMMediaStream, void (mozilla::DOMMediaStream::*)(mozilla::dom::MediaStreamTrack *)> /home/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:797
Posted file log.txt
Rank: 17
Priority: -- → P1
StreamCaptureTrackSource landed in 51, so assuming 51 and onward are affected.
Assignee: nobody → pehrson
This happens when StreamCaptureTrackSource::GetCORSMode() is called after StreamCaptureTrackSource::Destroy(). It'd be really exceptional if that happened outside of shutdown, if at all possible.
Blocks: 1259788
Component: Web Audio → Audio/Video: MediaStreamGraph
Kudos for the testcase, it discovered a bunch of issues.
Status: NEW → ASSIGNED
Comment on attachment 8825100 [details]
Bug 1329075 - Fix assertion failure in debug build.

https://reviewboard.mozilla.org/r/103334/#review103922
Attachment #8825100 - Flags: review?(rjesup) → review+
Comment on attachment 8825101 [details]
Bug 1329075 - Fix potential null deref issues in media element track sources.

https://reviewboard.mozilla.org/r/103336/#review103924
Attachment #8825101 - Flags: review?(rjesup) → review+
Comment on attachment 8825102 [details]
Bug 1329075 - Add asserts to StreamCaptureTrackSource for sanity.

https://reviewboard.mozilla.org/r/103338/#review103928
Attachment #8825102 - Flags: review?(rjesup) → review+
Comment on attachment 8825103 [details]
Bug 1329075 - Avoid an infinite event loop spin.

https://reviewboard.mozilla.org/r/103340/#review103932
Attachment #8825103 - Flags: review?(rjesup) → review+
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b26c4a99a679
Fix assertion failure in debug build. r=jesup
https://hg.mozilla.org/integration/autoland/rev/4be3015951a9
Fix potential null deref issues in media element track sources. r=jesup
https://hg.mozilla.org/integration/autoland/rev/116d0176a346
Add asserts to StreamCaptureTrackSource for sanity. r=jesup
https://hg.mozilla.org/integration/autoland/rev/d9b68272db66
Avoid an infinite event loop spin. r=jesup
Comment on attachment 8825101 [details]
Bug 1329075 - Fix potential null deref issues in media element track sources.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1259788
[User impact if declined]: There's a chance of crashes, closely tied to shutdown. There might also be a main thread freeze that is timing dependent and only in a new feature in 51.
[Is this code covered by automated tests?]: Detected by a fuzzing test. I'm unsure how they run in automation.
[Has the fix been verified in Nightly?]: I have verified it with the attached testcase on the merge revision, with the build from treeherder.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Changes are small, simple and isolated.
[String changes made/needed]: None.


This uplift request applies to all patches on this bug.
Attachment #8825101 - Flags: approval-mozilla-beta?
Attachment #8825101 - Flags: approval-mozilla-aurora?
(In reply to Andreas Pehrson [:pehrsons] (Telenor) from comment #15)
> This uplift request applies to all patches on this bug.

Though attachment 8825102 [details] ("Add asserts to StreamCaptureTrackSource for sanity") doesn't go cleanly on beta and can be safely ignored there.
I did some digging and noted that the null deref crash should not happen in 51 - it was introduced by bug 1301675 in [1], so in 52. The main thread freeze and the assertion failure might still happen though.


[1] http://searchfox.org/mozilla-central/diff/220b0176e07ad910210997c4e176a4ca7a50dcd8/dom/html/HTMLMediaElement.cpp#2252
Blocks: 1301675
Hi :pehrsons, 
Per comment #17, do we still need these patches in Beta51? It seems to me that the patches don't fix the issue.
Flags: needinfo?(pehrson)
(In reply to Gerry Chang [:gchang] (OOO 26 Dec - 10 Jan) from comment #18)
> Hi :pehrsons, 
> Per comment #17, do we still need these patches in Beta51? It seems to me
> that the patches don't fix the issue.

"Fix assertion failure in debug build" and  "Avoid an infinite event loop spin" definitely fix issues in 51 - the ones mentioned in comment 17. The comment was about what could happen without any patches, sorry for any confusion.
Flags: needinfo?(pehrson)
Comment on attachment 8825101 [details]
Bug 1329075 - Fix potential null deref issues in media element track sources.

fix potential crashes/hangs related to media element in aurora52

would be nice to have automated tests for this, maybe as a followup...
Attachment #8825101 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Jesse, what happens to these testcases that you make? Do they run automatically anywhere?
Flags: needinfo?(jschwartzentruber)
Comment on attachment 8825101 [details]
Bug 1329075 - Fix potential null deref issues in media element track sources.

Fix a potential crash. Beta51+. Should be in 51 beta 14.
Attachment #8825101 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
If the testcase is suitable, usually it would be added as a patch and landed together with the fix. A random example would be bug 1299062.
Flags: needinfo?(jschwartzentruber)
Flags: in-testsuite?
Please do get this testcase landed as a crashtest.
Flags: needinfo?(pehrson)
Well I'm basically asking because the test and crash depend on `fuzzPriv.quitApplication()`. How would you go about that in a crashtest?
Flags: needinfo?(jschwartzentruber)
Posted file testcase2.html
My mistake. It also works with window.close (assuming that's pref'ed on in tests).
Attachment #8824312 - Attachment is obsolete: true
Flags: needinfo?(pehrson)
Flags: needinfo?(jschwartzentruber)
You need to log in before you can comment on or make changes to this bug.