Closed Bug 1416724 Opened 8 years ago Closed 8 years ago

Get rid of DispatchFailureHandling in AbstractThread::Dispatch

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(4 files)

In general, a thread can fail the dispatching of runnables when shutting down. It's probably better to expose this failure and let the caller deal with it.
Assignee: nobody → amarchesini
Attachment #8927790 - Flags: review?(jwwang)
Attachment #8927791 - Flags: review?(bugs)
Attachment #8927792 - Flags: review?(jwwang)
Attached patch part 4 - netUtilSplinter Review
Attachment #8927793 - Flags: review?(jwwang)
Hmm, do we really need to remove support for asserting about success? That can be rather useful for debugging and implementation work in some cases. In general, sure, error result should be propagated and handled.
Priority: -- → P3
Attachment #8927793 - Flags: review?(jwwang) → review+
Comment on attachment 8927792 [details] [diff] [review] part 3 - dom/media Review of attachment 8927792 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/media/encoder/MediaEncoder.cpp @@ +469,5 @@ > "mozilla::VideoTrackEncoder::RegisterListener", > mVideoEncoder, &VideoTrackEncoder::RegisterListener, mEncoderListener)); > } > + > + MOZ_DIAGNOSTIC_ASSERT(NS_SUCCEEDED(rv)); We should have 2 assertions in |if (mAudioEncoder)| and |if (mVideoEncoder)| respectively.
Attachment #8927792 - Flags: review?(jwwang) → review+
Comment on attachment 8927790 [details] [diff] [review] part 1 - AbstractThread::Dispatch Review of attachment 8927790 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/threads/TaskDispatcher.h @@ +150,5 @@ > { > // Dispatch all groups that match |aThread|. > for (size_t i = 0; i < mTaskGroups.Length(); ++i) { > if (mTaskGroups[i]->mThread == aThread) { > + nsresult rv = DispatchTaskGroup(Move(mTaskGroups[i])); We should try our best to call DispatchTaskGroup() as much as possible. And return an error if any of DispatchTaskGroup() calls failed. I think this is part of the reason of this dispatch assertion mechanism since there is no way to handle dispatch failures when tail dispatching is enabled.
Attachment #8927790 - Flags: review?(jwwang) → review+
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/35a50167485d part 1 - AbstractThread::Dispatch should propage errors if failing the dispatching of Runnables, r=jwwang https://hg.mozilla.org/integration/mozilla-inbound/rev/bc854c315ec8 part 2 - MutableBlobStorage should check the return value of TaskQueue::Dispatch(), r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/01d1e5263bcd part 3 - AbstractThread::Dispatch return value check in dom/media, r=jwwang https://hg.mozilla.org/integration/mozilla-inbound/rev/45352aa4319d part 4 - AbstractThread::Dispatch return value check in netwerk/base, r=smaug
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/430e5be1f7bb part 1 - AbstractThread::Dispatch should propage errors if failing the dispatching of Runnables, r=jwwang https://hg.mozilla.org/integration/mozilla-inbound/rev/64c403042270 part 2 - MutableBlobStorage should check the return value of TaskQueue::Dispatch(), r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/ddbbd455f058 part 3 - AbstractThread::Dispatch return value check in dom/media, r=jwwang https://hg.mozilla.org/integration/mozilla-inbound/rev/05c7a8ba5632 part 4 - AbstractThread::Dispatch return value check in netwerk/base, r=smaug
Depends on: 1425996
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: