805451 - Deadlock if calling mozGetUserMedia from a mozGetUserMedia callback

Status: VERIFIED FIXED

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

Description

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

Need to release the AutoLock before calling the callback!

Comment 1

[Jason Smith [:jsmith]]

Smells like a blocker (even though triage hasn't happened for this bug yet).

Comment 2

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

Attachment: Release MediaManager lock before calling user callbacks

Comment 3

[Timothy B. Terriberry (:derf)]

Comment on attachment 675152 [details] [diff] [review]
Release MediaManager lock before calling user callbacks

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

r=me with a couple of changes.

::: dom/media/MediaManager.cpp
@@ +114,3 @@
>        }
>      }
> +    // Lock is released!

In order for the check above to work as documented, we need to ensure that no other thread removes the window after the above check. Right now that can only happen in the shutdown observer and in OnNavigation(). The former already asserts that it runs on the main thread, the same as this callback, but the latter does not. Please add an assertion to the latter, and document in _this_ function why we can assume the window will remain active after releasing the lock.

@@ +265,1 @@
>          }

Same as above (here you don't even have the "Lock is released" comment by way of explanation).

@@ +269,4 @@
>          return NS_OK;
>        }
>      }
> +

Gratuitous whitespace change.

Comment 4

[Eric Rescorla (:ekr)]

Comment on attachment 675152 [details] [diff] [review]
Release MediaManager lock before calling user callbacks

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

not that I am an expert but lgtm with derf's changes.

Comment 5

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

Attachment: Release MediaManager lock before calling user callbacks

Comment 6

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

Attachment: Release MediaManager lock before calling user callbacks

Comment 7

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

Attachment: Release MediaManager lock before calling user callbacks

Comment 8

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

Comment on attachment 675237 [details] [diff] [review]
Release MediaManager lock before calling user callbacks

Ok, I've almost totally neutered the lock (now only protecting the backend pointer, which is dynamically acquired off-main-thread - though with more analysis I might be able to prove it's only allocated and referenced from the mMediaThread.)

Cleaned up a bunch of code, and moved mMediaThread creation to MediaManager creation to remove a lot of locks and checks, and it gets created pretty much immediately anyways, so there's no use holding off.

ps: Ignore the "Lock is released!" in one spot; I'll remove it.

Comment 9

[Timothy B. Terriberry (:derf)]

Comment on attachment 675237 [details] [diff] [review]
Release MediaManager lock before calling user callbacks

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

r- for the double-checked lock.

I also think we should add something like nsDOMCameraManager::IsWindowStillActive(), instead of directly exposing the pointer in mActiveWindows via GetActiveWindows().

::: dom/media/MediaManager.cpp
@@ +925,1 @@
>    if (!mBackend) {

Double-checked locking doesn't work. See http://en.wikipedia.org/wiki/Double-checked_locking
This isn't perf-critical, so I don't think we should get fancy here.

@@ +956,5 @@
> +  for (uint32_t i = 0; i < length; i++) {
> +    nsRefPtr<GetUserMediaCallbackMediaStreamListener> listener =
> +      listeners->ElementAt(i);
> +    listener->Invalidate();
> +    listener = nullptr;

While you're here, I don't understand why this line is needed.

Comment 10

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

Attachment: Release MediaManager lock before calling user callbacks

Comment 11

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

Comment on attachment 675260 [details] [diff] [review]
Release MediaManager lock before calling user callbacks

Minor changes:
remove double-checked lock
Remove spurious listener = nullptr (pre-existing)
Switch to IsWindowStillActive() for checks
Make GetActiveWindows() private

Comment 12

[Timothy B. Terriberry (:derf)]

Comment on attachment 675260 [details] [diff] [review]
Release MediaManager lock before calling user callbacks

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

r=me

::: dom/media/MediaManager.cpp
@@ +432,5 @@
>    nsresult
>    Denied()
>    {
>      if (NS_IsMainThread()) {
> +      // Lock is released!  This is safe since we're on main-thread, and the

Don't forget to remove the "Lock is released!" part. There's no lock here.

@@ +917,5 @@
>  {
>    // Plugin backends as appropriate. The default engine also currently
>    // includes picture support for Android.
> +  // This IS called off main-thread.
> +  // Avoid locking for the common case

This comment is no longer accurate.

@@ +920,5 @@
> +  // This IS called off main-thread.
> +  // Avoid locking for the common case
> +  MutexAutoLock lock(mMutex);
> +
> +  // in theory it could exist now

Should probably kill this one as well (if not it should be properly capitalized and punctuated).

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/1e1c57c06adb

Comment 14

[Jason Smith [:jsmith]]

Verified on 10/26 build.

Comment 15

[Jason Smith [:jsmith]]

Added automation coverage for this in bug 822109's patch.