Another fix for MediaSessionAndroid DCHECK on destruction

Another fix for MediaSessionAndroid DCHECK on destruction

This CL removes the unnecessary DCHECK for the Java object
in MediaSessionAndroid destruction. This should fix an flaky test
caused by hitting this DCHECK() in some situations.

BUG=661444
Committed: https://crrev.com/a987baac512327c28cf37237f53020a23aa53cc4
Cr-Commit-Position: refs/heads/master@{#429757}

[Zhiqiang Zhang (Slow), 2016-11-03 23:32:22]

zqzhang@chromium.org changed reviewers:
+ boliu@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-03 23:32:22]

PTAL

Sorry the last fix didn't fully solves the issue.
The remaining flakes should be caused by GC happening after WebContentsImpl.java
and before content::MediaSessionImpl is destroyed. i.e. happens even earlier so
that MediaSessionDestroyed was not called yet. Therefore the weak reference in
MediaSessionAndroid is not reset.

[Zhiqiang Zhang (Slow), 2016-11-03 23:35:09]

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

[commit-bot: I haz the power, 2016-11-03 23:36:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[boliu, 2016-11-03 23:43:14]

> The issue is that the Java MediaSessionImpl may be garbage-collected
> after Java WebContentsImpl is garbage-collected and before native
> MediaSessionImpl/MediaSessionAndroid is destroyed.

I don't see how that's a problem.

It looks like the real issue is MediaSessionDestroyed isn't actually guaranteed
to be called before MediaSessionAndroid is destroyed, and the DCHECK is catching
a real problem. Either it's not actually a problem, so just remove the DCHECK,
or if it is a problem, then fix it.

https://codereview.chromium.org/2479703002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java
(right):

https://codereview.chromium.org/2479703002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:121:
protected void finalize() {
never ever ever use finalizers

[Zhiqiang Zhang (Slow), 2016-11-04 00:22:06]

On 2016/11/03 23:43:14, boliu wrote:
> > The issue is that the Java MediaSessionImpl may be garbage-collected
> > after Java WebContentsImpl is garbage-collected and before native
> > MediaSessionImpl/MediaSessionAndroid is destroyed.
> 
> I don't see how that's a problem.
> 
> It looks like the real issue is MediaSessionDestroyed isn't actually
guaranteed
> to be called before MediaSessionAndroid is destroyed, and the DCHECK is
catching
> a real problem. Either it's not actually a problem, so just remove the DCHECK,
> or if it is a problem, then fix it.
> 
OK, removing the DCHECK is safe here I think. Done.

Thanks for the review :) Otherwise I'll be introducing new bugs...

https://codereview.chromium.org/2479703002/diff/20001/content/public/android/...
File
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java
(right):

https://codereview.chromium.org/2479703002/diff/20001/content/public/android/...
content/public/android/java/src/org/chromium/content/browser/MediaSessionImpl.java:121:
protected void finalize() {
On 2016/11/03 23:43:14, boliu wrote:
> never ever ever use finalizers

OK. Undone this change.

Actually I think we don't need this finalizer, since the observers keeps a
strong reference to MediaSessionImpl so if an observer are still observing this
MediaSessionImpl, and the observer is reachable from a GC root (i.e. the
observer is still in use), MediaSessionImpl shall not be GC'ed.

[boliu, 2016-11-04 00:42:00]

lgtm

https://codereview.chromium.org/2479703002/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_android.cc (right):

https://codereview.chromium.org/2479703002/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.cc:66:
j_media_session_.reset();
you can drop this as well then

[Zhiqiang Zhang (Slow), 2016-11-04 00:47:41]

Description was changed from

==========
Another fix for MediaSessionAndroid DCHECK on destruction

This CL should fix an issue which a previous CL did not fully resolved.
The issue is that the Java MediaSessionImpl may be garbage-collected
after Java WebContentsImpl is garbage-collected and before native
MediaSessionImpl/MediaSessionAndroid is destroyed.

This CL handles both cases where the Java MediaSession is
garbage-collected first or the native MediaSession is destroyed first.

BUG=661444
==========

to

==========
Another fix for MediaSessionAndroid DCHECK on destruction

This CL removes the unnecessary DCHECK for the Java object
in MediaSessionAndroid destruction. This should fix an flaky test
caused by hitting this DCHECK() in some situations.

BUG=661444
==========

[Zhiqiang Zhang (Slow), 2016-11-04 00:47:56]

https://codereview.chromium.org/2479703002/diff/40001/content/browser/media/s...
File content/browser/media/session/media_session_android.cc (right):

https://codereview.chromium.org/2479703002/diff/40001/content/browser/media/s...
content/browser/media/session/media_session_android.cc:66:
j_media_session_.reset();
On 2016/11/04 00:42:00, boliu wrote:
> you can drop this as well then

Done.

[Zhiqiang Zhang (Slow), 2016-11-04 00:48:06]

The CQ bit was checked by zqzhang@chromium.org

[Zhiqiang Zhang (Slow), 2016-11-04 00:48:06]

The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org
Link to the patchset: https://codereview.chromium.org/2479703002/#ps60001
(title: "nit")

[commit-bot: I haz the power, 2016-11-04 00:49:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-04 01:39:11]

Description was changed from

==========
Another fix for MediaSessionAndroid DCHECK on destruction

This CL removes the unnecessary DCHECK for the Java object
in MediaSessionAndroid destruction. This should fix an flaky test
caused by hitting this DCHECK() in some situations.

BUG=661444
==========

to

==========
Another fix for MediaSessionAndroid DCHECK on destruction

This CL removes the unnecessary DCHECK for the Java object
in MediaSessionAndroid destruction. This should fix an flaky test
caused by hitting this DCHECK() in some situations.

BUG=661444
==========

[commit-bot: I haz the power, 2016-11-04 01:39:11]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-04 01:42:28]

Description was changed from

==========
Another fix for MediaSessionAndroid DCHECK on destruction

This CL removes the unnecessary DCHECK for the Java object
in MediaSessionAndroid destruction. This should fix an flaky test
caused by hitting this DCHECK() in some situations.

BUG=661444
==========

to

==========
Another fix for MediaSessionAndroid DCHECK on destruction

This CL removes the unnecessary DCHECK for the Java object
in MediaSessionAndroid destruction. This should fix an flaky test
caused by hitting this DCHECK() in some situations.

BUG=661444

Committed: https://crrev.com/a987baac512327c28cf37237f53020a23aa53cc4
Cr-Commit-Position: refs/heads/master@{#429757}
==========

[commit-bot: I haz the power, 2016-11-04 01:42:29]

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