Closed
Bug 805451
Opened 12 years ago
Closed 12 years ago
Deadlock if calling mozGetUserMedia from a mozGetUserMedia callback
Categories
(Core :: WebRTC: Audio/Video, defect)
Core
WebRTC: Audio/Video
Tracking
()
VERIFIED
FIXED
mozilla19
Tracking | Status | |
---|---|---|
firefox18 | --- | unaffected |
firefox19 | --- | fixed |
People
(Reporter: jesup, Assigned: jesup)
Details
(Keywords: hang, Whiteboard: [getUserMedia] [blocking-gum+])
Attachments
(1 file, 4 obsolete files)
18.88 KB,
patch
|
derf
:
review+
|
Details | Diff | Splinter Review |
Need to release the AutoLock before calling the callback!
Comment 1•12 years ago
|
||
Smells like a blocker (even though triage hasn't happened for this bug yet).
Whiteboard: [getUserMedia] [blocking-gum+]
Assignee | ||
Comment 2•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #675152 -
Flags: review?(tterribe)
Comment 3•12 years ago
|
||
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•12 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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #675152 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #675200 -
Flags: review?(tterribe)
Assignee | ||
Comment 6•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #675200 -
Attachment is obsolete: true
Attachment #675200 -
Flags: review?(tterribe)
Assignee | ||
Comment 7•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #675205 -
Attachment is obsolete: true
Assignee | ||
Comment 8•12 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 9•12 years ago
|
||
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•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #675237 -
Attachment is obsolete: true
Assignee | ||
Comment 11•12 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 12•12 years ago
|
||
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•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
status-firefox18:
--- → unaffected
status-firefox19:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•12 years ago
|
Attachment #675260 -
Flags: review?(tterribe)
Updated•12 years ago
|
Flags: in-testsuite?
Comment 15•12 years ago
|
||
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.
Description
•