1269963 - Add a SyncRunnable::DispatchToThread() overload for AbstractThread

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

As AbstractThread is also a citizen of xpcom, it deserves a place in SyncRunnable.

Comment 1

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

Attachment: MozReview Request: Bug 1269963 - Add a SyncRunnable::DispatchToThread() overload for AbstractThread. r=bobbyholley.

Review commit: https://reviewboard.mozilla.org/r/50381/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/50381/

Comment 2

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

try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2df423f0da0c

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

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

Thanks for the review!

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87607f70a502

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87607f70a502

Comment 7

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

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

Comment 8

[Sylvestre Ledru [:Sylvestre]]

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

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2b0b8ffc6495