1547784 - AddressSanitizer: SEGV /builds/worker/workspace/build/src/dom/html/HTMLMediaElement.cpp:4584:15 in mozilla::dom::HTMLMediaElement::FinishDecoderSetup(mozilla::MediaDecoder*)

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev 420e18a75314.

==20675==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fcce90f8953 bp 0x7ffef95142f0 sp 0x7ffef95141e0 T0)
==20675==The signal is caused by a READ memory access.
==20675==Hint: address points to the zero page.
#0 0x7fcce90f8952 in mozilla::dom::HTMLMediaElement::FinishDecoderSetup(mozilla::MediaDecoder*) /builds/worker/workspace/build/src/dom/html/HTMLMediaElement.cpp:4584:15
#1 0x7fcce90f09ea in mozilla::dom::HTMLMediaElement::LoadResource() /builds/worker/workspace/build/src/dom/html/HTMLMediaElement.cpp:2578:10
#2 0x7fcce90ed42d in mozilla::dom::HTMLMediaElement::SelectResource() /builds/worker/workspace/build/src/dom/html/HTMLMediaElement.cpp:2112:12
#3 0x7fcce90ea9b9 in mozilla::dom::HTMLMediaElement::SelectResourceWrapper() /builds/worker/workspace/build/src/dom/html/HTMLMediaElement.cpp:2055:3
#4 0x7fcce917822b in applyImpl<mozilla::dom::HTMLMediaElement, void (mozilla::dom::HTMLMediaElement::)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1122:12
#5 0x7fcce917822b in apply<mozilla::dom::HTMLMediaElement, void (mozilla::dom::HTMLMediaElement::)()> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1128
#6 0x7fcce917822b in mozilla::detail::RunnableMethodImpl<mozilla::dom::HTMLMediaElement*, void (mozilla::dom::HTMLMediaElement::)(), true, (mozilla::RunnableKind)0>::Run() /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:1174
#7 0x7fcce915eebf in mozilla::dom::nsSyncSection::Run() /builds/worker/workspace/build/src/dom/html/HTMLMediaElement.cpp:1916:16
#8 0x7fcce08910b7 in mozilla::CycleCollectedJSContext::ProcessStableStateQueue() /builds/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:382:12
#9 0x7fcce0894172 in mozilla::CycleCollectedJSContext::AfterProcessTask(unsigned int) /builds/worker/workspace/build/src/xpcom/base/CycleCollectedJSContext.cpp:441:3
#10 0x7fcce30157e5 in XPCJSContext::AfterProcessTask(unsigned int) /builds/worker/workspace/build/src/js/xpconnect/src/XPCJSContext.cpp:1273:28
#11 0x7fcce0b28296 in nsThread::ProcessNextEvent(bool, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1240:24
#12 0x7fcce0b2ef64 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:486:10
#13 0x7fcce1e9507f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:88:21
#14 0x7fcce1d6ddfe in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#15 0x7fcce1d6ddfe in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#16 0x7fcce1d6ddfe in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#17 0x7fcceb432ad3 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:137:27
#18 0x7fccefa3dd7e in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:919:20
#19 0x7fcce1d6ddfe in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:315:10
#20 0x7fcce1d6ddfe in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:308
#21 0x7fcce1d6ddfe in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:290
#22 0x7fccefa3ceec in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:757:34
#23 0x5589adad172e in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#24 0x5589adad172e in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:263
#25 0x7fcd04c5fb96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

alwu:

Looks like you're active maintainer of HTMLMediaElement, could you take a look or redirect to another person?

Comment 2

[Alastor Wu [:alwu]]

Sure, will take a look later, keep NI.

Comment 3

[Alastor Wu [:alwu]]

This crash happened in [1], because we had released the decoder in previous line [2] as the document was not an active document [3]. The document which was used to create video was not a current document of the doc shell, so we detroied the decoder.

From the codebase, it seems like we don't want to keep media key alive when document becomes inactive [4], but what would happen if we set media key for a video element which is create from an inactive document?

[1] https://searchfox.org/mozilla-central/rev/b756e6d00728dda4121f8278a744381d8643317a/dom/html/HTMLMediaElement.cpp#4584
[2] https://searchfox.org/mozilla-central/rev/b756e6d00728dda4121f8278a744381d8643317a/dom/html/HTMLMediaElement.cpp#4581
[3] https://searchfox.org/mozilla-central/rev/b756e6d00728dda4121f8278a744381d8643317a/dom/html/HTMLMediaElement.cpp#6042-6050
[4] https://searchfox.org/mozilla-central/rev/b756e6d00728dda4121f8278a744381d8643317a/dom/html/HTMLMediaElement.cpp#6046

---

Hi, Bryce,
Because I'm not clear what the correct behavior is for the media key, do you mind have a look for this bug?
Thank you.

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1547784 - Add crashtest for moving media element with media keys to inactive doc. r?alwu

Comment 5

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Attachment: Bug 1547784 - Return an error if an EME associated MediaElement becomes inactive as deocder setup finishes. r?alwu

This fixes an edge case where it was possible for an HTMLMediaElement in the
middle of setup to have ownership transferred to a inactive document and deref a
null pointer. This happened because we have special handling for EME related
media where we perform more aggressive shutdown on for media in inactive
documents.

As far as I can tell, there's nothing specced that forbids performing EME
related functionality on elements in inactive documents. However, our code
already prevents doing so in other cases. E.g. if you create an inactive
document, place an HTMLMediaElement in it and try to setup EME related data on
it, then that will fail. So this fix just covers another such case.

While it would be nice to support doing these operations on inactive document's
media, it seems like very much an edge case, and something that would require a
large amount of reworking in how we handle inactive documents. We can cross that
bridge later should we ever need do so.

Depends on D40481

Comment 6

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Holding off on landing this as I'm seeing another crash on the testcase and want to identify what's going on there.

Edit: think the other crash was down to me accidentally removing the reftest-wait from the test and getting into some bizzaro race. As such, I think this can go ahead.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/54b3b878f07d
Add crashtest for moving media element with media keys to inactive doc. r=alwu
https://hg.mozilla.org/integration/autoland/rev/21f0da4d5763
Return an error if an EME associated MediaElement becomes inactive as deocder setup finishes. r=alwu

Comment 8

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2fcf4972c072
Backed out 2 changesets for crashtest failrues on 1547784.html . CLOSED TREE

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Comment 10

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Not a regression to the best of my knowledge. Getting ready to reland.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b56af8ee88e
Add crashtest for moving media element with media keys to inactive doc. r=alwu
https://hg.mozilla.org/integration/autoland/rev/2a7a1901557e
Return an error if an EME associated MediaElement becomes inactive as deocder setup finishes. r=alwu

Comment 12

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3b56af8ee88e
https://hg.mozilla.org/mozilla-central/rev/2a7a1901557e

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.