Deadlock if calling mozGetUserMedia from a mozGetUserMedia callback

VERIFIED FIXED in Firefox 19

Status

()

--
critical
VERIFIED FIXED
6 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)

(Assignee)

Description

6 years ago
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+]
(Assignee)

Comment 2

6 years ago
Created attachment 675152 [details] [diff] [review]
Release MediaManager lock before calling user callbacks
(Assignee)

Updated

6 years ago
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 4

6 years ago
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+
(Assignee)

Comment 5

6 years ago
Created attachment 675200 [details] [diff] [review]
Release MediaManager lock before calling user callbacks
(Assignee)

Updated

6 years ago
Attachment #675152 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #675200 - Flags: review?(tterribe)
(Assignee)

Comment 6

6 years ago
Created attachment 675205 [details] [diff] [review]
Release MediaManager lock before calling user callbacks
(Assignee)

Updated

6 years ago
Attachment #675200 - Attachment is obsolete: true
Attachment #675200 - Flags: review?(tterribe)
(Assignee)

Comment 7

6 years ago
Created attachment 675237 [details] [diff] [review]
Release MediaManager lock before calling user callbacks
(Assignee)

Updated

6 years ago
Attachment #675205 - Attachment is obsolete: true
(Assignee)

Comment 8

6 years ago
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-
(Assignee)

Comment 10

6 years ago
Created attachment 675260 [details] [diff] [review]
Release MediaManager lock before calling user callbacks
(Assignee)

Updated

6 years ago
Attachment #675237 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
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+
(Assignee)

Comment 13

6 years ago
https://hg.mozilla.org/mozilla-central/rev/1e1c57c06adb
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
status-firefox18: --- → unaffected
status-firefox19: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Updated

6 years ago
Keywords: verifyme
Verified on 10/26 build.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Attachment #675260 - Flags: review?(tterribe)

Updated

6 years ago
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.