Remove lock from SharedMemoryDataConsumerHandle::Context destructor

Remove lock from SharedMemoryDataConsumerHandle::Context destructor

SharedMemoryDataConsumerHandle::Context posts tasks while its lock_
locked. That means a lock held by the TaskRunner will be locked
after |lock_| is locked. On the other hand, the Context may be
destructed in the middle of TaskRunner destruction, which means we
cannot lock |lock_| in the Context destruction. Otherwise, a
lock-order-inversion problem will be introduced.

This CL stops locking |lock_| in the destructor. It is safe because
no one accesses to Content's member variables without a valid
scoped_refptr to the Context.

BUG=651747
Committed: https://crrev.com/d9a07ccafb34327cc194631d354395245f65a52b
Cr-Commit-Position: refs/heads/master@{#422614}

[yhirano, 2016-10-03 06:48:15]

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

[commit-bot: I haz the power, 2016-10-03 06:48:28]

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

[yhirano, 2016-10-03 07:01:18]

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

[commit-bot: I haz the power, 2016-10-03 07:01:37]

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

[yhirano, 2016-10-03 07:02:31]

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

[commit-bot: I haz the power, 2016-10-03 07:02:36]

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

[yhirano, 2016-10-03 07:08:25]

Description was changed from

==========
thread fix

BUG=651747
==========

to

==========
Remove lock from SharedMemoryDataConsumerHandle::Context destructor

SharedMemoryDataConsumerHandle::Context posts tasks while its lock_
locked. That means a lock held by the TaskRunner will be locked
after |lock_| is locked. On the other hand, the Context may be
destructed in the middle of TaskRunner destruction, which means we
cannot lock |lock_| in the Context destruction. Otherwise, a
lock-order-inversion problem will be introduced.


BUG=651747
==========

[yhirano, 2016-10-03 07:08:25]

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

[yhirano, 2016-10-03 07:14:02]

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

[commit-bot: I haz the power, 2016-10-03 07:14:15]

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

[yhirano, 2016-10-03 07:14:39]

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

[commit-bot: I haz the power, 2016-10-03 07:14:54]

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

[yhirano, 2016-10-03 07:21:09]

Patchset #3 (id:40001) has been deleted

[yhirano, 2016-10-03 07:21:14]

Patchset #2 (id:20001) has been deleted

[yhirano, 2016-10-03 07:21:19]

Patchset #1 (id:1) has been deleted

[commit-bot: I haz the power, 2016-10-03 07:52:46]

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

[commit-bot: I haz the power, 2016-10-03 07:52:47]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[yhirano, 2016-10-03 08:08:13]

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

[commit-bot: I haz the power, 2016-10-03 08:08:24]

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

[yhirano, 2016-10-03 08:50:06]

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

[commit-bot: I haz the power, 2016-10-03 08:50:21]

Dry run: 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-10-03 10:13:28]

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

[commit-bot: I haz the power, 2016-10-03 10:13:29]

Dry run: This issue passed the CQ dry run.

[hiroshige, 2016-10-03 11:09:25]

lgtm with a comment to CL description:
Could you add explanation why removing the lock in ~Context() is safe?

(Splitting this CL into two CLs, one for replacing raw pointer with unique_ptr
and one for removing lock in ~Context() might be clearer but this CL is also OK)

(FYI I blamed the code and the lock was added by me at
https://codereview.chromium.org/1234213002/#ps60001.
The lock looks like added to make ~Context() consistent with
lock_.AssertAquired() added to Clear().)

[yhirano, 2016-10-03 11:36:59]

Description was changed from

==========
Remove lock from SharedMemoryDataConsumerHandle::Context destructor

SharedMemoryDataConsumerHandle::Context posts tasks while its lock_
locked. That means a lock held by the TaskRunner will be locked
after |lock_| is locked. On the other hand, the Context may be
destructed in the middle of TaskRunner destruction, which means we
cannot lock |lock_| in the Context destruction. Otherwise, a
lock-order-inversion problem will be introduced.


BUG=651747
==========

to

==========
Remove lock from SharedMemoryDataConsumerHandle::Context destructor

SharedMemoryDataConsumerHandle::Context posts tasks while its lock_
locked. That means a lock held by the TaskRunner will be locked
after |lock_| is locked. On the other hand, the Context may be
destructed in the middle of TaskRunner destruction, which means we
cannot lock |lock_| in the Context destruction. Otherwise, a
lock-order-inversion problem will be introduced.

This CL stops locking |lock_| in the destructor. It is safe because
no one accesses to Content's member variables without a valid
scoped_refptr to the Context.

BUG=651747
==========

[yhirano, 2016-10-03 11:37:22]

On 2016/10/03 11:09:25, hiroshige wrote:
> lgtm with a comment to CL description:
> Could you add explanation why removing the lock in ~Context() is safe?
> 
Done.

[yhirano, 2016-10-03 11:37:40]

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

[yhirano, 2016-10-03 11:37:41]

+jam@ for OWNER review.

[jam, 2016-10-03 17:41:32]

lgtm

[yhirano, 2016-10-04 00:00:08]

The CQ bit was checked by yhirano@chromium.org

[commit-bot: I haz the power, 2016-10-04 00:00:45]

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-10-04 00:10:35]

Description was changed from

==========
Remove lock from SharedMemoryDataConsumerHandle::Context destructor

SharedMemoryDataConsumerHandle::Context posts tasks while its lock_
locked. That means a lock held by the TaskRunner will be locked
after |lock_| is locked. On the other hand, the Context may be
destructed in the middle of TaskRunner destruction, which means we
cannot lock |lock_| in the Context destruction. Otherwise, a
lock-order-inversion problem will be introduced.

This CL stops locking |lock_| in the destructor. It is safe because
no one accesses to Content's member variables without a valid
scoped_refptr to the Context.

BUG=651747
==========

to

==========
Remove lock from SharedMemoryDataConsumerHandle::Context destructor

SharedMemoryDataConsumerHandle::Context posts tasks while its lock_
locked. That means a lock held by the TaskRunner will be locked
after |lock_| is locked. On the other hand, the Context may be
destructed in the middle of TaskRunner destruction, which means we
cannot lock |lock_| in the Context destruction. Otherwise, a
lock-order-inversion problem will be introduced.

This CL stops locking |lock_| in the destructor. It is safe because
no one accesses to Content's member variables without a valid
scoped_refptr to the Context.

BUG=651747
==========

[commit-bot: I haz the power, 2016-10-04 00:10:36]

Committed patchset #4 (id:40002)

[commit-bot: I haz the power, 2016-10-04 00:13:05]

Description was changed from

==========
Remove lock from SharedMemoryDataConsumerHandle::Context destructor

SharedMemoryDataConsumerHandle::Context posts tasks while its lock_
locked. That means a lock held by the TaskRunner will be locked
after |lock_| is locked. On the other hand, the Context may be
destructed in the middle of TaskRunner destruction, which means we
cannot lock |lock_| in the Context destruction. Otherwise, a
lock-order-inversion problem will be introduced.

This CL stops locking |lock_| in the destructor. It is safe because
no one accesses to Content's member variables without a valid
scoped_refptr to the Context.

BUG=651747
==========

to

==========
Remove lock from SharedMemoryDataConsumerHandle::Context destructor

SharedMemoryDataConsumerHandle::Context posts tasks while its lock_
locked. That means a lock held by the TaskRunner will be locked
after |lock_| is locked. On the other hand, the Context may be
destructed in the middle of TaskRunner destruction, which means we
cannot lock |lock_| in the Context destruction. Otherwise, a
lock-order-inversion problem will be introduced.

This CL stops locking |lock_| in the destructor. It is safe because
no one accesses to Content's member variables without a valid
scoped_refptr to the Context.

BUG=651747

Committed: https://crrev.com/d9a07ccafb34327cc194631d354395245f65a52b
Cr-Commit-Position: refs/heads/master@{#422614}
==========

[commit-bot: I haz the power, 2016-10-04 00:13:09]

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