Make DBusStatistics only run on the main thread

Make DBusStatistics only run on the main thread and add additional CHECKs to ensure thread safety.

Calls from other threads will be ignored. Currently the only DBus calls from other threads are for Geolocation. Supporting statistics gathering across multiple threads is unnecessary overhead that we don't currently need.

BUG=168134
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175706

[stevenjb, 2013-01-03 22:59:27]

It turns out that ObjectProxy methods might be called from threads other than
the UI thread. DBusStatistics was not thread safe which was causing occasional
crashes. This makes DBusStatistics thread safe and adds checks to ensure thread
safety.

[davidjames, 2013-01-04 02:21:46]

LGTM. Also ran lots of trybot runs with this on Chrome OS through a few thousand
runs of VMTests and ran into no issues related to this CL.

[hashimoto, 2013-01-04 11:20:10]

Please rewrite the change description.

https://codereview.chromium.org/11761015/diff/3005/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

https://codereview.chromium.org/11761015/diff/3005/dbus/dbus_statistics.cc#ne...
dbus/dbus_statistics.cc:72: // MessageLoop::current() might be NULL in tests.
How about setting up the message loop in tests instead of allowing its absence?
I think it will make the code clearer.

https://codereview.chromium.org/11761015/diff/3005/dbus/dbus_statistics.cc#ne...
dbus/dbus_statistics.cc:74: origin_message_loop_proxy_ =
MessageLoop::current()->message_loop_proxy();
nit: How about using MessageLoopProxy::current()?

https://codereview.chromium.org/11761015/diff/3005/dbus/dbus_statistics.cc#ne...
dbus/dbus_statistics.cc:94: if (base::PlatformThread::CurrentId() !=
origin_thread_id_) {
How about using SingleThreadTaskRunner:: BelongsToCurrentThread()?
(MessageLoopProxy is a SingleThreadTaskRunner)
The same goes for the CHECKs in other places.

[stevenjb, 2013-01-04 20:30:37]

https://codereview.chromium.org/11761015/diff/3005/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

https://codereview.chromium.org/11761015/diff/3005/dbus/dbus_statistics.cc#ne...
dbus/dbus_statistics.cc:72: // MessageLoop::current() might be NULL in tests.
On 2013/01/04 11:20:10, hashimoto wrote:
> How about setting up the message loop in tests instead of allowing its
absence?
> I think it will make the code clearer.
Replacing this with a CHECK fails for a handful of tests; trying to change all
of them didn't seem to make sense (and may not be feasible in all instances). We
already have similar code elsewhere.

https://codereview.chromium.org/11761015/diff/3005/dbus/dbus_statistics.cc#ne...
dbus/dbus_statistics.cc:74: origin_message_loop_proxy_ =
MessageLoop::current()->message_loop_proxy();
On 2013/01/04 11:20:10, hashimoto wrote:
> nit: How about using MessageLoopProxy::current()?

Done.

https://codereview.chromium.org/11761015/diff/3005/dbus/dbus_statistics.cc#ne...
dbus/dbus_statistics.cc:94: if (base::PlatformThread::CurrentId() !=
origin_thread_id_) {
On 2013/01/04 11:20:10, hashimoto wrote:
> How about using SingleThreadTaskRunner:: BelongsToCurrentThread()?
> (MessageLoopProxy is a SingleThreadTaskRunner)
> The same goes for the CHECKs in other places.

As long as origin_message_loop_proxy_ might be NULL we can't use that, and I
really don't want to modify any tests just for the sake of this code.

[hashimoto, 2013-01-07 13:28:00]

https://codereview.chromium.org/11761015/diff/3005/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

https://codereview.chromium.org/11761015/diff/3005/dbus/dbus_statistics.cc#ne...
dbus/dbus_statistics.cc:72: // MessageLoop::current() might be NULL in tests.
On 2013/01/04 20:30:37, stevenjb (chromium) wrote:
> On 2013/01/04 11:20:10, hashimoto wrote:
> > How about setting up the message loop in tests instead of allowing its
> absence?
> > I think it will make the code clearer.
> Replacing this with a CHECK fails for a handful of tests; trying to change all
> of them didn't seem to make sense (and may not be feasible in all instances).
We
> already have similar code elsewhere.

OK.
Please fix the failing browser tests.

[satorux1, 2013-01-08 03:38:47]

LGTM with nits.

https://codereview.chromium.org/11761015/diff/18002/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

https://codereview.chromium.org/11761015/diff/18002/dbus/dbus_statistics.cc#n...
dbus/dbus_statistics.cc:70: CHECK_EQ(origin_thread_id_,
base::PlatformThread::CurrentId());
DCHECK_EQ?

https://codereview.chromium.org/11761015/diff/18002/dbus/dbus_statistics.cc#n...
dbus/dbus_statistics.cc:91: CHECK(stat);
DCHECK? I thought we rarely used CHECK in chrome code.

https://codereview.chromium.org/11761015/diff/18002/dbus/dbus_statistics.cc#n...
dbus/dbus_statistics.cc:108: CHECK_EQ(origin_thread_id_,
base::PlatformThread::CurrentId());
ditto.

[hashimoto, 2013-01-08 03:52:30]

lgtm with a nit

https://codereview.chromium.org/11761015/diff/18002/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

https://codereview.chromium.org/11761015/diff/18002/dbus/dbus_statistics.cc#n...
dbus/dbus_statistics.cc:87: // This code is not thread safe; ignore calls from
other threads.
How about adding a DLOG describing the reason why the call is ignored?

[stevenjb, 2013-01-09 00:03:14]

https://codereview.chromium.org/11761015/diff/18002/dbus/dbus_statistics.cc
File dbus/dbus_statistics.cc (right):

https://codereview.chromium.org/11761015/diff/18002/dbus/dbus_statistics.cc#n...
dbus/dbus_statistics.cc:70: CHECK_EQ(origin_thread_id_,
base::PlatformThread::CurrentId());
On 2013/01/08 03:38:47, satorux1 wrote:
> DCHECK_EQ?

Good call, done.

https://codereview.chromium.org/11761015/diff/18002/dbus/dbus_statistics.cc#n...
dbus/dbus_statistics.cc:87: // This code is not thread safe; ignore calls from
other threads.
On 2013/01/08 03:52:30, hashimoto wrote:
> How about adding a DLOG describing the reason why the call is ignored?

Done.

https://codereview.chromium.org/11761015/diff/18002/dbus/dbus_statistics.cc#n...
dbus/dbus_statistics.cc:91: CHECK(stat);
On 2013/01/08 03:38:47, satorux1 wrote:
> DCHECK? I thought we rarely used CHECK in chrome code.
Yeah, I was being extra paranoid while running the tests, but this one should be
a DCHECk also. Done.

https://codereview.chromium.org/11761015/diff/18002/dbus/dbus_statistics.cc#n...
dbus/dbus_statistics.cc:108: CHECK_EQ(origin_thread_id_,
base::PlatformThread::CurrentId());
On 2013/01/08 03:38:47, satorux1 wrote:
> ditto.

Done.

[commit-bot: I haz the power, 2013-01-09 00:11:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/11761015/19006

[commit-bot: I haz the power, 2013-01-09 04:41:09]

Change committed as 175706