Skip to content

Commit eedd5e2

Browse files
bakulfbakulf
committed
Bug 1416724 - part 1 - AbstractThread::Dispatch should propage errors if failing the dispatching of Runnables, r=jwwang
1 parent f24a75f commit eedd5e2

9 files changed

+56
-67
lines changed

‎xpcom/tests/gtest/TestMozPromise.cpp

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
#include "mozilla/TaskQueue.h"
1111
#include "mozilla/MozPromise.h"
12+
#include "mozilla/Unused.h"
1213

1314
#include "nsISupportsImpl.h"
1415
#include "mozilla/SharedThreadPool.h"
@@ -60,12 +61,11 @@ class DelayedResolveOrReject : public Runnable
6061

6162
if (--mIterations == 0) {
6263
mPromise->ResolveOrReject(mValue, __func__);
63-
} else {
64-
nsCOMPtr<nsIRunnable> r = this;
65-
mTaskQueue->Dispatch(r.forget());
64+
return NS_OK;
6665
}
6766

68-
return NS_OK;
67+
nsCOMPtr<nsIRunnable> r = this;
68+
return mTaskQueue->Dispatch(r.forget());
6969
}
7070

7171
void Cancel() {
@@ -87,7 +87,7 @@ void
8787
RunOnTaskQueue(TaskQueue* aQueue, FunctionType aFun)
8888
{
8989
nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction("RunOnTaskQueue", aFun);
90-
aQueue->Dispatch(r.forget());
90+
Unused << aQueue->Dispatch(r.forget());
9191
}
9292

9393
// std::function can't come soon enough. :-(
@@ -162,11 +162,11 @@ TEST(MozPromise, AsyncResolve)
162162
RefPtr<DelayedResolveOrReject> c = new DelayedResolveOrReject(queue, p, RRValue::MakeReject(32.0), 7);
163163

164164
nsCOMPtr<nsIRunnable> ref = a.get();
165-
queue->Dispatch(ref.forget());
165+
Unused << queue->Dispatch(ref.forget());
166166
ref = b.get();
167-
queue->Dispatch(ref.forget());
167+
Unused << queue->Dispatch(ref.forget());
168168
ref = c.get();
169-
queue->Dispatch(ref.forget());
169+
Unused << queue->Dispatch(ref.forget());
170170

171171
p->Then(queue, __func__, [queue, a, b, c] (int aResolveValue) -> void {
172172
EXPECT_EQ(aResolveValue, 42);
@@ -197,7 +197,7 @@ TEST(MozPromise, CompletionPromises)
197197
[queue] (int aVal) -> RefPtr<TestPromise> {
198198
RefPtr<TestPromise::Private> p = new TestPromise::Private(__func__);
199199
nsCOMPtr<nsIRunnable> resolver = new DelayedResolveOrReject(queue, p, RRValue::MakeResolve(aVal - 8), 10);
200-
queue->Dispatch(resolver.forget());
200+
Unused << queue->Dispatch(resolver.forget());
201201
return RefPtr<TestPromise>(p);
202202
},
203203
DO_FAIL)

‎xpcom/tests/gtest/TestStateWatching.cpp

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "mozilla/SharedThreadPool.h"
99
#include "mozilla/StateWatching.h"
1010
#include "mozilla/TaskQueue.h"
11+
#include "mozilla/Unused.h"
1112
#include "nsISupportsImpl.h"
1213
#include "VideoUtils.h"
1314

@@ -32,7 +33,7 @@ TEST(WatchManager, Shutdown)
3233
WatchManager<Foo> manager(p, queue);
3334
Watchable<bool> notifier(false, "notifier");
3435

35-
queue->Dispatch(NS_NewRunnableFunction(
36+
Unused << queue->Dispatch(NS_NewRunnableFunction(
3637
"TestStateWatching::WatchManager_Shutdown_Test::TestBody", [&]() {
3738
manager.Watch(notifier, &Foo::Notify);
3839
notifier = true; // Trigger the call to Foo::Notify().

‎xpcom/tests/gtest/TestTaskQueue.cpp

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
#include "gtest/gtest.h"
88
#include "mozilla/SharedThreadPool.h"
99
#include "mozilla/TaskQueue.h"
10+
#include "mozilla/Unused.h"
1011
#include "VideoUtils.h"
1112

1213
namespace TestTaskQueue {
@@ -29,15 +30,15 @@ TEST(TaskQueue, EventOrder)
2930

3031
// We expect task1 happens before task3.
3132
for (int i = 0; i < 10000; ++i) {
32-
tq1->Dispatch(
33+
Unused << tq1->Dispatch(
3334
NS_NewRunnableFunction(
3435
"TestTaskQueue::TaskQueue_EventOrder_Test::TestBody",
3536
[&]() {
36-
tq2->Dispatch(NS_NewRunnableFunction(
37+
Unused << tq2->Dispatch(NS_NewRunnableFunction(
3738
"TestTaskQueue::TaskQueue_EventOrder_Test::TestBody",
3839
[]() { // task0
3940
}));
40-
tq3->Dispatch(NS_NewRunnableFunction(
41+
Unused << tq3->Dispatch(NS_NewRunnableFunction(
4142
"TestTaskQueue::TaskQueue_EventOrder_Test::TestBody",
4243
[&]() { // task1
4344
EXPECT_EQ(1, ++counter);
@@ -46,10 +47,10 @@ TEST(TaskQueue, EventOrder)
4647
++sync;
4748
mon.Notify();
4849
}));
49-
tq2->Dispatch(NS_NewRunnableFunction(
50+
Unused << tq2->Dispatch(NS_NewRunnableFunction(
5051
"TestTaskQueue::TaskQueue_EventOrder_Test::TestBody",
5152
[&]() { // task2
52-
tq3->Dispatch(NS_NewRunnableFunction(
53+
Unused << tq3->Dispatch(NS_NewRunnableFunction(
5354
"TestTaskQueue::TaskQueue_EventOrder_Test::TestBody",
5455
[&]() { // task3
5556
EXPECT_EQ(0, --counter);
@@ -60,7 +61,6 @@ TEST(TaskQueue, EventOrder)
6061
}));
6162
}));
6263
}),
63-
AbstractThread::AssertDispatchSuccess,
6464
AbstractThread::TailDispatch);
6565

6666
// Ensure task1 and task3 are done before next loop.

‎xpcom/threads/AbstractThread.cpp

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -47,19 +47,15 @@ class EventTargetWrapper : public AbstractThread
4747
}
4848

4949
virtual nsresult Dispatch(already_AddRefed<nsIRunnable> aRunnable,
50-
DispatchFailureHandling aFailureHandling = AssertDispatchSuccess,
5150
DispatchReason aReason = NormalDispatch) override
5251
{
5352
AbstractThread* currentThread;
5453
if (aReason != TailDispatch && (currentThread = GetCurrent()) && RequiresTailDispatch(currentThread)) {
55-
currentThread->TailDispatcher().AddTask(this, Move(aRunnable), aFailureHandling);
56-
return NS_OK;
54+
return currentThread->TailDispatcher().AddTask(this, Move(aRunnable));
5755
}
5856

5957
RefPtr<nsIRunnable> runner(new Runner(this, Move(aRunnable), false /* already drained by TaskGroupRunnable */));
60-
nsresult rv = mTarget->Dispatch(runner.forget(), NS_DISPATCH_NORMAL);
61-
MOZ_DIAGNOSTIC_ASSERT(aFailureHandling == DontAssertDispatchSuccess || NS_SUCCEEDED(rv));
62-
return rv;
58+
return mTarget->Dispatch(runner.forget(), NS_DISPATCH_NORMAL);
6359
}
6460

6561
// Prevent a GCC warning about the other overload of Dispatch being hidden.
@@ -223,8 +219,7 @@ AbstractThread::DispatchFromScript(nsIRunnable* aEvent, uint32_t aFlags)
223219
NS_IMETHODIMP
224220
AbstractThread::Dispatch(already_AddRefed<nsIRunnable> aEvent, uint32_t aFlags)
225221
{
226-
Dispatch(Move(aEvent), DontAssertDispatchSuccess, NormalDispatch);
227-
return NS_OK;
222+
return Dispatch(Move(aEvent), NormalDispatch);
228223
}
229224

230225
NS_IMETHODIMP
@@ -234,12 +229,14 @@ AbstractThread::DelayedDispatch(already_AddRefed<nsIRunnable> aEvent,
234229
return NS_ERROR_NOT_IMPLEMENTED;
235230
}
236231

237-
void
232+
nsresult
238233
AbstractThread::TailDispatchTasksFor(AbstractThread* aThread)
239234
{
240235
if (MightHaveTailTasks()) {
241-
TailDispatcher().DispatchTasksFor(aThread);
236+
return TailDispatcher().DispatchTasksFor(aThread);
242237
}
238+
239+
return NS_OK;
243240
}
244241

245242
bool

‎xpcom/threads/AbstractThread.h

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -67,10 +67,8 @@ class AbstractThread : public nsISerialEventTarget
6767
NS_IMETHOD DispatchFromScript(nsIRunnable *event, uint32_t flags) override;
6868
NS_IMETHOD DelayedDispatch(already_AddRefed<nsIRunnable> event, uint32_t delay) override;
6969

70-
enum DispatchFailureHandling { AssertDispatchSuccess, DontAssertDispatchSuccess };
7170
enum DispatchReason { NormalDispatch, TailDispatch };
7271
virtual nsresult Dispatch(already_AddRefed<nsIRunnable> aRunnable,
73-
DispatchFailureHandling aHandling = AssertDispatchSuccess,
7472
DispatchReason aReason = NormalDispatch) = 0;
7573

7674
virtual bool IsCurrentThreadIn() = 0;
@@ -89,7 +87,7 @@ class AbstractThread : public nsISerialEventTarget
8987
// Helper functions for methods on the tail TasklDispatcher. These check
9088
// HasTailTasks to avoid allocating a TailDispatcher if it isn't
9189
// needed.
92-
void TailDispatchTasksFor(AbstractThread* aThread);
90+
nsresult TailDispatchTasksFor(AbstractThread* aThread);
9391
bool HasTailTasksFor(AbstractThread* aThread);
9492

9593
// Returns true if this supports the tail dispatcher.

‎xpcom/threads/StateMirroring.h

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -155,8 +155,7 @@ class Canonical
155155
mMirrors[i]->OwnerThread()->Dispatch(
156156
NewRunnableMethod("AbstractMirror::NotifyDisconnected",
157157
mMirrors[i],
158-
&AbstractMirror<T>::NotifyDisconnected),
159-
AbstractThread::DontAssertDispatchSuccess);
158+
&AbstractMirror<T>::NotifyDisconnected));
160159
}
161160
mMirrors.Clear();
162161
}
@@ -338,7 +337,7 @@ class Mirror
338337
aCanonical,
339338
&AbstractCanonical<T>::AddMirror,
340339
this);
341-
aCanonical->OwnerThread()->Dispatch(r.forget(), AbstractThread::DontAssertDispatchSuccess);
340+
aCanonical->OwnerThread()->Dispatch(r.forget());
342341
mCanonical = aCanonical;
343342
}
344343
public:
@@ -357,7 +356,7 @@ class Mirror
357356
mCanonical,
358357
&AbstractCanonical<T>::RemoveMirror,
359358
this);
360-
mCanonical->OwnerThread()->Dispatch(r.forget(), AbstractThread::DontAssertDispatchSuccess);
359+
mCanonical->OwnerThread()->Dispatch(r.forget());
361360
mCanonical = nullptr;
362361
}
363362

‎xpcom/threads/TaskDispatcher.h

Lines changed: 23 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,10 @@ class TaskDispatcher
5454

5555
// Regular tasks are dispatched asynchronously, and run after state change
5656
// tasks.
57-
virtual void AddTask(AbstractThread* aThread,
58-
already_AddRefed<nsIRunnable> aRunnable,
59-
AbstractThread::DispatchFailureHandling aFailureHandling = AbstractThread::AssertDispatchSuccess) = 0;
57+
virtual nsresult AddTask(AbstractThread* aThread,
58+
already_AddRefed<nsIRunnable> aRunnable) = 0;
6059

61-
virtual void DispatchTasksFor(AbstractThread* aThread) = 0;
60+
virtual nsresult DispatchTasksFor(AbstractThread* aThread) = 0;
6261
virtual bool HasTasksFor(AbstractThread* aThread) = 0;
6362
virtual void DrainDirectTasks() = 0;
6463
};
@@ -122,9 +121,8 @@ class AutoTaskDispatcher : public TaskDispatcher
122121
EnsureTaskGroup(aThread).mStateChangeTasks.AppendElement(r.forget());
123122
}
124123

125-
void AddTask(AbstractThread* aThread,
126-
already_AddRefed<nsIRunnable> aRunnable,
127-
AbstractThread::DispatchFailureHandling aFailureHandling) override
124+
nsresult AddTask(AbstractThread* aThread,
125+
already_AddRefed<nsIRunnable> aRunnable) override
128126
{
129127
nsCOMPtr<nsIRunnable> r = aRunnable;
130128
MOZ_RELEASE_ASSERT(r);
@@ -139,11 +137,7 @@ class AutoTaskDispatcher : public TaskDispatcher
139137
PerThreadTaskGroup& group = *mTaskGroups.LastElement();
140138
group.mRegularTasks.AppendElement(r.forget());
141139

142-
// The task group needs to assert dispatch success if any of the runnables
143-
// it's dispatching want to assert it.
144-
if (aFailureHandling == AbstractThread::AssertDispatchSuccess) {
145-
group.mFailureHandling = AbstractThread::AssertDispatchSuccess;
146-
}
140+
return NS_OK;
147141
}
148142

149143
bool HasTasksFor(AbstractThread* aThread) override
@@ -152,15 +146,27 @@ class AutoTaskDispatcher : public TaskDispatcher
152146
(aThread == AbstractThread::GetCurrent() && HaveDirectTasks());
153147
}
154148

155-
void DispatchTasksFor(AbstractThread* aThread) override
149+
nsresult DispatchTasksFor(AbstractThread* aThread) override
156150
{
151+
nsresult rv = NS_OK;
152+
157153
// Dispatch all groups that match |aThread|.
158154
for (size_t i = 0; i < mTaskGroups.Length(); ++i) {
159155
if (mTaskGroups[i]->mThread == aThread) {
160-
DispatchTaskGroup(Move(mTaskGroups[i]));
156+
nsresult rv2 = DispatchTaskGroup(Move(mTaskGroups[i]));
157+
158+
if (NS_WARN_IF(NS_FAILED(rv2)) && NS_SUCCEEDED(rv)) {
159+
// We should try our best to call DispatchTaskGroup() as much as
160+
// possible and return an error if any of DispatchTaskGroup() calls
161+
// failed.
162+
rv = rv2;
163+
}
164+
161165
mTaskGroups.RemoveElementAt(i--);
162166
}
163167
}
168+
169+
return rv;
164170
}
165171

166172
private:
@@ -169,7 +175,7 @@ class AutoTaskDispatcher : public TaskDispatcher
169175
{
170176
public:
171177
explicit PerThreadTaskGroup(AbstractThread* aThread)
172-
: mThread(aThread), mFailureHandling(AbstractThread::DontAssertDispatchSuccess)
178+
: mThread(aThread)
173179
{
174180
MOZ_COUNT_CTOR(PerThreadTaskGroup);
175181
}
@@ -179,7 +185,6 @@ class AutoTaskDispatcher : public TaskDispatcher
179185
RefPtr<AbstractThread> mThread;
180186
nsTArray<nsCOMPtr<nsIRunnable>> mStateChangeTasks;
181187
nsTArray<nsCOMPtr<nsIRunnable>> mRegularTasks;
182-
AbstractThread::DispatchFailureHandling mFailureHandling;
183188
};
184189

185190
class TaskGroupRunnable : public Runnable
@@ -250,15 +255,14 @@ class AutoTaskDispatcher : public TaskDispatcher
250255
return nullptr;
251256
}
252257

253-
void DispatchTaskGroup(UniquePtr<PerThreadTaskGroup> aGroup)
258+
nsresult DispatchTaskGroup(UniquePtr<PerThreadTaskGroup> aGroup)
254259
{
255260
RefPtr<AbstractThread> thread = aGroup->mThread;
256261

257-
AbstractThread::DispatchFailureHandling failureHandling = aGroup->mFailureHandling;
258262
AbstractThread::DispatchReason reason = mIsTailDispatcher ? AbstractThread::TailDispatch
259263
: AbstractThread::NormalDispatch;
260264
nsCOMPtr<nsIRunnable> r = new TaskGroupRunnable(Move(aGroup));
261-
thread->Dispatch(r.forget(), failureHandling, reason);
265+
return thread->Dispatch(r.forget(), reason);
262266
}
263267

264268
// Direct tasks. We use a Maybe<> because (a) this class is hot, (b)

‎xpcom/threads/TaskQueue.cpp

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,6 @@ class TaskQueue::EventTargetWrapper final : public nsISerialEventTarget
3939
nsCOMPtr<nsIRunnable> runnable = aEvent;
4040
MonitorAutoLock mon(mTaskQueue->mQueueMonitor);
4141
return mTaskQueue->DispatchLocked(/* passed by ref */runnable,
42-
DontAssertDispatchSuccess,
4342
NormalDispatch);
4443
}
4544

@@ -106,7 +105,6 @@ TaskQueue::TailDispatcher()
106105
// See Dispatch() in TaskQueue.h for more details.
107106
nsresult
108107
TaskQueue::DispatchLocked(nsCOMPtr<nsIRunnable>& aRunnable,
109-
DispatchFailureHandling aFailureHandling,
110108
DispatchReason aReason)
111109
{
112110
mQueueMonitor.AssertCurrentThreadOwns();
@@ -116,8 +114,7 @@ TaskQueue::DispatchLocked(nsCOMPtr<nsIRunnable>& aRunnable,
116114

117115
AbstractThread* currentThread;
118116
if (aReason != TailDispatch && (currentThread = GetCurrent()) && RequiresTailDispatch(currentThread)) {
119-
currentThread->TailDispatcher().AddTask(this, aRunnable.forget(), aFailureHandling);
120-
return NS_OK;
117+
return currentThread->TailDispatcher().AddTask(this, aRunnable.forget());
121118
}
122119

123120
mTasks.push(aRunnable.forget());

‎xpcom/threads/TaskQueue.h

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -61,20 +61,14 @@ class TaskQueue : public AbstractThread
6161

6262
TaskQueue* AsTaskQueue() override { return this; }
6363

64-
nsresult Dispatch(already_AddRefed<nsIRunnable> aRunnable,
65-
DispatchFailureHandling aFailureHandling = AssertDispatchSuccess,
66-
DispatchReason aReason = NormalDispatch) override
64+
MOZ_MUST_USE nsresult
65+
Dispatch(already_AddRefed<nsIRunnable> aRunnable,
66+
DispatchReason aReason = NormalDispatch) override
6767
{
6868
nsCOMPtr<nsIRunnable> r = aRunnable;
6969
{
7070
MonitorAutoLock mon(mQueueMonitor);
71-
nsresult rv = DispatchLocked(/* passed by ref */r, aFailureHandling, aReason);
72-
#if defined(DEBUG) || !defined(RELEASE_OR_BETA) || defined(EARLY_BETA_OR_EARLIER)
73-
if (NS_FAILED(rv) && aFailureHandling == AssertDispatchSuccess) {
74-
MOZ_CRASH_UNSAFE_PRINTF("%s: Dispatch failed. rv=%x", mName, uint32_t(rv));
75-
}
76-
#endif
77-
return rv;
71+
return DispatchLocked(/* passed by ref */r, aReason);
7872
}
7973
// If the ownership of |r| is not transferred in DispatchLocked() due to
8074
// dispatch failure, it will be deleted here outside the lock. We do so
@@ -121,7 +115,6 @@ class TaskQueue : public AbstractThread
121115
void AwaitIdleLocked();
122116

123117
nsresult DispatchLocked(nsCOMPtr<nsIRunnable>& aRunnable,
124-
DispatchFailureHandling aFailureHandling,
125118
DispatchReason aReason = NormalDispatch);
126119

127120
void MaybeResolveShutdown()

0 commit comments

Comments
 (0)