Closed Bug 1269963 Opened 4 years ago Closed 4 years ago

Add a SyncRunnable::DispatchToThread() overload for AbstractThread

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(1 file)

As AbstractThread is also a citizen of xpcom, it deserves a place in SyncRunnable.
Attachment #8748540 - Flags: review?(bobbyholley)
Blocks: 1270039
Blocks: 1270350
Attachment #8748540 - Flags: review?(bobbyholley) → review+
Comment on attachment 8748540 [details]
MozReview Request: Bug 1269963 - Add a SyncRunnable::DispatchToThread() overload for AbstractThread. r=bobbyholley.

https://reviewboard.mozilla.org/r/50381/#review47521

r=me with that fixed

::: xpcom/threads/AbstractThread.cpp:106
(Diff revision 1)
>  AbstractThread::RequiresTailDispatch(AbstractThread* aThread) const
>  {
>    // We require tail dispatch if both the source and destination
>    // threads support it.
> -  return SupportsTailDispatch() && aThread->SupportsTailDispatch();
> +  return SupportsTailDispatch() && aThread && aThread->SupportsTailDispatch();
>  }

Instead of doing this, I would prefer to add a helper called:

RequiresTailDispatchFromCurrentThread()
{
  AbstractThread* current = GetCurrent();
  return current && RequiresTailDispatch(current);
}

And then MOZ_ASSERT(aThread) in RequiresTailDispatch.

We can also use this new helper from the other callers of RequiresTailDispatch.
Thanks for the review!
https://hg.mozilla.org/mozilla-central/rev/87607f70a502
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment on attachment 8748540 [details]
MozReview Request: Bug 1269963 - Add a SyncRunnable::DispatchToThread() overload for AbstractThread. r=bobbyholley.

Approval Request Comment
[Feature/regressing bug #]:1269963
[User impact if declined]: Per bug 1274498 comment 4, we want to uplift bug 1270350 to beta (48) to see if crash can be reduced. This bug is the prerequisite of bug 1270350.
[Describe test coverage new/current, TreeHerder]: Tested on Central and TreeHerder.
[Risks and why]: Low. The change is simple and has been running on Central for weeks without any regression.
[String/UUID change made/needed]:none
Attachment #8748540 - Flags: approval-mozilla-beta?
Comment on attachment 8748540 [details]
MozReview Request: Bug 1269963 - Add a SyncRunnable::DispatchToThread() overload for AbstractThread. r=bobbyholley.

taking it to fix the crash
should be in 48 beta 3
Attachment #8748540 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Blocks: 1345748
Whiteboard: [QDL][TDC-MVP][MEDIA]
Whiteboard: [QDL][TDC-MVP][MEDIA]
You need to log in before you can comment on or make changes to this bug.