Deadlock if calling mozGetUserMedia from a mozGetUserMedia callback

VERIFIED FIXED in Firefox 19

Status

()

--
critical
VERIFIED FIXED
7 years ago
6 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

({hang})

Trunk
mozilla19
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 unaffected, firefox19 fixed)

Details

(Whiteboard: [getUserMedia] [blocking-gum+])

Attachments

(1 attachment, 4 obsolete attachments)

Need to release the AutoLock before calling the callback!
Keywords: hang
Smells like a blocker (even though triage hasn't happened for this bug yet).
Whiteboard: [getUserMedia] [blocking-gum+]
Attachment #675152 - Flags: review?(tterribe)
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.
Attachment #675152 - Flags: review?(tterribe) → review+
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.
Attachment #675152 - Flags: review+
Attachment #675152 - Attachment is obsolete: true
Attachment #675200 - Flags: review?(tterribe)
Attachment #675200 - Attachment is obsolete: true
Attachment #675200 - Flags: review?(tterribe)
Attachment #675205 - Attachment is obsolete: true
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.
Attachment #675237 - Flags: review?(tterribe)
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.
Attachment #675237 - Flags: review?(tterribe) → review-
Attachment #675237 - Attachment is obsolete: true
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
Attachment #675260 - Flags: review?(tterribe)
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).
Attachment #675260 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/1e1c57c06adb
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
status-firefox18: --- → unaffected
status-firefox19: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Keywords: verifyme
Verified on 10/26 build.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Attachment #675260 - Flags: review?(tterribe)
Flags: in-testsuite?
Added automation coverage for this in bug 822109's patch.
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.