Add comment clarifying the usage of CreateSingleThreadTaskRunner

Add comment clarifying the usage of CreateSingleThreadTaskRunner

R=thestig@chromium.org
CC=gab@chromium.org,peter@chromium.org,hashimoto@chromium.org
NOTRY=true

Review-Url: https://codereview.chromium.org/2928513003
Cr-Commit-Position: refs/heads/master@{#480990}
Committed: https://chromium.googlesource.com/chromium/src/+/3bfd311d7f6dbebc232d2de77a7e83421d8989fc

[Tom Anderson, 2017-06-06 20:50:09]

Description was changed from

==========
Add comment clarifying the usage of CreateSingleThreadTaskRunner

R=thestig@chromium.org
CC=gab@chromium.org,peter@chromium.org,hashimoto@chromium.org
==========

to

==========
Add comment clarifying the usage of CreateSingleThreadTaskRunner

R=thestig@chromium.org
CC=gab@chromium.org,peter@chromium.org,hashimoto@chromium.org
NOTRY=true
==========

[Tom Anderson, 2017-06-06 20:51:45]

thomasanderson@chromium.org changed reviewers:
+ gab@chromium.org, hashimoto@chromium.org

[Tom Anderson, 2017-06-06 20:51:45]

thestig ptal

hashimoto: Here's the DCHECK I'm getting.  Is there anything in ::dbus that
requires this to be single threaded?  Can this be made into a sequence check
instead?

[80745:80850:0606/134116.893444:FATAL:bus.cc(832)] Check failed:
origin_task_runner_.get(). 
#0 0x7f7d884b898b base::debug::StackTrace::StackTrace()
#1 0x7f7d884b768c base::debug::StackTrace::StackTrace()
#2 0x7f7d8852b243 logging::LogMessage::~LogMessage()
#3 0x7f7d7d01b577 dbus::Bus::GetOriginTaskRunner()
#4 0x7f7d7d0668fa dbus::ObjectProxy::HandleMessage()
#5 0x7f7d7d06540d dbus::ObjectProxy::HandleMessageThunk()
#6 0x7f7d6b4699d6 dbus_connection_dispatch
#7 0x7f7d7d019b39 dbus::Bus::ProcessAllIncomingDataIfAny()
#8 0x7f7d7d030997
_ZN4base8internal13FunctorTraitsIMN4dbus3BusEFvvEvE6InvokeIRK13scoped_refptrIS3_EJEEEvS5_OT_DpOT0_
#9 0x7f7d7d0308e1
_ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIRKMN4dbus3BusEFvvEJRK13scoped_refptrIS5_EEEEvOT_DpOT0_
#10 0x7f7d7d030882
_ZN4base8internal7InvokerINS0_9BindStateIMN4dbus3BusEFvvEJ13scoped_refptrIS4_EEEEFvvEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#11 0x7f7d7d0307cc
_ZN4base8internal7InvokerINS0_9BindStateIMN4dbus3BusEFvvEJ13scoped_refptrIS4_EEEEFvvEE3RunEPNS0_13BindStateBaseE
#12 0x7f7d8847416e
_ZNO4base8CallbackIFvvELNS_8internal8CopyModeE0ELNS2_10RepeatModeE0EE3RunEv
#13 0x7f7d884be5ee base::debug::TaskAnnotator::RunTask()
#14 0x7f7d886871bb base::internal::TaskTracker::PerformRunTask()
#15 0x7f7d88689913 base::internal::TaskTrackerPosix::PerformRunTask()
#16 0x7f7d886863ad base::internal::TaskTracker::RunNextTask()
#17 0x7f7d88673bc1 base::internal::SchedulerWorker::Thread::ThreadMain()
#18 0x7f7d8869537a base::(anonymous namespace)::ThreadFunc()
#19 0x7f7d889f4184 start_thread
#20 0x7f7d6e95dbed clone

[hashimoto, 2017-06-07 03:49:35]

hashimoto@chromium.org changed reviewers:
+ satorux@chromium.org

[hashimoto, 2017-06-07 03:49:35]

+satorux@ as the original author of the D-Bus code.
I believe the origin thread checks in our D-Bus code can be replaced with
sequence checks, but I guess it's going to require some amount of effort to
finish.

[satorux1, 2017-06-08 04:31:36]

On 2017/06/07 03:49:35, hashimoto wrote:
> +satorux@ as the original author of the D-Bus code.
> I believe the origin thread checks in our D-Bus code can be replaced with
> sequence checks, but I guess it's going to require some amount of effort to
> finish.

I think it's safer to stick with the same thread checks because of
crbug.com/130984

[gab, 2017-06-08 16:18:10]

On 2017/06/08 04:31:36, satorux1 wrote:
> On 2017/06/07 03:49:35, hashimoto wrote:
> > +satorux@ as the original author of the D-Bus code.
> > I believe the origin thread checks in our D-Bus code can be replaced with
> > sequence checks, but I guess it's going to require some amount of effort to
> > finish.
> 
> I think it's safer to stick with the same thread checks because of
> crbug.com/130984

Per https://crbug.com/130984#c3 I'd agree with that sentiment, my original
request was mostly to add a comment on the
base::CreateSingleThreadTaskRunnerWithTraits() call as to why that's required to
be single-threaded. If there's a good reason to be thread-affine, "library
claims to be thread-safe but isn't happy when accessed from different threads
even if properly sequenced" is a good reason, that's cool.

[Tom Anderson, 2017-06-19 19:02:38]

On 2017/06/08 16:18:10, gab wrote:
> On 2017/06/08 04:31:36, satorux1 wrote:
> > On 2017/06/07 03:49:35, hashimoto wrote:
> > > +satorux@ as the original author of the D-Bus code.
> > > I believe the origin thread checks in our D-Bus code can be replaced with
> > > sequence checks, but I guess it's going to require some amount of effort
to
> > > finish.
> > 
> > I think it's safer to stick with the same thread checks because of
> > crbug.com/130984
> 
> Per https://crbug.com/130984#c3 I'd agree with that sentiment, my original
> request was mostly to add a comment on the
> base::CreateSingleThreadTaskRunnerWithTraits() call as to why that's required
to
> be single-threaded. If there's a good reason to be thread-affine, "library
> claims to be thread-safe but isn't happy when accessed from different threads
> even if properly sequenced" is a good reason, that's cool.

Makes sense.  Updated the comment and referenced that bug

[gab, 2017-06-20 21:42:51]

lgtm

[Tom Anderson, 2017-06-20 21:50:30]

The CQ bit was checked by thomasanderson@chromium.org

[commit-bot: I haz the power, 2017-06-20 21:50:54]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 22:04:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-20 22:04:45]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Lei Zhang, 2017-06-20 22:08:48]

The CQ bit was checked by thestig@chromium.org

[Lei Zhang, 2017-06-20 22:08:49]

lgtm

[commit-bot: I haz the power, 2017-06-20 22:09:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-20 22:16:37]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1497996528727250,
"parent_rev": "183f3046f02992f1390d59a357de419689826c2e", "commit_rev":
"3bfd311d7f6dbebc232d2de77a7e83421d8989fc"}

[commit-bot: I haz the power, 2017-06-20 22:16:51]

Description was changed from

==========
Add comment clarifying the usage of CreateSingleThreadTaskRunner

R=thestig@chromium.org
CC=gab@chromium.org,peter@chromium.org,hashimoto@chromium.org
NOTRY=true
==========

to

==========
Add comment clarifying the usage of CreateSingleThreadTaskRunner

R=thestig@chromium.org
CC=gab@chromium.org,peter@chromium.org,hashimoto@chromium.org
NOTRY=true

Review-Url: https://codereview.chromium.org/2928513003
Cr-Commit-Position: refs/heads/master@{#480990}
Committed:
https://chromium.googlesource.com/chromium/src/+/3bfd311d7f6dbebc232d2de77a7e...
==========

[commit-bot: I haz the power, 2017-06-20 22:16:53]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/3bfd311d7f6dbebc232d2de77a7e...