1416724 - Get rid of DispatchFailureHandling in AbstractThread::Dispatch

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

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.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - AbstractThread::Dispatch

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - MutableBlobStorage

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 3 - dom/media

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 4 - netUtil

Comment 5

[Olli Pettay [:smaug]]

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.

Comment 6

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

Comment 9

[Stefan Hindli [:stefan_hindli]]

Backed out 4 changesets (bug 1416724) for failing /builds/worker/workspace/build/src/dom/media/hls/HLSDemuxer.cpp:89:5 r=backout on a CLOSED TREE
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=45352aa4319debd6ad3445f2729db197ffc83eac&filter-classifiedState=unclassified&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=144862128
https://hg.mozilla.org/integration/mozilla-inbound/rev/7613f8e20ad525e183b9e5a237cb5725fbe3f630

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/430e5be1f7bb
https://hg.mozilla.org/mozilla-central/rev/64c403042270
https://hg.mozilla.org/mozilla-central/rev/ddbbd455f058
https://hg.mozilla.org/mozilla-central/rev/05c7a8ba5632