Closed
Bug 1257063
Opened 8 years ago
Closed 8 years ago
Don't destruct the runnable inside the lock when TaskQueue::Dispatch fails
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jwwang, Assigned: jwwang)
References
Details
Attachments
(1 file, 1 obsolete file)
https://hg.mozilla.org/mozilla-central/file/5e14887312d4523ab59c3f6c6c94a679cf42b496/xpcom/threads/TaskQueue.h#l47 When dispatch fails, the destructor of the runnable will be run inside the lock of TaskQueue which is bad because the destructor might try to acquire other locks and result in potential deadlock.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40331/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40331/
Assignee | ||
Updated•8 years ago
|
Attachment #8731043 -
Flags: review?(bobbyholley)
Comment 2•8 years ago
|
||
Comment on attachment 8731043 [details] MozReview Request: Bug 1257063 - Don't destruct the runnable inside the lock when TaskQueue::Dispatch fails. https://reviewboard.mozilla.org/r/40331/#review36879 ::: xpcom/threads/TaskQueue.h:55 (Diff revision 1) > - Unused << rv; > + Unused << rv; > - } > + } > + // The destructor of the runnable might access TaskQueue. In order to > + // prevent deadlocks, we need to release the runnable outside the lock > + // if the ownership is not transferred due to dispatch failure. > + RefPtr<nsIRunnable> releaseMe = aRunnable; This doesn't make sense. We unconditionally move aRunnable above, so it will always be null by the time we reach here. ::: xpcom/threads/TaskQueue.h:90 (Diff revision 1) > // mQueueMonitor must be held. > void AwaitIdleLocked(); > > enum DispatchMode { AbortIfFlushing, IgnoreFlushing }; > > - nsresult DispatchLocked(already_AddRefed<nsIRunnable> aRunnable, DispatchMode aMode, > + nsresult DispatchLocked(already_AddRefed<nsIRunnable>&& aRunnable, Per discussion at bug 1247972 comment 19, I think we should avoid passing already_AddRefed<T>&& in favor of already_AddRefed<T>. We can Move() either of them. ::: xpcom/threads/TaskQueue.cpp:43 (Diff revision 1) > MOZ_ASSERT(mTailDispatcher); > return *mTailDispatcher; > } > > nsresult > -TaskQueue::DispatchLocked(already_AddRefed<nsIRunnable> aRunnable, > +TaskQueue::DispatchLocked(already_AddRefed<nsIRunnable>&& aRunnable, Same here. Perhaps your intention was to pass already_AddRefed<T>&? Passing && requires the caller to Move(), which means that ownership of the reference is unconditionally passed to the callee. already_AddRefed<T>& could work, but it seems a bit confusing. I guess it might work with heavy comments. Passing already_AddRefed<T>* might be better, since it would make it more obvious what's going on, since the caller would need to pass with & and the callee would need to dereference with *.
Attachment #8731043 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 3•8 years ago
|
||
https://reviewboard.mozilla.org/r/40331/#review36879 > This doesn't make sense. We unconditionally move aRunnable above, so it will always be null by the time we reach here. I got the idea from https://hg.mozilla.org/mozilla-central/file/5e14887312d4523ab59c3f6c6c94a679cf42b496/mfbt/UniquePtr.h#l181. When |already_AddRefed<nsIRunnable>&&| is used, it doesn't necessarily transfer the ownship as |already_AddRefed<nsIRunnable>| does. That is exactly what I need that no ownship transfer when dispatch fails so the runnable can be deleted outside the lock.
Comment 4•8 years ago
|
||
(In reply to JW Wang [:jwwang] from comment #3) > https://reviewboard.mozilla.org/r/40331/#review36879 > > > This doesn't make sense. We unconditionally move aRunnable above, so it will always be null by the time we reach here. > > I got the idea from > https://hg.mozilla.org/mozilla-central/file/ > 5e14887312d4523ab59c3f6c6c94a679cf42b496/mfbt/UniquePtr.h#l181. > When |already_AddRefed<nsIRunnable>&&| is used, it doesn't necessarily > transfer the ownship as |already_AddRefed<nsIRunnable>| does. > That is exactly what I need that no ownship transfer when dispatch fails so > the runnable can be deleted outside the lock. Hm, I guess you're right, since Move() doesn't necessarily invoke the Move constructor. It still seems confusing, but maybe it's just me. Let's see what Gerald thinks.
Flags: needinfo?(gsquelart)
I see how the patch works, it does the job... However the actual use of a Move()'d-from object feels wrong. Sutter (and/or others experts?) argue that moved-from objects should only be destroyed, or could be totally overwritten through an assignment, but nothing else; I believe the STL containers are designed with that philosophy. But I see that it is necessary in your patch because of the 'assert(!mRawPtr)' in ~already_AddRefed(), which would trigger if the nsIRunnable was not stolen by a task dispatch. I think it would be more elegant (or at least less tricky) to just take explicit shared ownership of aRunnable by RefPtr in Dispatch() *before* taking the lock in a block, and pass that RefPtr by reference or pointer to DispatchLocked(), which would do a forget() on it in the case of a successful task dispatch -- just like in the original code, we're just basically moving the nsCOMPtr into Dispatch(). If the task dispatch doesn't happen, the RefPtr in Dispatch() will be destroyed with its nsIRunnable, outside of the lock block. (Thought exercise only, I might have missed something that prevents this solution!)
Flags: needinfo?(gsquelart)
Assignee | ||
Comment 6•8 years ago
|
||
Thanks for the advice! I will try to roll a patch according to your suggestion. On the other hand, if https://hg.mozilla.org/mozilla-central/file/5e14887312d4523ab59c3f6c6c94a679cf42b496/mfbt/UniquePtr.h#l181 is not the right convention to deal with conditional transfer of ownership, we should establish a convention for code writers and reviewers.
JW: Good point too! Maybe I'm a dinosaur who's scared of reusing potentially-moved-from objects. :-) Jeff, you wrote UniquePtr and that comment about conditional transfer through UniquePtr&&. Do you think we can safely apply this technique to already_AddRefed as well? Please see patch. And what do you think about using UniquePtr/already_AddRef after a Move()? As JW wrote, we should probably establish and document a convention around this. (Another forum might be more appropriate for this discussion?)
Flags: needinfo?(jwalden+bmo)
Comment 8•8 years ago
|
||
I agree with Gerald that passing an nsRefPtr by reference is probably more understandable here. Maybe start a thread on dev-platform about the && convention?
Assignee | ||
Comment 9•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/40963/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/40963/
Assignee | ||
Updated•8 years ago
|
Attachment #8731043 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jwwang
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8731977 [details] MozReview Request: Bug 1257063 - Don't destruct the runnable inside the lock when TaskQueue::Dispatch fails. Per comment 8. Discussion is here: https://groups.google.com/forum/#!topic/mozilla.dev.platform/VLtl2yL_TlA
Attachment #8731977 -
Flags: review?(bobbyholley)
Comment 11•8 years ago
|
||
Comment on attachment 8731977 [details] MozReview Request: Bug 1257063 - Don't destruct the runnable inside the lock when TaskQueue::Dispatch fails. https://reviewboard.mozilla.org/r/40963/#review37515 r=me with those comments added. ::: dom/media/FlushableTaskQueue.cpp:28 (Diff revision 1) > + nsCOMPtr<nsIRunnable> r = aRunnable; > + { > - MonitorAutoLock mon(mQueueMonitor); > + MonitorAutoLock mon(mQueueMonitor); > - AutoSetFlushing autoFlush(this); > + AutoSetFlushing autoFlush(this); > - FlushLocked(); > + FlushLocked(); > - nsCOMPtr<nsIRunnable> r = dont_AddRef(aRunnable.take()); > + nsresult rv = DispatchLocked(r, IgnoreFlushing, AssertDispatchSuccess); Add a comment like: DispatchLocked(/* passed by ref */ r, ... ::: xpcom/threads/TaskQueue.h:49 (Diff revision 1) > DispatchReason aReason = NormalDispatch) override > { > + nsCOMPtr<nsIRunnable> r = aRunnable; > + { > - MonitorAutoLock mon(mQueueMonitor); > + MonitorAutoLock mon(mQueueMonitor); > - nsresult rv = DispatchLocked(Move(aRunnable), AbortIfFlushing, aFailureHandling, aReason); > + nsresult rv = DispatchLocked(r, AbortIfFlushing, aFailureHandling, aReason); same here. ::: xpcom/threads/TaskQueue.h:53 (Diff revision 1) > - MonitorAutoLock mon(mQueueMonitor); > + MonitorAutoLock mon(mQueueMonitor); > - nsresult rv = DispatchLocked(Move(aRunnable), AbortIfFlushing, aFailureHandling, aReason); > + nsresult rv = DispatchLocked(r, AbortIfFlushing, aFailureHandling, aReason); > - MOZ_DIAGNOSTIC_ASSERT(aFailureHandling == DontAssertDispatchSuccess || NS_SUCCEEDED(rv)); > + MOZ_DIAGNOSTIC_ASSERT(aFailureHandling == DontAssertDispatchSuccess || NS_SUCCEEDED(rv)); > - Unused << rv; > + Unused << rv; > - } > + } > + // If the ownership of |r| is not transferred in DispatchLocked() due to Duplicate this comment in FlushAndDispatch? ::: xpcom/threads/TaskQueue.cpp:43 (Diff revision 1) > MOZ_ASSERT(mTailDispatcher); > return *mTailDispatcher; > } > > nsresult > -TaskQueue::DispatchLocked(already_AddRefed<nsIRunnable> aRunnable, > +TaskQueue::DispatchLocked(nsCOMPtr<nsIRunnable>& aRunnable, I might add a comment above this method as well that we take the runnable by ref such that the failure cases don't cause us to release it.
Attachment #8731977 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 12•8 years ago
|
||
Thanks! I will address the nits.
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/26f9c1949e1c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Flags: needinfo?(jwalden+bmo)
You need to log in
before you can comment on or make changes to this bug.
Description
•