Closed Bug 1196632 Opened 6 years ago Closed 6 years ago

pass ownership of runnables to NS_DispatchToMainThread in MSG

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: karlt, Assigned: karlt)

References

Details

Attachments

(2 files)

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: nobody → karlt
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+
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.
https://hg.mozilla.org/mozilla-central/rev/b3ef0a1e1f6c
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
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.
Flags: needinfo?(karlt)
(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
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 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+
Filed Bug 1198100.
Flags: needinfo?(karlt)
You need to log in before you can comment on or make changes to this bug.