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)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
References
Details
Attachments
(4 files)
20.73 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
5.98 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
39.24 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
1.62 KB,
patch
|
jwwang
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•8 years ago
|
||
Assignee: nobody → amarchesini
Attachment #8927790 -
Flags: review?(jwwang)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8927791 -
Flags: review?(bugs)
Assignee | ||
Comment 3•8 years ago
|
||
Attachment #8927792 -
Flags: review?(jwwang)
Assignee | ||
Comment 4•8 years ago
|
||
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+
Updated•8 years ago
|
Priority: -- → P3
Updated•8 years ago
|
Attachment #8927793 -
Flags: review?(jwwang) → review+
Comment 6•8 years ago
|
||
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 7•8 years ago
|
||
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
Comment 9•8 years ago
|
||
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
Flags: needinfo?(amarchesini)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(amarchesini)
Comment 10•8 years ago
|
||
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
Comment 11•8 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•