aw: Avoid uncontrolled video context destruction

aw: Avoid uncontrolled video context destruction

Since r289474, the sync factory nulls out the video context
which means the destruction is uncontrolled. Context
destruction is synchronous, so can cause deadlocks if there
are not webviews attached to view tree to run GL.

This is a rewrite of r289474 that just returns null
VideoContextProvider if no webview is ready to use it. But
still hold on to the reference to the context to avoid
destruction.

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

[boliu, 2014-08-22 17:14:20]

ptal

[boliu, 2014-08-22 17:23:14]

On 2014/08/22 17:14:20, boliu wrote:
> ptal

And my bad for not thinking about this case when reviewing the original fix.

[hush (inactive), 2014-08-22 17:45:30]

On 2014/08/22 17:23:14, boliu wrote:
> On 2014/08/22 17:14:20, boliu wrote:
> > ptal
> 
> And my bad for not thinking about this case when reviewing the original fix.

This means the ref count for the video context is always larger than 0? Until
the synchronous_compositor_factory gets destructed.
Does that mean the video context stays alive until application quits?

[boliu, 2014-08-22 17:46:05]

On 2014/08/22 17:45:30, hush wrote:
> On 2014/08/22 17:23:14, boliu wrote:
> > On 2014/08/22 17:14:20, boliu wrote:
> > > ptal
> > 
> > And my bad for not thinking about this case when reviewing the original fix.
> 
> This means the ref count for the video context is always larger than 0? Until
> the synchronous_compositor_factory gets destructed.
> Does that mean the video context stays alive until application quits?

Yes and yes. Same behavior as before r289474

[hush (inactive), 2014-08-22 17:49:38]

https://codereview.chromium.org/498623002/diff/1/content/browser/android/in_p...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/498623002/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:250:
scoped_refptr<StreamTextureFactorySynchronousImpl::ContextProvider>();
can we just return null here?

[boliu, 2014-08-22 17:54:17]

https://codereview.chromium.org/498623002/diff/1/content/browser/android/in_p...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/498623002/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:250:
scoped_refptr<StreamTextureFactorySynchronousImpl::ContextProvider>();
On 2014/08/22 17:49:38, hush wrote:
> can we just return null here?

Hmm, compiler seems happy, but not sure if it fits chromium style

[boliu, 2014-08-22 18:14:30]

https://codereview.chromium.org/498623002/diff/1/content/browser/android/in_p...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/498623002/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:250:
scoped_refptr<StreamTextureFactorySynchronousImpl::ContextProvider>();
On 2014/08/22 17:54:17, boliu wrote:
> On 2014/08/22 17:49:38, hush wrote:
> > can we just return null here?
> 
> Hmm, compiler seems happy, but not sure if it fits chromium style

Took a cursory look and didn't find any instance of "return NULL" used on a
ref-counted type.

[hush (inactive), 2014-08-22 18:26:43]

https://codereview.chromium.org/498623002/diff/1/content/browser/android/in_p...
File content/browser/android/in_process/synchronous_compositor_factory_impl.cc
(right):

https://codereview.chromium.org/498623002/diff/1/content/browser/android/in_p...
content/browser/android/in_process/synchronous_compositor_factory_impl.cc:250:
scoped_refptr<StreamTextureFactorySynchronousImpl::ContextProvider>();
alright.
lgtm
sorry that I broke this thing...
On 2014/08/22 18:14:30, boliu wrote:
> On 2014/08/22 17:54:17, boliu wrote:
> > On 2014/08/22 17:49:38, hush wrote:
> > > can we just return null here?
> > 
> > Hmm, compiler seems happy, but not sure if it fits chromium style
> 
> Took a cursory look and didn't find any instance of "return NULL" used on a
> ref-counted type.

[boliu, 2014-08-22 18:50:28]

Daniel? :p

[no sievers, 2014-08-22 21:03:34]

lgtm

[boliu, 2014-08-22 21:06:27]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-08-22 21:07:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/498623002/1

[commit-bot: I haz the power, 2014-08-22 22:17:31]

Committed patchset #1 (1) as 291527