Restore CDM context on Resume().

Restore CDM context on Resume().

Also converts all uses of |weak_factory_| to use a |weak_this_|, as GetWeakPtr() is not threadsafe.

BUG=595898
Committed: https://crrev.com/7655364f9ae672fdbd05e5751006387207b0bf51
Cr-Commit-Position: refs/heads/master@{#382109}

[sandersd (OOO until July 31), 2016-03-17 22:32:12]

sandersd@chromium.org changed reviewers:
+ xhwang@chromium.org

[ddorwin, 2016-03-17 22:44:18]

The BUG entry is incorrect.

I don't know the background here, but will resume work on Android and other
platforms where the DRM resources might have been reclaimed by the system?

[sandersd (OOO until July 31), 2016-03-17 22:46:12]

Description was changed from

==========
Restore CDM context on Resume().

BUG=95898
==========

to

==========
Restore CDM context on Resume().

BUG=595898
==========

[sandersd (OOO until July 31), 2016-03-17 22:47:41]

On 2016/03/17 22:44:18, ddorwin wrote:
> The BUG entry is incorrect.
> 
> I don't know the background here, but will resume work on Android and other
> platforms where the DRM resources might have been reclaimed by the system?

Sorry, missed the leading '5'.

The behavior here at least fixes the recoverable cases. Handling reclaimed
resources would be a general problem for Spitzer, but this CL is to fix Desktop
now that Suspend/Resume is enabled everywhere.

[xhwang, 2016-03-17 23:11:15]

The logic looks good. I just have a question around weak ptr.

https://chromiumcodereview.appspot.com/1815013002/diff/1/media/base/pipeline_...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1815013002/diff/1/media/base/pipeline_...
media/base/pipeline_impl.cc:745: weak_factory_.GetWeakPtr(),
It seems we are already calling GetWeakPtr() on two threads, this is unsafe
according to:

https://codereview.chromium.org/1464183002#msg22

Actually a WeakPtrFactory can only be bound to one task runner and we should
have DCHECK for it. So I am confused how things are working today?

https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

[xhwang, 2016-03-17 23:12:20]

Also, it'll be great to have some test coverage, though it's not clear to me
what's the best way to do it.

[sandersd (OOO until July 31), 2016-03-17 23:25:03]

I agree on testing, but I have not found a way to do it yet. A unit test buys
very little, but we don't have a mechanism for forcing suspend/resume during an
integration test yet.

When I was writing suspend/resume I added a chaotic timer to randomly
suspend/resume, and ran all the media tests with it. We'll want something more
refined.

https://chromiumcodereview.appspot.com/1815013002/diff/1/media/base/pipeline_...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1815013002/diff/1/media/base/pipeline_...
media/base/pipeline_impl.cc:745: weak_factory_.GetWeakPtr(),
On 2016/03/17 23:11:15, xhwang wrote:
> It seems we are already calling GetWeakPtr() on two threads, this is unsafe
> according to:
> 
> https://codereview.chromium.org/1464183002#msg22
> 
> Actually a WeakPtrFactory can only be bound to one task runner and we should
> have DCHECK for it. So I am confused how things are working today?
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...

It's safe to call from two threads; the rule is that the weak pointers that
resulting pointers must be dereferenced from only one thread.

In this case I believe that Unretained(this) is safe because we own |renderer_|.
I'll just do that.

[sandersd (OOO until July 31), 2016-03-18 00:53:27]

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

[commit-bot: I haz the power, 2016-03-18 00:53:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1815013002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1815013002/20001

[commit-bot: I haz the power, 2016-03-18 00:57:49]

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

[commit-bot: I haz the power, 2016-03-18 00:57:50]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sandersd (OOO until July 31), 2016-03-18 02:03:11]

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

[commit-bot: I haz the power, 2016-03-18 02:03:35]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1815013002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1815013002/20001

[commit-bot: I haz the power, 2016-03-18 03:26:48]

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

[commit-bot: I haz the power, 2016-03-18 03:26:49]

Dry run: This issue passed the CQ dry run.

[xhwang, 2016-03-18 17:05:54]

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

[xhwang, 2016-03-18 17:05:55]

+wez: Please see the discussion about the thread safety of GetWeakPtr() call :)

https://chromiumcodereview.appspot.com/1815013002/diff/1/media/base/pipeline_...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1815013002/diff/1/media/base/pipeline_...
media/base/pipeline_impl.cc:745: weak_factory_.GetWeakPtr(),
On 2016/03/17 23:25:03, sandersd wrote:
> On 2016/03/17 23:11:15, xhwang wrote:
> > It seems we are already calling GetWeakPtr() on two threads, this is unsafe
> > according to:
> > 
> > https://codereview.chromium.org/1464183002#msg22
> > 
> > Actually a WeakPtrFactory can only be bound to one task runner and we should
> > have DCHECK for it. So I am confused how things are working today?
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
> 
> It's safe to call from two threads; 

+wez

GetWeakPtr() call is not thread safe according to previous discussion:
https://codereview.chromium.org/1464183002#msg22

wez: Could you please update the comments in weak_ptr.h to make this clear? It
has confused most people I think.

So here we probably should get a |weak_this| during construction time and add a
comment saying that it's bound to the media thread. Then we use |weak_this|
whenever we need a weak pointer.

> the rule is that the weak pointers that
> resulting pointers must be dereferenced from only one thread.

I see. I think today we only dereference the weak pointer on the media thread,
so we are good here.
 
> In this case I believe that Unretained(this) is safe because we own
|renderer_|.
> I'll just do that.

See my comments in the new code.

It seems using a weak pointer is actually safe because the callback should be
fired on the media thread.

https://chromiumcodereview.appspot.com/1815013002/diff/20001/media/base/pipel...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1815013002/diff/20001/media/base/pipel...
media/base/pipeline_impl.cc:746: cdm_attached_cb, cdm_context));
Typically when you use base::Unretained() you'll want to comment why it's safe.

That being said, how can you prevent the |renderer_| from posting the callback
instead of calling directly, and while the callback is waiting in the message
loop, the PipelineImpl and Renderer got destructed?

[Wez, 2016-03-18 19:49:28]

https://codereview.chromium.org/1815013002/diff/1/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/1815013002/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:745: weak_factory_.GetWeakPtr(),
On 2016/03/18 17:05:55, xhwang wrote:
> On 2016/03/17 23:25:03, sandersd wrote:
> > On 2016/03/17 23:11:15, xhwang wrote:
> > > It seems we are already calling GetWeakPtr() on two threads, this is
unsafe
> > > according to:
> > > 
> > > https://codereview.chromium.org/1464183002#msg22
> > > 
> > > Actually a WeakPtrFactory can only be bound to one task runner and we
should
> > > have DCHECK for it. So I am confused how things are working today?
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
> > 
> > It's safe to call from two threads;

GetWeakPtr is safe to call from different threads, but is NOT thread-safe - it
may be called on thread A and later on thread B, but not in parallel.
 
> +wez
> 
> GetWeakPtr() call is not thread safe according to previous discussion:
> https://codereview.chromium.org/1464183002#msg22
> 
> wez: Could you please update the comments in weak_ptr.h to make this clear? It
> has confused most people I think.

I have a CL in review w/ dcheng@ to clean that up, yes.

> So here we probably should get a |weak_this| during construction time and add
a
> comment saying that it's bound to the media thread. Then we use |weak_this|
> whenever we need a weak pointer.

This class already uses weak_factory_.GetWeakPtr() from both the caller and
|task_runner_| threads, which is bad for two reasons:
1. GetWeakPtr(), as has been noted, is not thread-safe.
2. If a coding error (e.g. in the caller) leads to one of the
Bind(...GetWeakPtr()) paths being executed _after_
weak_factory_.InvalidateWeakPtrs() then the new task(s) _will_ get executed.

#1 doesn't break this code because GetWeakPtr is in practice thread-safe once
there is an active WeakPtr - this code cannot call GetWeakPtr() on
|task_runner_| without first having called it on the caller thread.

#2 should be addressed as you suggest, by taking a WeakPtr at construction time,
to pass to Bind.

However, the design of this class is also overcomplicated by having it run on
both threads; it should be split into an outer PipelineImpl that runs only on
the caller thread, and a subcomponent object that runs only on the
|task_runner_|, and which contains the WeakPtrFactory, for example.

https://codereview.chromium.org/1815013002/diff/20001/media/base/pipeline_imp...
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/1815013002/diff/20001/media/base/pipeline_imp...
media/base/pipeline_impl.cc:746: cdm_attached_cb, cdm_context));
On 2016/03/18 17:05:55, xhwang wrote:
> Typically when you use base::Unretained() you'll want to comment why it's
safe.
> 
> That being said, how can you prevent the |renderer_| from posting the callback
> instead of calling directly, and while the callback is waiting in the message
> loop, the PipelineImpl and Renderer got destructed?

Agreed; the comment on SetCdm() implies that it may complete asynchronously. Is
it the case that it won't be fired if e.g. the caller deletes |renderer_| before
it completes, though?

[xhwang, 2016-03-18 20:05:34]

https://codereview.chromium.org/1815013002/diff/1/media/base/pipeline_impl.cc
File media/base/pipeline_impl.cc (right):

https://codereview.chromium.org/1815013002/diff/1/media/base/pipeline_impl.cc...
media/base/pipeline_impl.cc:745: weak_factory_.GetWeakPtr(),
On 2016/03/18 19:49:28, Wez wrote:
> On 2016/03/18 17:05:55, xhwang wrote:
> > On 2016/03/17 23:25:03, sandersd wrote:
> > > On 2016/03/17 23:11:15, xhwang wrote:
> > > > It seems we are already calling GetWeakPtr() on two threads, this is
> unsafe
> > > > according to:
> > > > 
> > > > https://codereview.chromium.org/1464183002#msg22
> > > > 
> > > > Actually a WeakPtrFactory can only be bound to one task runner and we
> should
> > > > have DCHECK for it. So I am confused how things are working today?
> > > > 
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/weak_p...
> > > 
> > > It's safe to call from two threads;
> 
> GetWeakPtr is safe to call from different threads, but is NOT thread-safe - it
> may be called on thread A and later on thread B, but not in parallel.
>  
> > +wez
> > 
> > GetWeakPtr() call is not thread safe according to previous discussion:
> > https://codereview.chromium.org/1464183002#msg22
> > 
> > wez: Could you please update the comments in weak_ptr.h to make this clear?
It
> > has confused most people I think.
> 
> I have a CL in review w/ dcheng@ to clean that up, yes.
> 
> > So here we probably should get a |weak_this| during construction time and
add
> a
> > comment saying that it's bound to the media thread. Then we use |weak_this|
> > whenever we need a weak pointer.
> 
> This class already uses weak_factory_.GetWeakPtr() from both the caller and
> |task_runner_| threads, which is bad for two reasons:
> 1. GetWeakPtr(), as has been noted, is not thread-safe.
> 2. If a coding error (e.g. in the caller) leads to one of the
> Bind(...GetWeakPtr()) paths being executed _after_
> weak_factory_.InvalidateWeakPtrs() then the new task(s) _will_ get executed.
> 
> #1 doesn't break this code because GetWeakPtr is in practice thread-safe once
> there is an active WeakPtr - this code cannot call GetWeakPtr() on
> |task_runner_| without first having called it on the caller thread.
> 
> #2 should be addressed as you suggest, by taking a WeakPtr at construction
time,
> to pass to Bind.
> 
> However, the design of this class is also overcomplicated by having it run on
> both threads;

This is one of the oldest piece of our code base and I agree that it needs to be
improved :)

> it should be split into an outer PipelineImpl that runs only on
> the caller thread, and a subcomponent object that runs only on the
> |task_runner_|, and which contains the WeakPtrFactory, for example.

Agreed. We talked about this and it's in our plan to split PipelineImpl to two
parts.

[sandersd (OOO until July 31), 2016-03-18 20:26:18]

https://chromiumcodereview.appspot.com/1815013002/diff/20001/media/base/pipel...
File media/base/pipeline_impl.cc (right):

https://chromiumcodereview.appspot.com/1815013002/diff/20001/media/base/pipel...
media/base/pipeline_impl.cc:746: cdm_attached_cb, cdm_context));
On 2016/03/18 19:49:28, Wez wrote:
> On 2016/03/18 17:05:55, xhwang wrote:
> > Typically when you use base::Unretained() you'll want to comment why it's
> safe.
> > 
> > That being said, how can you prevent the |renderer_| from posting the
callback
> > instead of calling directly, and while the callback is waiting in the
message
> > loop, the PipelineImpl and Renderer got destructed?
> 
> Agreed; the comment on SetCdm() implies that it may complete asynchronously.
Is
> it the case that it won't be fired if e.g. the caller deletes |renderer_|
before
> it completes, though?

This is safe because ~RendererImpl resets the callback, but xhwang@ is right
that we shouldn't depend on that. I've added a weak_this_ and updated all of the
code.

[sandersd (OOO until July 31), 2016-03-18 20:27:00]

Description was changed from

==========
Restore CDM context on Resume().

BUG=595898
==========

to

==========
Restore CDM context on Resume().

Also converts all uses of |weak_factory_| to use a |weak_this_|, as GetWeakPtr()
is not threadsafe.

BUG=595898
==========

[sandersd (OOO until July 31), 2016-03-18 20:33:25]

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

[commit-bot: I haz the power, 2016-03-18 20:33:53]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1815013002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1815013002/80001

[xhwang, 2016-03-18 20:51:23]

It would be nice to add a comment about adding tests somewhere. I think
suspend/resume+EME should be a pretty typical use case so it would be nice to
address that comment soon.

Otherwise LGTM. Thanks for fixing this!

[commit-bot: I haz the power, 2016-03-18 21:52:59]

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

[commit-bot: I haz the power, 2016-03-18 21:53:00]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2016-03-18 22:08:02]

On 2016/03/18 20:51:23, xhwang wrote:
> It would be nice to add a comment about adding tests somewhere. I think
> suspend/resume+EME should be a pretty typical use case so it would be nice to
> address that comment soon.
> 
> Otherwise LGTM. Thanks for fixing this!

I filed crbug.com/596191

[sandersd (OOO until July 31), 2016-03-18 22:08:25]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2016-03-18 22:08:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1815013002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1815013002/80001

[commit-bot: I haz the power, 2016-03-18 22:14:43]

Description was changed from

==========
Restore CDM context on Resume().

Also converts all uses of |weak_factory_| to use a |weak_this_|, as GetWeakPtr()
is not threadsafe.

BUG=595898
==========

to

==========
Restore CDM context on Resume().

Also converts all uses of |weak_factory_| to use a |weak_this_|, as GetWeakPtr()
is not threadsafe.

BUG=595898
==========

[commit-bot: I haz the power, 2016-03-18 22:14:45]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-03-18 22:16:06]

Description was changed from

==========
Restore CDM context on Resume().

Also converts all uses of |weak_factory_| to use a |weak_this_|, as GetWeakPtr()
is not threadsafe.

BUG=595898
==========

to

==========
Restore CDM context on Resume().

Also converts all uses of |weak_factory_| to use a |weak_this_|, as GetWeakPtr()
is not threadsafe.

BUG=595898

Committed: https://crrev.com/7655364f9ae672fdbd05e5751006387207b0bf51
Cr-Commit-Position: refs/heads/master@{#382109}
==========

[commit-bot: I haz the power, 2016-03-18 22:16:07]

Patchset 5 (id:??) landed as
https://crrev.com/7655364f9ae672fdbd05e5751006387207b0bf51
Cr-Commit-Position: refs/heads/master@{#382109}