Fix an issue that an invalid surface could be passed to MediaPlayerBridge

Fix an issue that an invalid surface could be passed to MediaPlayerBridge

When entering fullscreen, setVideoSurface() can be called before
OnMediaPrepared().
It is then possible that user immediately exits fullscreen video.
In that case, when onMediaPrepared() is called, the surface is already
released.
We need to check whether a surface is valid or not before passing it to
MediaPlayer.

BUG=540990
Committed: https://crrev.com/ddcd5f8b7c909bd437576411498008d4450239da
Cr-Commit-Position: refs/heads/master@{#353219}

[qinmin, 2015-10-08 21:53:55]

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

[qinmin, 2015-10-08 21:53:55]

PTAL

[no sievers, 2015-10-08 23:00:27]

https://codereview.chromium.org/1398703003/diff/1/media/base/android/media_pl...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/1398703003/diff/1/media/base/android/media_pl...
media/base/android/media_player_bridge.cc:112: surface_ =  surface.IsValid() ?
surface.Pass() : gfx::ScopedJavaSurface();
Should we just make ScopedJavaSurface::MoveFrom() work for null surfaces? Since
it works for scoped_ptr, for example, right?

Also, less verbose, 'surface_ = surface' should work, I think.

https://codereview.chromium.org/1398703003/diff/1/ui/gl/android/scoped_java_s...
File ui/gl/android/scoped_java_surface.cc (right):

https://codereview.chromium.org/1398703003/diff/1/ui/gl/android/scoped_java_s...
ui/gl/android/scoped_java_surface.cc:83: return
JNI_Surface::Java_Surface_isValid(env, j_surface_.obj());
Are you sure we need to check this? Or is it good enough to reset surface_ to
null in SetVideoSurface (i.e. you other change)?

This *seems* odd, because we don't really do that anywhere else we deal with
Surfaces. Before it's released ContentVideoView.surfaceDestroyed() should cause
us to reset our reference.

[qinmin, 2015-10-08 23:47:23]

https://codereview.chromium.org/1398703003/diff/1/media/base/android/media_pl...
File media/base/android/media_player_bridge.cc (right):

https://codereview.chromium.org/1398703003/diff/1/media/base/android/media_pl...
media/base/android/media_player_bridge.cc:112: surface_ =  surface.IsValid() ?
surface.Pass() : gfx::ScopedJavaSurface();
On 2015/10/08 23:00:27, sievers wrote:
> Should we just make ScopedJavaSurface::MoveFrom() work for null surfaces?
Since
> it works for scoped_ptr, for example, right?
> 
> Also, less verbose, 'surface_ = surface' should work, I think.

with the move-only type, the right hand side has to have a .Pass().

https://codereview.chromium.org/1398703003/diff/1/ui/gl/android/scoped_java_s...
File ui/gl/android/scoped_java_surface.cc (right):

https://codereview.chromium.org/1398703003/diff/1/ui/gl/android/scoped_java_s...
ui/gl/android/scoped_java_surface.cc:83: return
JNI_Surface::Java_Surface_isValid(env, j_surface_.obj());
On 2015/10/08 23:00:27, sievers wrote:
> Are you sure we need to check this? Or is it good enough to reset surface_ to
> null in SetVideoSurface (i.e. you other change)?
> 
> This *seems* odd, because we don't really do that anywhere else we deal with
> Surfaces. Before it's released ContentVideoView.surfaceDestroyed() should
cause
> us to reset our reference.

removed this function. When surfaceDestroyed() is called, setVideoSurface(null)
should be called. In that case, we can just set surface_ to surface(which is
null).

[no sievers, 2015-10-09 00:10:51]

lgtm

[qinmin, 2015-10-09 00:24:06]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2015-10-09 00:25:10]

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

[commit-bot: I haz the power, 2015-10-09 03:04:05]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-09 03:05:37]

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