Decouple MediaSession messages from WebContents (full patch)

content/browser/media/session/media_session_android.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CONTENT_BROWSER_MEDIA_SESSION_MEDIA_SESSION_ANDROID_H_
+#define CONTENT_BROWSER_MEDIA_SESSION_MEDIA_SESSION_ANDROID_H_
+
+#include <jni.h>
+#include <memory>
+#include <vector>
+
+#include "base/android/scoped_java_ref.h"
+#include "content/public/browser/media_session_observer.h"
+
+namespace content {
+
+class MediaSessionImpl;
+
+bool RegisterMediaSessionNatives(JNIEnv* env);

[boliu, 2016/10/29 01:11:28]

remove

[Zhiqiang Zhang (Slow), 2016/10/31 11:57:34]

Done.

+
+// This class is interlayer between native MediaSession and Java
+// MediaSession. This class is owned by the native MediaSession and will
+// teardown Java MediaSession when the native MediaSession is destroyed.
+// Java MediaSessionObservers are also proxied via this class.
+class MediaSessionAndroid final : public MediaSessionObserver {
+ public:
+  static bool Register(JNIEnv* env);
+
+  explicit MediaSessionAndroid(MediaSession* session);
+  ~MediaSessionAndroid() override;
+
+  base::android::ScopedJavaLocalRef<jobject> GetJavaObject();

[boliu, 2016/10/29 01:11:28]

Leaking the java object outside of class is generally a bad pattern
(unfortunately we do it all over the place)

in this case, it can be avoided by friending the jni function

[Zhiqiang Zhang (Slow), 2016/10/31 11:57:34]

Ugh, I tried but failed. The problem is that the jni function is a static
function (declared in the generated jni header), so it has to be declared before
MediaSessionAndroid as a static function. Then all other files including
media_session_impl.h will get compilation failure saying "static ...(...)" is
declared but not defined (-Werror=unused-function).

I added a comment above this method to alert users to take care.

Maybe have some non-copyable JavaLocalRef like unique_ptr<>? That may prevent
leaking the reference.

Or maybe I think there are some limitations in the JNI generator?  If Java can
call a static member method of a class, we can eliminate some need for
non-class-member "GetJavaObject()".

[boliu, 2016/10/31 18:58:55]

Oh, static :/

You could declare another function, so static function calls this function calls
GetJavaObject, but that's like back to the original problem of exposing that
other function.

One step further would be to forward declare an inner class/struct, which can
hide more details...

Or alternative is to implement it java, but java WebContents doesn't have a
UserData equivalent afaik, so maybe not.

Inner class/struct it is then?

[Zhiqiang Zhang (Slow), 2016/10/31 22:22:26]

Done. But actually I found other files including the header can define the inner
struct themselves while not triggering a redefinition error. So basically this
solution is making leaking the object harder but still possible. 
Not sure if it can be caught by more strict compiler/linker checks :/

[boliu, 2016/10/31 22:44:02]

Really? I'd expect a link error. Perhaps a runtime error if you are building
component build and your definitions are in different shared libraries..
You said for the other comment that java WebContentsImpl is holding the java
side, maybe you should just implement this on the java side as well?

But this is ok too

[Zhiqiang Zhang (Slow), 2016/10/31 23:30:28]

Maybe... 
WebContentsImpl is currently package-scoped (see its declaration) so I cannot do
it for now.
I left a comment in MediaSession.fromWebContents()

+
+  // MediaSessionObserver implementation.
+  void MediaSessionDestroyed() override;
+  void MediaSessionStateChanged(bool is_controllable,
+                                bool is_suspended) override;
+  void MediaSessionMetadataChanged(
+      const base::Optional<MediaMetadata>& metadata) override;
+
+  // MediaSession method wrappers.
+  void Resume(JNIEnv* env, const base::android::JavaParamRef<jobject>& j_obj);
+  void Suspend(JNIEnv* env, const base::android::JavaParamRef<jobject>& j_obj);
+  void Stop(JNIEnv* env, const base::android::JavaParamRef<jobject>& j_obj);
+
+ private:
+  // The linked Java object
+  base::android::ScopedJavaGlobalRef<jobject> j_media_session_;

[boliu, 2016/10/29 01:11:28]

I raised the point about strong reference from native earlier. Don't remember
getting an answer though, if onDetachedFromWindow guarantees this object will be
deleted?

fwiw WebContentsImpl.java/etc is already wrong here, and should be fixed. don't
make this wrong too :)

https://cs.chromium.org/chromium/src/content/public/android/java/src/org/chro...
https://cs.chromium.org/chromium/src/android_webview/java/src/org/chromium/an...

[Zhiqiang Zhang (Slow), 2016/10/31 11:57:34]

I don't know about how GC on WebContentsImpl.java works, but I can only explain
the link chain for MediaSession and let you decide. The link chain is:
WebContentsImpl(native) -> MediaSession(native) -> MediaSessionAndroid(native)
-> MediaSessionImpl.Java
When WebContentsImpl(native) is destroyed, all the native objects on the chain
will be destroyed.

The Java user (non-WebView) will has the following chain:
Tab.java<->MediaSessionTabHelper.java (observer)<->MediaSessionImpl.java
Also I think the link MediaSessionTabHelper->Tab can be changed to
WeakReference.

Is there a possible leak on this? Or maybe we should make
MediaSessionTabHelper->MediaSessionImpl weak if yes.

I think "a Java user referencing MediaSessionImpl.java" is safer than "a Java
user referencing WebContentsImpl.java". If the user does't reference too high on
the reference chain, it should be OK.

[boliu, 2016/10/31 18:58:55]

Yes in webview, where native WebContentsImpl destruction relies on gc, so a
strong reference from native (ie a gc root) can't transitively reference
WebContents and prevent it from garbage collected.
Be very careful that *something* is keeping the MediaSessionImpl alive on the
java side (java references are generally not gc roots, other than static and
local references) while the WebContents is alive. Is there something like that?

[Zhiqiang Zhang (Slow), 2016/10/31 19:50:03]

I think the problem lies in the client side.

If we use weak reference here, then we still need to find some object to hold a
reference of the MediaSessionImpl.java, otherwise it will be garbage collected
when we still want to use it. WebContentsImpl.java could possibly hold the
MediaSesssionImpl.java reference, but that still cannot prevent having a
reference cycle if the observer prevents WebContents being gc'ed.

So I think if the client is causing a leak, it doesn't matter we use a
strong/weak reference here.

[boliu, 2016/10/31 20:13:19]

Meaning outside of content?
Yes exactly.
I think you might be misunderstanding how garbage collection works.

Reference cycles are fine. gc is able to figure out the whole cycle is no longer
reachable from a gc root, and then collect them all in one go (this of course is
different from ref counting)

It's also ok for java WebContentsImpl/MediaSesssionImpl/MediaSessionObserver to
have strong references between each other without causing gc issues, since none
of those references are gc roots.

If you are worried client introducing *another* gc root in client code, then
that's always a possibility. But at least the problem is not with this code.

[Zhiqiang Zhang (Slow), 2016/10/31 22:22:26]

Yes.
So IIUC WebContentsAndroid is holding a gc root of WebContentsImpl.java which it
should not, right?

Now I should go with having weak reference here and let WebContentsImpl.java
hold the reference.

[boliu, 2016/10/31 22:44:02]

That's a good point. Yes it will still be referenced by WebContentsImpl's
existing gc root. But overall, the point here is don't introduce *another* gc
root.

[Zhiqiang Zhang (Slow), 2016/10/31 23:30:28]

Acknowledged.

+  // The native MediaSession object
+  MediaSessionImpl* media_session_;
+
+  DISALLOW_COPY_AND_ASSIGN(MediaSessionAndroid);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_MEDIA_SESSION_MEDIA_SESSION_ANDROID_H_