Closed Bug 1416724 Opened 2 years ago Closed 2 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.
Attachment #8927791 - Flags: review?(bugs) → review+
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.