Fix race when calling GetWeakPtr() from multiple threads.

Fix race when calling GetWeakPtr() from multiple threads.

WeakReferenceOwner allows moving ownership to a different thread
by checking if all WeakPtr instances have gone away and recreating
that flag if that is the case.
However the entirety of this operation is not atomic, and therefore
a race exists if the last WeakPtr instance goes away one one thread
while GetWeakPtr() is called on another thread.
Avoid this by keeping a persistent reference to the flag.

BUG=356540
Committed: https://crrev.com/4727025e0b53a0613eb5c90d6ddcba39643476d1
Cr-Commit-Position: refs/heads/master@{#304681}

[no sievers, 2014-11-18 20:57:55]

sievers@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[no sievers, 2014-11-18 20:58:23]

ptal

[DaleCurtis, 2014-11-18 21:15:07]

Nice fix. I thought you would use the new WeakPtr everywhere but now I see what
you're doing.

I think the base WeakPtr class needs documentation around this plus some form of
check if possible.

Is there any easy way to make the check atomic in the base WeakPtr class? We're
already using a scoped refptr which uses atomics under the hood so using an
atomic compare/swap seems possible.

[no sievers, 2014-11-18 21:17:06]

On 2014/11/18 21:15:07, DaleCurtis wrote:
> Nice fix. I thought you would use the new WeakPtr everywhere but now I see
what
> you're doing.
> 
> I think the base WeakPtr class needs documentation around this plus some form
of
> check if possible.
> 
> Is there any easy way to make the check atomic in the base WeakPtr class?
We're
> already using a scoped refptr which uses atomics under the hood so using an
> atomic compare/swap seems possible.

Probably not without an expensive lock. We had discussed this separately
recently.
Will follow up with wez again. I also suggested making GetWeakPtr() truly
constant (right now flag_ is mutable).
You'd then more explicitly destroy the WeakReferenceOwner when you do need to
move ownership to a different thread (which I think one rarely does).

[DaleCurtis, 2014-11-18 21:33:53]

Ah, yeah, I just played with it for a bit and the !HasOneRef() requirement makes
it seemingly impossible to implement a simple Atomic_CompareAndSwap approach a
la something like:

  if (!flag_) {
    scoped_refptr<WeakReference::Flag> new_flag(new WeakReference::Flag());
    Acquire_CompareAndSwap(&flag_, NULL, &new_flag);
  }

so lgtm, sadface.

[no sievers, 2014-11-18 21:34:23]

The CQ bit was checked by sievers@chromium.org

[commit-bot: I haz the power, 2014-11-18 21:34:56]

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

[commit-bot: I haz the power, 2014-11-18 22:24:08]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2014-11-18 22:24:44]

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

[Wez, 2014-11-18 23:28:09]

wez@chromium.org changed reviewers:
+ wez@chromium.org

[Wez, 2014-11-18 23:28:10]

https://codereview.chromium.org/735143002/diff/1/media/blink/buffered_data_so...
File media/blink/buffered_data_source.cc (right):

https://codereview.chromium.org/735143002/diff/1/media/blink/buffered_data_so...
media/blink/buffered_data_source.cc:105: weak_ptr_(weak_factory_.GetWeakPtr()) {
You don't seem to be actually referring to this member anywhere, though?

https://codereview.chromium.org/735143002/diff/1/media/blink/buffered_data_so...
File media/blink/buffered_data_source.h (right):

https://codereview.chromium.org/735143002/diff/1/media/blink/buffered_data_so...
media/blink/buffered_data_source.h:46: // before being passed to other threads.
It may be deleted on any thread.
This means you can't use thread-bound resources like WeakPtrFactory in this
class - WeakPtrFactory must always be torn down or invalidated on the same
thread that the WeakPtrs are used on.

Which threads does this implementation actually run, or get called, on?  It
looks like it's an "owned" object that gets called from multiple threads...?

[Wez, 2014-11-18 23:28:57]

not LGTM, incidentally.

[no sievers, 2014-11-18 23:32:19]

https://codereview.chromium.org/735143002/diff/1/media/blink/buffered_data_so...
File media/blink/buffered_data_source.h (right):

https://codereview.chromium.org/735143002/diff/1/media/blink/buffered_data_so...
media/blink/buffered_data_source.h:46: // before being passed to other threads.
It may be deleted on any thread.
On 2014/11/18 23:28:10, Wez wrote:
> This means you can't use thread-bound resources like WeakPtrFactory in this
> class - WeakPtrFactory must always be torn down or invalidated on the same
> thread that the WeakPtrs are used on.
> 
> Which threads does this implementation actually run, or get called, on?  It
> looks like it's an "owned" object that gets called from multiple threads...?

I missed that. That'd be bad. I thought this object lives on a thread which
doesn't change. And another thread comes and gets a WeakPtr (while it knows the
object is still around) and then uses the WeakPtr only to run tasks on the
thread that the object lives on.

[DaleCurtis, 2014-11-18 23:46:41]

The BufferedDataSource is created and destroyed on the render thread (see
WebMediaPlayerImpl's main_task_runner_) it does vend WeakPtrs (which are deref'd
on the render thread) from the media thread though, which is what this CL
presumably resolves.

[DaleCurtis, 2014-11-18 23:47:46]

https://codereview.chromium.org/735143002/diff/1/media/blink/buffered_data_so...
File media/blink/buffered_data_source.h (right):

https://codereview.chromium.org/735143002/diff/1/media/blink/buffered_data_so...
media/blink/buffered_data_source.h:46: // before being passed to other threads.
It may be deleted on any thread.
On 2014/11/18 23:32:19, sievers wrote:
> On 2014/11/18 23:28:10, Wez wrote:
> > This means you can't use thread-bound resources like WeakPtrFactory in this
> > class - WeakPtrFactory must always be torn down or invalidated on the same
> > thread that the WeakPtrs are used on.
> > 
> > Which threads does this implementation actually run, or get called, on?  It
> > looks like it's an "owned" object that gets called from multiple threads...?
> 
> I missed that. That'd be bad. I thought this object lives on a thread which
> doesn't change. And another thread comes and gets a WeakPtr (while it knows
the
> object is still around) and then uses the WeakPtr only to run tasks on the
> thread that the object lives on.

I don't think these docs are true anymore, see:

https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...

Which owns the BufferedDataSource.

[Wez, 2014-11-19 18:46:59]

On 2014/11/18 23:47:46, DaleCurtis wrote:
[snip]
> I don't think these docs are true anymore, see:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/webmed...
> 
> Which owns the BufferedDataSource.

In that case can we update the docs and add a ThreadChecker, or just checks
against the render-task-runner, to ensure that that constraint is respected?

[DaleCurtis, 2014-11-19 18:49:44]

Yeah that's a good idea. sievers@ is welcome to it in a followup CL or can
assign the bug to me and I'll change them / find someone to do so.