1488832 - Assertion failure: mInitDone, at /builds/worker/workspace/build/src/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:190

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P3)

Description

[Jason Kratzer [:jkratzer]]

Attachment: testcase.html

Testcase found while fuzzing mozilla-central rev 26990836dc5c.

Testcase must be served via a local webserver in order to reproduce.

rax = 0x0000000000000000   rdx = 0x0000000000000000
rcx = 0x0000000000000b40   rbx = 0x00007f959eefcc10
rsi = 0x00007f95ba3478b0   rdi = 0x00007f95ba346680
rbp = 0x00007f959f57c1e0   rsp = 0x00007f959f57bdf0
r8 = 0x00007f95ba3478b0    r9 = 0x00007f959f57d700
r10 = 0x00000000ffffffee   r11 = 0x0000000000000000
r12 = 0x00007f95af6e6200   r13 = 0x00007f95a0463850
r14 = 0x00007f959eef9838   r15 = 0x00007f959eef9870
rip = 0x00007f95aa7e6421
OS|Linux|0.0.0 Linux 4.15.0-33-generic #36-Ubuntu SMP Wed Aug 15 16:00:05 UTC 2018 x86_64
CPU|amd64|family 6 model 78 stepping 3|1
GPU|||
Crash|SIGSEGV /SEGV_MAPERR|0x0|23
23|0|libxul.so|mozilla::MediaEngineRemoteVideoSource::Allocate(mozilla::dom::MediaTrackConstraints const&, mozilla::MediaEnginePrefs const&, nsTString<char16_t> const&, mozilla::ipc::PrincipalInfo const&, mozilla::AllocationHandle**, char const**)|hg:hg.mozilla.org/mozilla-central:dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:26990836dc5cc3cd1b8027392b79210e71094dc3|191|0x18
23|1|libxul.so|mozilla::MediaDevice::Allocate(mozilla::dom::MediaTrackConstraints const&, mozilla::MediaEnginePrefs const&, mozilla::ipc::PrincipalInfo const&, char const**)|hg:hg.mozilla.org/mozilla-central:dom/media/MediaManager.cpp:26990836dc5cc3cd1b8027392b79210e71094dc3|1111|0x13
23|2|libxul.so|mozilla::GetUserMediaTask::Run()|hg:hg.mozilla.org/mozilla-central:dom/media/MediaManager.cpp:26990836dc5cc3cd1b8027392b79210e71094dc3|1850|0x12
23|3|libxul.so|nsThread::ProcessNextEvent(bool, bool*)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThread.cpp:26990836dc5cc3cd1b8027392b79210e71094dc3|1161|0x15
23|4|libxul.so|NS_ProcessNextEvent(nsIThread*, bool)|hg:hg.mozilla.org/mozilla-central:xpcom/threads/nsThreadUtils.cpp:26990836dc5cc3cd1b8027392b79210e71094dc3|519|0x11
23|5|libxul.so|mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)|hg:hg.mozilla.org/mozilla-central:ipc/glue/MessagePump.cpp:26990836dc5cc3cd1b8027392b79210e71094dc3|334|0xa
23|6|libxul.so|MessageLoop::RunInternal()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:26990836dc5cc3cd1b8027392b79210e71094dc3|325|0x17
23|7|libxul.so|MessageLoop::Run()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/message_loop.cc:26990836dc5cc3cd1b8027392b79210e71094dc3|318|0x8
23|8|libxul.so|base::Thread::ThreadMain()|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/thread.cc:26990836dc5cc3cd1b8027392b79210e71094dc3|181|0x8
23|9|libxul.so|ThreadFunc|hg:hg.mozilla.org/mozilla-central:ipc/chromium/src/base/platform_thread_posix.cc:26990836dc5cc3cd1b8027392b79210e71094dc3|44|0xa
23|10|libpthread-2.27.so||||0x76db
23|11|libc-2.27.so||||0x12188f

Comment 1

[Matthew Gregan [:kinetik]]

ni? Jan-Ivar since Andreas is on PTO and I'm not sure who wants to own this

Comment 2

[Andreas Pehrson [:pehrsons]]

So mInitDone can be legitimately false if CamerasChild::GetCaptureDevice in Init() failed. We'll want to change this assert back to the guard we had before.

Comment 3

[Andreas Pehrson [:pehrsons]]

(In reply to Andreas Pehrson [:pehrsons] from comment #2)
> So mInitDone can be legitimately false if CamerasChild::GetCaptureDevice in
> Init() failed. We'll want to change this assert back to the guard we had
> before.

That is actually not what is happening here. When reproducing this I see for the failing MediaEngineRemoteVideoSource the following sequence:

- Init()
- Shutdown()
- Allocate()

No wonder mInitDone is false. Perhaps it's still the best solution to re-instate the runtime guard for mInitDone, but first we have to understand this race properly.

Comment 4

[Andreas Pehrson [:pehrsons]]

We are basically continuing with Allocate() after an async operation despite having started shutting down in the meantime.

Better window validity checks after async operations fixes this.

I'll also revert the runtime guards for mInitDone since GetCaptureDevice could legitimately fail.

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1488832 - Improve window validity checks after async gUM steps. r?jib

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1488832 - Change init assert to runtime guard. r?jib

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9009931 [details]
Bug 1488832 - Change init assert to runtime guard. r?jib

Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision.

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9009930 [details]
Bug 1488832 - Improve window validity checks after async gUM steps. r?jib

Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision.

Comment 9

[Andreas Pehrson [:pehrsons]]

I've made some attempts at an automated test but to no avail. I'll push it to try to make sure but locally it hasn't reproduced.

I ended up with a mochitest (for loopback devices, thus the real device backends) that loads the testcase in an iframe and is done after a minute (should be enough to verify that it works).

gUM requests are working and all, but no crashes.

Comment 10

[Andreas Pehrson [:pehrsons]]

The good news is that try can repro the issue with my mochitest. The bad news that I still can't locally :-)

I'll just reduce the timeout, validate it, and put the patch up.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e15535acae84843e7171edefd446895e6b05856c

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1488832 - Add mochitest. r?jib

Comment 12

[Andreas Pehrson [:pehrsons]]

Still failing when waiting 10s: https://treeherder.mozilla.org/#/jobs?repo=try&revision=70f181c0b739a8868361dd730f6335509f674f0e

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 9010614 [details]
Bug 1488832 - Add mochitest. r?jib

Jan-Ivar Bruaroey [:jib] (needinfo? me) has approved the revision.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c808e4d6fb53
Add mochitest. r=jib
https://hg.mozilla.org/integration/autoland/rev/2ea7113db27c
Improve window validity checks after async gUM steps. r=jib
https://hg.mozilla.org/integration/autoland/rev/fe9563002b6f
Change init assert to runtime guard. r=jib

Comment 15

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/c808e4d6fb53
https://hg.mozilla.org/mozilla-central/rev/2ea7113db27c
https://hg.mozilla.org/mozilla-central/rev/fe9563002b6f