Implement a simple ThreadChecker for Mojo and add to |internal::SharedData| and |internal::Router|.

Implement a simple ThreadChecker for Mojo and add to |internal::SharedData| and |internal::Router|.

|internal::SharedData| is used in several places, in particular,
callbacks, which are very easy to misuse by copying around between
threads. |internal::Router| is used commonly between the various
bindings classes, making it an ideal place to catch misuse of bindings.

BUG=537911
Committed: https://crrev.com/7cff2feae7fcccc416827013df112396c84b5017
Cr-Commit-Position: refs/heads/master@{#352508}

[Anand Mistry (off Chromium), 2015-10-01 07:40:02]

amistry@chromium.org changed reviewers:
+ rockot@chromium.org, yzshen@chromium.org

[Anand Mistry (off Chromium), 2015-10-01 07:40:03]

Note, the use of assert() in a few places is due to dependencies.

[yzshen1, 2015-10-01 16:48:10]

Thanks, Anand. It mostly looks good.

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/mojo_p...
File third_party/mojo/mojo_public.gyp (right):

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/mojo_p...
third_party/mojo/mojo_public.gyp:167: 'sources!': [
I thought files with platform suffix are filtered out on unmatched platforms
automatically?

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
File third_party/mojo/src/mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
third_party/mojo/src/mojo/public/cpp/bindings/BUILD.gn:87: if (!is_win) {
Again, I thought we could directly put these files in the sources list and it
will do the right thing?

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
File third_party/mojo/src/mojo/public/cpp/bindings/lib/router.h (right):

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
third_party/mojo/src/mojo/public/cpp/bindings/lib/router.h:30: void
set_incoming_receiver(MessageReceiverWithResponderStatus* receiver) {
I wonder why you didn't put the checker in all the public methods?

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
File third_party/mojo/src/mojo/public/cpp/bindings/lib/shared_data.h (right):

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
third_party/mojo/src/mojo/public/cpp/bindings/lib/shared_data.h:60:
assert(thread_checker_.CalledOnValidThread());
Why using assert here but not MOJO_DCHECK as what you did in router?

[Anand Mistry (off Chromium), 2015-10-01 23:16:52]

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/mojo_p...
File third_party/mojo/mojo_public.gyp (right):

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/mojo_p...
third_party/mojo/mojo_public.gyp:167: 'sources!': [
On 2015/10/01 16:48:09, yzshen1 wrote:
> I thought files with platform suffix are filtered out on unmatched platforms
> automatically?

Oops. Didn't realise that.

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
File third_party/mojo/src/mojo/public/cpp/bindings/BUILD.gn (right):

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
third_party/mojo/src/mojo/public/cpp/bindings/BUILD.gn:87: if (!is_win) {
On 2015/10/01 16:48:10, yzshen1 wrote:
> Again, I thought we could directly put these files in the sources list and it
> will do the right thing?

Done.

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
File third_party/mojo/src/mojo/public/cpp/bindings/lib/router.h (right):

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
third_party/mojo/src/mojo/public/cpp/bindings/lib/router.h:30: void
set_incoming_receiver(MessageReceiverWithResponderStatus* receiver) {
On 2015/10/01 16:48:10, yzshen1 wrote:
> I wonder why you didn't put the checker in all the public methods?

Since this is an internal class, I looked at the call sites. These two functions
are only called along side the constructor, so I left them. handle() is actually
dead code.

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
File third_party/mojo/src/mojo/public/cpp/bindings/lib/shared_data.h (right):

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
third_party/mojo/src/mojo/public/cpp/bindings/lib/shared_data.h:60:
assert(thread_checker_.CalledOnValidThread());
On 2015/10/01 16:48:10, yzshen1 wrote:
> Why using assert here but not MOJO_DCHECK as what you did in router?

Because this is part of the "callback" GN rule which doesn't (and probably
can't) depend on environment, which is where MOJO_DCHECK is defined.

[yzshen1, 2015-10-02 16:05:45]

LGTM

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
File third_party/mojo/src/mojo/public/cpp/bindings/lib/shared_data.h (right):

https://codereview.chromium.org/1380043002/diff/40001/third_party/mojo/src/mo...
third_party/mojo/src/mojo/public/cpp/bindings/lib/shared_data.h:60:
assert(thread_checker_.CalledOnValidThread());
On 2015/10/01 23:16:52, Anand Mistry wrote:
> On 2015/10/01 16:48:10, yzshen1 wrote:
> > Why using assert here but not MOJO_DCHECK as what you did in router?
> 
> Because this is part of the "callback" GN rule which doesn't (and probably
> can't) depend on environment, which is where MOJO_DCHECK is defined.

I see, it is all because callback is used in environment/ and utility/. A little
confusing though. :/

[Anand Mistry (off Chromium), 2015-10-06 00:26:21]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2015-10-06 00:28:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1380043002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1380043002/60001

[commit-bot: I haz the power, 2015-10-06 01:37:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-06 01:38:04]

Patchset 4 (id:??) landed as
https://crrev.com/7cff2feae7fcccc416827013df112396c84b5017
Cr-Commit-Position: refs/heads/master@{#352508}

[Elliot Glaysher, 2015-10-06 18:26:52]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1369933004/ by erg@chromium.org.

The reason for reverting is: Mandoline now crashes immediately on startup.
Reverting this change locally fixes it.

(mandoline is probably doing something unsafe, but it worked previously to this
patch.)

mandoline:
../../third_party/mojo/src/mojo/public/cpp/bindings/lib/shared_data.h:64: void
mojo::internal::SharedData<mojo::internal::SharedPtr<mojo::Callback<void
()>::Runnable>::Impl>::Holder::Release() [T =
mojo::internal::SharedPtr<mojo::Callback<void ()>::Runnable>::Impl]: Assertion
`thread_checker_.CalledOnValidThread()' failed.
.