base: Re-apply WeakPtr support for SequencedWorkerPools, fixing deadlock

base: Re-apply WeakPtr support for SequencedWorkerPools, fixing deadlock

This reverts the revert found here: https://codereview.chromium.org/19882002

It also fixes the deadlock that caused the revert, reported here:
http://crbug.com/261448

Patchset 1 is simply what was originally committed (and reverted). Subsequent patchsets show the fix of the deadlock problem.

BUG=165590
TBR=darin

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

[tommycli, 2013-07-25 15:57:09]

ajwong: I think this patch fixes the issue you described here:
http://crbug.com/261448

Patchset 1 is the original.
Patchset 2 is the updated, with comments.

https://codereview.chromium.org/20163004/diff/9001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/20163004/diff/9001/base/memory/weak_ptr.cc#ne...
base/memory/weak_ptr.cc:20: DCHECK(HasOneRef() ||
sequence_checker_.CalledOnValidSequencedThread())
Changing the order of the conditionals also coincidentally avoids the issue
since it's only Flags that have never had IsValid called that will have
unassigned sequence tokens.

The other change is the real fix, but changing the order here makes sense
anyways.

https://codereview.chromium.org/20163004/diff/9001/base/threading/sequenced_w...
File base/threading/sequenced_worker_pool.cc (right):

https://codereview.chromium.org/20163004/diff/9001/base/threading/sequenced_w...
base/threading/sequenced_worker_pool.cc:1127: // Don't construct lazy instance
on check.
This is the 'real' fix - where it won't attempt to construct a lazy instance of
the g_lazy_tls_ptr on checking.

[tommycli, 2013-07-25 19:27:52]

+akalin, darin: This patch may fix the deadlock issue with the sequenced weak
pointers. Review if interested.

[akalin, 2013-07-25 19:36:37]

https://codereview.chromium.org/20163004/diff/9001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/20163004/diff/9001/base/memory/weak_ptr.cc#ne...
base/memory/weak_ptr.cc:20: DCHECK(HasOneRef() ||
sequence_checker_.CalledOnValidSequencedThread())
On 2013/07/25 15:57:09, tommycli wrote:
> Changing the order of the conditionals also coincidentally avoids the issue
> since it's only Flags that have never had IsValid called that will have
> unassigned sequence tokens.
> 
> The other change is the real fix, but changing the order here makes sense
> anyways.

I'm a bit wary about changing the order here. The original order makes more
sense to me since it follows the logic of the comment. Also, with
short-circuiting, it's unclear that this order is equivalent to the original
one.

I'd rather keep it as-is if there's no reason for it.

[tommycli, 2013-07-25 19:54:59]

akalin: Addressed your comments.

I'm 'pretty sure' my change will fix the deadlock issue - but not 'dead sure'.
Why was this issue not detected during the CQ trybots during the original
commit?

https://codereview.chromium.org/20163004/diff/9001/base/memory/weak_ptr.cc
File base/memory/weak_ptr.cc (right):

https://codereview.chromium.org/20163004/diff/9001/base/memory/weak_ptr.cc#ne...
base/memory/weak_ptr.cc:20: DCHECK(HasOneRef() ||
sequence_checker_.CalledOnValidSequencedThread())
Done. Sounds good.

On 2013/07/25 19:36:38, akalin wrote:
> On 2013/07/25 15:57:09, tommycli wrote:
> > Changing the order of the conditionals also coincidentally avoids the issue
> > since it's only Flags that have never had IsValid called that will have
> > unassigned sequence tokens.
> > 
> > The other change is the real fix, but changing the order here makes sense
> > anyways.
> 
> I'm a bit wary about changing the order here. The original order makes more
> sense to me since it follows the logic of the comment. Also, with
> short-circuiting, it's unclear that this order is equivalent to the original
> one.
> 
> I'd rather keep it as-is if there's no reason for it.

[akalin, 2013-07-25 19:59:19]

LGTM

but please revert quickly if it causes problems again.

[tommycli, 2013-07-25 20:32:36]

Sounds good. TBRing darin because he previously LGed it.

I send to CQ now.

[commit-bot: I haz the power, 2013-07-25 20:57:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/20163004/25001

[commit-bot: I haz the power, 2013-07-26 02:47:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommycli@chromium.org/20163004/25001

[commit-bot: I haz the power, 2013-07-26 14:24:00]

Change committed as 213906