Closed Bug 1225731 Opened 4 years ago Closed 3 years ago

Early bailout from TaskQueue::DispatchLocked()

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file, 1 obsolete file)

It is easier to know who the caller is when dispatch fails without waiting for the tail dispatch phase where it is hard to know who the original caller is.
Bug 1225731 - Early bailout from TaskQueue::DispatchLocked().
Attachment #8689406 - Flags: review?(nfroyd)
Assignee: nobody → jwwang
Comment on attachment 8689406 [details]
MozReview Request: Bug 1225731 - Early bailout from TaskQueue::DispatchLocked().

https://reviewboard.mozilla.org/r/25627/#review23111

::: xpcom/threads/TaskQueue.cpp
(Diff revision 1)
> -  if (aReason != TailDispatch && (currentThread = GetCurrent()) && RequiresTailDispatch(currentThread)) {
> -    currentThread->TailDispatcher().AddTask(this, r.forget(), aFailureHandling);
> -    return NS_OK;

As I understand your explaination, the case we're trying to fix goes something like this:

1. DispatchLocked some thing.
2. Discover that it can be tail dispatched.
3. Do the tail dispatch.
4. Once it actually gets handled in tail dispatch, we discover that there's some sort of error...
5. ...at which point we have no clue where it got dispatched from in the first place?

And then this patch modifies things to:

1. DispatchLocked some thing
2. Discover that we can't dispatch things at all (for whatever reason).
3. Fail, but with an intelligble dispatch stack.

Is that correct?

How do we know that the task will definitely not be dispatchable in both cases?  I assume that the mIsShutdown flag, once flipped to indicate shutdown, isn't going to be flipped back, but the mIsFlushing flag certainly looks like it can go back and forth.  So before this change, we might have tail dispatched something (while we were flushing) which would (presumably) succeed when it actually goes to run (?).  But after this change, we would fail to dispatch that thing, because we were in the middle of flushing.  That doesn't seem like a good change.
Attachment #8689406 - Flags: review?(nfroyd)
ni? bholley because he originally wrote this and might have intelligent things to say about comment 3.  I'm also realizing that I didn't ask for nearly enough comments about this code. :)
Flags: needinfo?(bobbyholley)
(In reply to Nathan Froyd [:froydnj] from comment #3)
> but the mIsFlushing flag certainly
> looks like it can go back and forth.  So before this change, we might have
> tail dispatched something (while we were flushing) which would (presumably)
> succeed when it actually goes to run (?).

No, tail dispatch won't succeed even without this patch if we are flushing.

Tail dispatching also calls TaskQueue::DispatchLocked() with aReason==TailDispatch. So dispatch will fail if mIsFlushing or mIsShutdown is true.
(In reply to Nathan Froyd [:froydnj] (high latency through 29 November) from comment #3)
> How do we know that the task will definitely not be dispatchable in both
> cases?  I assume that the mIsShutdown flag, once flipped to indicate
> shutdown, isn't going to be flipped back

Correct.

, but the mIsFlushing flag certainly
> looks like it can go back and forth.  So before this change, we might have
> tail dispatched something (while we were flushing) which would (presumably)
> succeed when it actually goes to run (?).

Fair. But flushing is deprecated, and only used in the FlushableTaskQueue subclass, which is only used for PDMs and is supposed to go away. I think we could actually assert that tail dispatch and Flushable* are never enabled for the same task queue. Or if not, we could MOZ_DIAGNOSTIC_ASSERT against the case you're concerned about here.
Flags: needinfo?(bobbyholley)
(In reply to Nathan Froyd [:froydnj] from comment #3)
> How do we know that the task will definitely not be dispatchable in both
> cases?
The dispatch will be considered success only when it passes both cases. As Murphy's law said, if it could fail, it will fail.

> I assume that the mIsShutdown flag, once flipped to indicate
> shutdown, isn't going to be flipped back, but the mIsFlushing flag certainly
> looks like it can go back and forth.  So before this change, we might have
> tail dispatched something (while we were flushing) which would (presumably)
> succeed when it actually goes to run (?).  But after this change, we would
> fail to dispatch that thing, because we were in the middle of flushing. 
> That doesn't seem like a good change.
If we need to handle the failure case, we will always need to handle that. No matter the chance is lower (without the patch) or higher (with the patch). So I think there is still merit in this patch which makes debugging easier despite it might lower the chance of success in the flushing case.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Depends on: 1316529
(In reply to Nathan Froyd [:froydnj] from comment #3)
> How do we know that the task will definitely not be dispatchable in both
> cases?  I assume that the mIsShutdown flag, once flipped to indicate
> shutdown, isn't going to be flipped back, but the mIsFlushing flag certainly
> looks like it can go back and forth.  So before this change, we might have
> tail dispatched something (while we were flushing) which would (presumably)
> succeed when it actually goes to run (?).  But after this change, we would
> fail to dispatch that thing, because we were in the middle of flushing. 
> That doesn't seem like a good change.

Bug 1316529 will remove mIsFlushing so this is no longer a concern.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Attachment #8689406 - Attachment is obsolete: true
Comment on attachment 8809300 [details]
Bug 1225731 - Early bailout from TaskQueue::DispatchLocked().

https://reviewboard.mozilla.org/r/91886/#review92018

Thank you!
Attachment #8809300 - Flags: review?(nfroyd) → review+
Thanks!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bf09a2e3fb6b
Early bailout from TaskQueue::DispatchLocked(). r=froydnj
https://hg.mozilla.org/mozilla-central/rev/bf09a2e3fb6b
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.