Fix data races on |SharedMemoryDataConsumerHandle::Context::notification_task_runner_|

Fix data races on |SharedMemoryDataConsumerHandle::Context::notification_task_runner_|

Writes to |notification_task_runner_| are protected by |lock_| but reads to |notification_task_runner_| in NotifyInternal() and PostNotify() are not protected.
This CL fixes the races by adding locks.

BUG=480746
Committed: https://crrev.com/e1a55538db15f150f4195dde5c8598d9bcf998d9
Cr-Commit-Position: refs/heads/master@{#339008}

[hiroshige, 2015-07-15 12:45:27]

hiroshige@chromium.org changed reviewers:
+ yhirano@chromium.org

[hiroshige, 2015-07-15 12:45:27]

PTAL.

[yhirano, 2015-07-15 13:30:09]

https://codereview.chromium.org/1234213002/diff/20001/content/child/shared_me...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1234213002/diff/20001/content/child/shared_me...
content/child/shared_memory_data_consumer_handle.cc:294: if (needs_notification)
{
I decoupled this section because I though PostNotify doesn't need lock. As it
was wrong can we move this block in the lock-protected area? I don't know if
taking two small locks is better than taking one medium-sized lock.

[hiroshige, 2015-07-15 21:35:40]

https://codereview.chromium.org/1234213002/diff/20001/content/child/shared_me...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1234213002/diff/20001/content/child/shared_me...
content/child/shared_memory_data_consumer_handle.cc:294: if (needs_notification)
{
On 2015/07/15 13:30:09, yhirano wrote:
> I decoupled this section because I though PostNotify doesn't need lock. As it
> was wrong can we move this block in the lock-protected area? I don't know if
> taking two small locks is better than taking one medium-sized lock.

Done.
Taking one lock instead of two is at least good for readability, I think.

[yhirano, 2015-07-16 01:40:35]

Thanks, LGTM

[hiroshige, 2015-07-16 02:37:53]

hiroshige@chromium.org changed reviewers:
+ kinuko@chromium.org

[hiroshige, 2015-07-16 02:37:54]

+kinuko@, could you take a look at as a content/ OWNER?

[kinuko, 2015-07-16 03:28:44]

https://codereview.chromium.org/1234213002/diff/40001/content/child/shared_me...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1234213002/diff/40001/content/child/shared_me...
content/child/shared_memory_data_consumer_handle.cc:287: context_->PostNotify();
If some methods of Context are assumed to be called with a lock held while
others are not, it'd be better to clearly document about the lock condition and
to have AssertAquired in each method.  The current usage of the context lock
feels a bit error prone.

[hiroshige, 2015-07-16 05:35:58]

https://codereview.chromium.org/1234213002/diff/40001/content/child/shared_me...
File content/child/shared_memory_data_consumer_handle.cc (right):

https://codereview.chromium.org/1234213002/diff/40001/content/child/shared_me...
content/child/shared_memory_data_consumer_handle.cc:287: context_->PostNotify();
On 2015/07/16 03:28:44, kinuko wrote:
> If some methods of Context are assumed to be called with a lock held while
> others are not, it'd be better to clearly document about the lock condition
and
> to have AssertAquired in each method.  The current usage of the context lock
> feels a bit error prone.

Added comments/assertions in Patch Set 4.

[kinuko, 2015-07-16 05:49:04]

lgtm

[hiroshige, 2015-07-16 08:57:41]

The CQ bit was checked by hiroshige@chromium.org

[hiroshige, 2015-07-16 08:57:42]

The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/1234213002/#ps60001
(title: "Add AssertAquired().")

[commit-bot: I haz the power, 2015-07-16 08:57:53]

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

[commit-bot: I haz the power, 2015-07-16 10:23:35]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-16 10:25:36]

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