1029401 - crash in mozilla::MediaEngineTabVideoSource::StopRunnable

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Gian-Carlo Pascutto [:gcp]]

https://bugzilla.mozilla.org/show_bug.cgi?id=1018928#c16

Android 2.3 only, though there's no reason to think it's specific to that
https://tbpl.mozilla.org/php/getParsedLog.php?id=42304741&tree=Try&full=1#error0

The crashing line is:
  mVideoSource->mTabSource->NotifyStreamStop(mVideoSource->mWindow);

Other weird stuff:
  if (privateDOMWindow && mVideoSource && privateDOMWindow->GetChromeEventHandler()) {
    privateDOMWindow->GetChromeEventHandler()->RemoveEventListener(NS_LITERAL_STRING("MozAfterPaint"), mVideoSource, false);
  }

  if (mVideoSource->mTimer) {
    mVideoSource->mTimer->Cancel();
    mVideoSource->mTimer = nullptr;
  }

Note how the first line checks if mVideoSource != nullptr and the second part just dereferences it without a check.

Maybe related to bug 964754.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

The bug is 100% reproducible on try:
https://tbpl.mozilla.org/?tree=Try&rev=3522953e529c

I didn't manage to reproduce it on my local Android 2.3 device + Robocop, though.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

Some roundtrips with the tryserver confirm that mVideoSource->mTabSource is a nullptr. 

https://tbpl.mozilla.org/php/getParsedLog.php?id=42471931&tree=Try&full=1#error0

If I'm going to guess, this looks like a classic case of the Runnable to shut down only being activated after the object destructor has already finished.

Comment 3

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Fix object ownership.

Investigating who owns the MediaEngineTabVideoSource, I see it's on the aVSources thing in MediaEngineWebRTC, which is curiously different from all other engines which are permanently owned by MediaEngineWebRTC. So this changes it to also own the TabVideoSource, which handily avoids reinitializing it every time it's used. This does mean the window lookup has to be moved around.

I'm not positively sure all of that is correct, but after applying the patch manual testing of Tab Streaming seem to show it still works, and it makes my new Android WebRTC tests go green on try:
https://tbpl.mozilla.org/?tree=Try&pusher=[e-mail]

Comment 4

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e92f1571ab9d

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Bah, I missed that the final try run had this go orange on 2.3 anyway:
https://tbpl.mozilla.org/?tree=Try&rev=5b198b28880b

So this patch is not correct/incomplete.

Comment 6

[Carsten Book [:Tomcat]]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #5)
> Bah, I missed that the final try run had this go orange on 2.3 anyway:
> https://tbpl.mozilla.org/?tree=Try&rev=5b198b28880b
> 
> So this patch is not correct/incomplete.

yeah sorry had to this out for testfailure like https://tbpl.mozilla.org/php/getParsedLog.php?id=42620970&tree=Mozilla-Inbound

Comment 7

[Gian-Carlo Pascutto [:gcp]]

I put some more debugging/tracing stuff and fired off a few try runs:
https://hg.mozilla.org/try/rev/0be49eb27ef2

04:05:42     INFO -  06-30 03:39:26.379 I/Gecko   (  798): [798] ###!!! ABORT: VideoSource was never started: file /builds/slave/try-and-0000000000000000000000/build/content/media/webrtc/MediaEngineTabVideoSource.cpp, line 81
04:05:42     INFO -  06-30 03:39:26.412 E/Gecko   (  798): mozalloc_abort: [798] ###!!! ABORT: VideoSource was never started: file /builds/slave/try-and-0000000000000000000000/build/content/media/webrtc/MediaEngineTabVideoSource.cpp, line 81

So the problem is that the StopRunnable can be called without the StartRunnable having been run (and it's not a use-after-free problem). That means we can just bail from the StopRunnable for an easy fix.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. Check if the runnable was initialized

See previous comments. Not sure why this ends up being needed, but it's extra safety and a small code cleanup, so why not.

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8448002 [details] [diff] [review]
Patch 1. Check if the runnable was initialized

Review of attachment 8448002 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/webrtc/MediaEngineTabVideoSource.cpp
@@ +51,5 @@
>  
>  nsresult
>  MediaEngineTabVideoSource::StopRunnable::Run()
>  {
> +  if (!mVideoSource || !mVideoSource->mWindow)

seems like we should fall through to cancel the timer and notify the stream (even with null).

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8448002 [details] [diff] [review]
Patch 1. Check if the runnable was initialized

Review of attachment 8448002 [details] [diff] [review]:
-----------------------------------------------------------------

Having now read your previous comment, I'd prefer we do this check before dispatching the runnable in the first place.

It is possible to not dispatch the start runnable if we don't get a window from the tab source http://mxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineTabVideoSource.cpp#100

We should be able to null check mWindow here http://mxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineTabVideoSource.cpp#100

Comment 11

[Gian-Carlo Pascutto [:gcp]]

Attachment: Patch 1. v2 Check if Window was obtained

Null checking in the place you indicated is already done: it's exactly what's causing this bug because it bails before the initialization finishes, so thanks for pointing that out :)

This moves the null check before the StopRunnable is created.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9204ad8b61a

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e9204ad8b61a