Closed
Bug 1196632
Opened 9 years ago
Closed 9 years ago
pass ownership of runnables to NS_DispatchToMainThread in MSG
Categories
(Core :: Audio/Video: MediaStreamGraph, defect)
Core
Audio/Video: MediaStreamGraph
Tracking
()
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: karlt, Assigned: karlt)
References
Details
Attachments
(2 files)
5.21 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
3.41 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
The already_AddRefed<nsIRunnable>&& overload saves some ref-counting and provides an assertion if called too late in shutdown. The main thread is only closed to events after all threads have shut down and the main thread event queue is closed, so I wouldn't expect to hit this assertion. The exception might be if event dispatch is triggered from object destruction during shutdown. If that is happening, then I'm hoping some stack traces for the assertions might provide clues re bug 1193922.
Assignee | ||
Comment 1•9 years ago
|
||
Attachment #8650283 -
Flags: review?(padenot)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → karlt
Comment 2•9 years ago
|
||
Comment on attachment 8650283 [details] [diff] [review] pass ownership of runnables to NS_DispatchToMainThread Review of attachment 8650283 [details] [diff] [review]: ----------------------------------------------------------------- r+, but almost all of these can be simplified further by using DispatchToMainThread(do_AddRef(new ....)); and getting rid of the temporary com/refptr. r+ if you go that route also
Attachment #8650283 -
Flags: review?(padenot) → review+
Assignee | ||
Comment 3•9 years ago
|
||
Thanks, Randell. I expect I will find do_AddRef useful at some point but I don't think such code would fit on one line in any of these situations. Maybe I just haven't got used to do_AddRef yet, but even if some did fit on one line I'd prefer consistency and I like the construction clearly standing out on its own.
Comment 5•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b3ef0a1e1f6c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Comment 6•9 years ago
|
||
Comment on attachment 8650283 [details] [diff] [review] pass ownership of runnables to NS_DispatchToMainThread Review of attachment 8650283 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/MediaStreamGraph.cpp @@ +2409,5 @@ > nsRefPtr<MediaStream> mStream; > }; > > nsRefPtr<nsRunnable> runnable = new NotifyRunnable(this); > + if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(runnable.forget())))) { This is wrong. The statement below assigns the runnable to a class field. The runnable must not forget at this point.
Updated•9 years ago
|
Flags: needinfo?(karlt)
Comment 7•9 years ago
|
||
(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6) > ::: dom/media/MediaStreamGraph.cpp > @@ +2409,5 @@ > > nsRefPtr<MediaStream> mStream; > > }; > > > > nsRefPtr<nsRunnable> runnable = new NotifyRunnable(this); > > + if (NS_WARN_IF(NS_FAILED(NS_DispatchToMainThread(runnable.forget())))) { > > This is wrong. The statement below assigns the runnable to a class field. > The runnable must not forget at this point. Good catch! Please land a followup to remove .forget() ASAP, r=jesup
Assignee | ||
Comment 8•9 years ago
|
||
Thanks for pointing this out, Xidorn. The failure to set mNotificationMainThreadRunnable is actually harmless. All callers of AddMainThreadListener make the call before the stream is even in the graph, well before mFinishedNotificationSent is set. If AddMainThreadListener() were called multiple times after mFinishedNotificationSent is set then we'd get some extra NotifyRunnables but NotifyMainThreadListeners() clears mMainThreadListeners anyway so we still get only one notification per listener. mNotificationMainThreadRunnable is an unnecessary optimization, so better not to add storage to every MediaStream.
Attachment #8651561 -
Flags: review?(amarchesini)
Comment 9•9 years ago
|
||
Comment on attachment 8651561 [details] [diff] [review] remove mNotificationMainThreadRunnable Review of attachment 8651561 [details] [diff] [review]: ----------------------------------------------------------------- You should file a separate bug for this.
Attachment #8651561 -
Flags: review?(amarchesini) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•