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

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

Description

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

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

Comment 1

[Jesse Schwartzentruber (:truber)]

Attachment: log.txt

Comment 2

[Andreas Pehrson [:pehrsons]]

StreamCaptureTrackSource landed in 51, so assuming 51 and onward are affected.

Comment 3

[Andreas Pehrson [:pehrsons]]

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.

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1329075 - Fix assertion failure in debug build.

With the attached test case I was failing an assert in a debug build because
NotifyMediaTrackEnabled exited early due to `mReadyState == HAVE_NOTHING` but
NotifyMediaTrackDisabled didn't. The latter would fail an assert.

Review commit: https://reviewboard.mozilla.org/r/103334/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103334/

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1329075 - Fix potential null deref issues in media element track sources.

Review commit: https://reviewboard.mozilla.org/r/103336/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103336/

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1329075 - Add asserts to StreamCaptureTrackSource for sanity.

Review commit: https://reviewboard.mozilla.org/r/103338/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103338/

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1329075 - Avoid an infinite event loop spin.

Because we add tracks to the output streams async, it's possible to switch the
mSrcStream of a media element and *then* get a notification of an added track,
when this track originated from the old mSrcStream.
If the new mSrcStream is an output stream of the media element, this would
again add a new track async, which on the next event loop spin would show up
on mSrcStream, and the loop continues.

Review commit: https://reviewboard.mozilla.org/r/103340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103340/

Comment 8

[Andreas Pehrson [:pehrsons]]

Kudos for the testcase, it discovered a bunch of issues.

Comment 9

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

Comment on attachment 8825100 [details]
Bug 1329075 - Fix assertion failure in debug build.

https://reviewboard.mozilla.org/r/103334/#review103922

Comment 10

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

Comment on attachment 8825101 [details]
Bug 1329075 - Fix potential null deref issues in media element track sources.

https://reviewboard.mozilla.org/r/103336/#review103924

Comment 11

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

Comment on attachment 8825102 [details]
Bug 1329075 - Add asserts to StreamCaptureTrackSource for sanity.

https://reviewboard.mozilla.org/r/103338/#review103928

Comment 12

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

Comment on attachment 8825103 [details]
Bug 1329075 - Avoid an infinite event loop spin.

https://reviewboard.mozilla.org/r/103340/#review103932

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b26c4a99a679
https://hg.mozilla.org/mozilla-central/rev/4be3015951a9
https://hg.mozilla.org/mozilla-central/rev/116d0176a346
https://hg.mozilla.org/mozilla-central/rev/d9b68272db66

Comment 15

[Andreas Pehrson [:pehrsons]]

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.

Comment 16

[Andreas Pehrson [:pehrsons]]

(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.

Comment 17

[Andreas Pehrson [:pehrsons]]

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

Comment 18

[Gerry Chang [:gchang]]

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.

Comment 19

[Andreas Pehrson [:pehrsons]]

(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.

Comment 20

[Julien Cristau [:jcristau]]

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...

Comment 21

[Andreas Pehrson [:pehrsons]]

Jesse, what happens to these testcases that you make? Do they run automatically anywhere?

Comment 22

[Gerry Chang [:gchang]]

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.

Comment 23

[Jesse Schwartzentruber (:truber)]

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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/bde4edd2b386
https://hg.mozilla.org/releases/mozilla-aurora/rev/5086cf9061bd
https://hg.mozilla.org/releases/mozilla-aurora/rev/99745f6d10aa
https://hg.mozilla.org/releases/mozilla-aurora/rev/c1748b934c37

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Please do get this testcase landed as a crashtest.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/43abd9683a4a
https://hg.mozilla.org/releases/mozilla-beta/rev/c1d413e08878
https://hg.mozilla.org/releases/mozilla-beta/rev/25d6e63be71a

Comment 27

[Andreas Pehrson [:pehrsons]]

Well I'm basically asking because the test and crash depend on `fuzzPriv.quitApplication()`. How would you go about that in a crashtest?

Comment 28

[Jesse Schwartzentruber (:truber)]

Attachment: testcase2.html

My mistake. It also works with window.close (assuming that's pref'ed on in tests).