Fix a data race in WebMessagePortChannelImpl.

Fix a data race in WebMessagePortChannelImpl.

It's unsafe to pass a WebString (which is logically a RefPtr<StringImpl>)
across threads because the underlying object is not thread-safe ref-counted.
Instead, the base::string16 that the receiving method accepts should be
constructed before posting the task.

BUG=635659
Committed: https://crrev.com/414c42a0b400f213b58314c1b26926f9b897ac8a
Cr-Commit-Position: refs/heads/master@{#410761}

[jbroman, 2016-08-09 17:45:59]

The CQ bit was checked by jbroman@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 17:46:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jbroman, 2016-08-09 17:50:03]

jbroman@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2016-08-09 18:45:08]

This fix lgtm.

How do we know we're not doing that elsewhere? Can we make a WebString
unbindable?

[jbroman, 2016-08-09 19:25:59]

On 2016/08/09 at 18:45:08, avi wrote:
> This fix lgtm.
> 
> How do we know we're not doing that elsewhere? Can we make a WebString
unbindable?

It's not impossible to do, but this is only unsafe if the callback could be
destroyed on another thread. AFAIK we don't currently have similar checks for
scoped_refptr<T> where T is not ThreadSafeRefCounted.

I've tried adding a check locally[1], but the sites I found (before the compile
aborted) aren't unsafe:
mock_webrtc_peer_connection_handler.cc:169,183; mock_web_user_media_client.cc:57
(these seem to run on the same thread, but I don't know components/test_runner
that well)
user_media_client_impl.cc:838 (uses current thread)
webcontentdecryptionmoduleaccess_impl.cc:78 (uses current thread)
web_bluetooth_impl.cc:90 (I think this task also winds up being on the correct
thread)

One could imagine separating callbacks that are run on the original thread and
those that run on another thread. But this is a longer-term effort[2].

[1] In case I forget, the template magic is:

template <bool...> struct all_true : std::true_type {};
template <bool A, bool... Rest> struct all_true<A, Rest...> :
std::integral_constant<bool, A && all_true<Rest...>::value> {};

  static_assert(
      all_true<!std::is_same<blink::WebString, typename
std::decay<BoundArgs>::type>::value...>::value,
      "No parameter can decay to blink::WebString.");

[2] hiroshige@'s
https://docs.google.com/document/d/114s1KTrf6xXdwInRDMDJAdxX_zIKt6xJSbZ-9qBgU...
is relevant

[jbroman, 2016-08-09 19:26:30]

The CQ bit was unchecked by jbroman@chromium.org

[jbroman, 2016-08-09 19:26:33]

The CQ bit was checked by jbroman@chromium.org

[commit-bot: I haz the power, 2016-08-09 19:27:04]

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, 2016-08-09 19:37:26]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-09 19:39:16]

Description was changed from

==========
Fix a data race in WebMessagePortChannelImpl.

It's unsafe to pass a WebString (which is logically a RefPtr<StringImpl>)
across threads because the underlying object is not thread-safe ref-counted.
Instead, the base::string16 that the receiving method accepts should be
constructed before posting the task.

BUG=635659
==========

to

==========
Fix a data race in WebMessagePortChannelImpl.

It's unsafe to pass a WebString (which is logically a RefPtr<StringImpl>)
across threads because the underlying object is not thread-safe ref-counted.
Instead, the base::string16 that the receiving method accepts should be
constructed before posting the task.

BUG=635659

Committed: https://crrev.com/414c42a0b400f213b58314c1b26926f9b897ac8a
Cr-Commit-Position: refs/heads/master@{#410761}
==========

[commit-bot: I haz the power, 2016-08-09 19:39:17]

Patchset 1 (id:??) landed as
https://crrev.com/414c42a0b400f213b58314c1b26926f9b897ac8a
Cr-Commit-Position: refs/heads/master@{#410761}