Move ContentViewCore native ownership to WebContents

This means the nest of native objects (once built up) have the same life-times,
and the mismatch is handled at the JNI boundary.

GC might cause WebContents to get deleted before ContentViewCore (if AwContents is finalized before the java ContentViewCore)


BUG=

Review URL: https://chromiumcodereview.appspot.com/11067002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@162052 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/android/content_view_core_impl.cc

@@ -65,24 +65,66 @@ enum PopupItemType {
   POPUP_ITEM_TYPE_ENABLED
 };
 
+namespace content {
+
 namespace {
 jfieldID g_native_content_view;
+
+const void* kContentViewUserDataKey = &kContentViewUserDataKey;
 }  // namespace
 
-namespace content {
+// Enables a callback when the underlying WebContents is destroyed, to enable
+// nulling the back-pointer.
+class ContentViewCoreImpl::ContentViewUserData
+    : public base::SupportsUserData::Data {
+ public:
+  explicit ContentViewUserData(ContentViewCoreImpl* content_view_core)
+      : content_view_core_(content_view_core) {
+  }
+
+  virtual ~ContentViewUserData() {
+    // TODO(joth): When chrome has finished removing the TabContents class (see
+    // crbug.com/107201) consider inverting relationship, so ContentViewCore
+    // would own WebContents. That effectively implies making the WebContents
+    // destructor private on Android.
+    delete content_view_core_;
+  }
+
+  ContentViewCoreImpl* get() const { return content_view_core_; }
+
+ private:
+  // Not using scoped_ptr as ContentViewCoreImpl destructor is private.
+  ContentViewCoreImpl* content_view_core_;
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(ContentViewUserData);
+};
 
 struct ContentViewCoreImpl::JavaObject {
   ScopedJavaGlobalRef<jclass> rect_clazz;
   jmethodID rect_constructor;
 };
 
+// static
+ContentViewCoreImpl* ContentViewCoreImpl::FromWebContents(
+    content::WebContents* web_contents) {
+  ContentViewCoreImpl::ContentViewUserData* data =
+      reinterpret_cast<ContentViewCoreImpl::ContentViewUserData*>(
+          web_contents->GetUserData(kContentViewUserDataKey));
+  return data ? data->get() : NULL;
+}
+
+// static
+ContentViewCore* ContentViewCore::FromWebContents(
+    content::WebContents* web_contents) {
+  return ContentViewCoreImpl::FromWebContents(web_contents);
+}
+
 ContentViewCore* ContentViewCore::GetNativeContentViewCore(JNIEnv* env,
                                                            jobject obj) {
   return reinterpret_cast<ContentViewCore*>(
       env->GetIntField(obj, g_native_content_view));
 }
 
-
 ContentViewCoreImpl::ContentViewCoreImpl(JNIEnv* env, jobject obj,
                                          bool hardware_accelerated,
                                          WebContents* web_contents,
@@ -119,31 +161,42 @@ ContentViewCoreImpl::ContentViewCoreImpl(JNIEnv* env, jobject obj,
       webkit_glue::BuildUserAgentFromOSAndProduct(kLinuxInfoStr, product);
   web_contents->SetUserAgentOverride(spoofed_ua);
 
-  InitWebContents(web_contents);
+  InitWebContents();
 }
 
 ContentViewCoreImpl::~ContentViewCoreImpl() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> j_obj = java_ref_.get(env);
+  java_ref_.reset();
+  if (!j_obj.is_null()) {
+    Java_ContentViewCore_onNativeContentViewCoreDestroyed(
+        env, j_obj.obj(), reinterpret_cast<jint>(this));
+  }
   // Make sure nobody calls back into this object while we are tearing things
   // down.
   notification_registrar_.RemoveAll();
 
   delete java_object_;
   java_object_ = NULL;
+}
+
+void ContentViewCoreImpl::OnJavaContentViewCoreDestroyed(JNIEnv* env,
+                                                         jobject obj) {
+  DCHECK(env->IsSameObject(java_ref_.get(env).obj(), obj));
   java_ref_.reset();
 }
 
-void ContentViewCoreImpl::Destroy(JNIEnv* env, jobject obj) {
-  delete this;
-}
-
-void ContentViewCoreImpl::InitWebContents(WebContents* web_contents) {
-  web_contents_ = static_cast<WebContentsImpl*>(web_contents);
+void ContentViewCoreImpl::InitWebContents() {
+  DCHECK(web_contents_);
   notification_registrar_.Add(this,
       NOTIFICATION_RENDER_VIEW_HOST_CHANGED,
       Source<NavigationController>(&web_contents_->GetController()));
 
   static_cast<WebContentsViewAndroid*>(web_contents_->GetView())->
       SetContentViewCore(this);
+  DCHECK(!web_contents_->GetUserData(kContentViewUserDataKey));
+  web_contents_->SetUserData(kContentViewUserDataKey,
+                             new ContentViewUserData(this));
 }
 
 void ContentViewCoreImpl::Observe(int type,

content/browser/android/content_view_core_impl.h

@@ -35,6 +35,7 @@ class RenderWidgetHostViewAndroid;
 class ContentViewCoreImpl : public ContentViewCore,
                             public NotificationObserver {
  public:
+  static ContentViewCoreImpl* FromWebContents(WebContents* web_contents);
   ContentViewCoreImpl(JNIEnv* env,
                       jobject obj,
                       bool hardware_accelerated,
@@ -42,7 +43,6 @@ class ContentViewCoreImpl : public ContentViewCore,
                       ui::WindowAndroid* window_android);
 
   // ContentViewCore implementation.
-  virtual void Destroy(JNIEnv* env, jobject obj) OVERRIDE;
   virtual base::android::ScopedJavaLocalRef<jobject> GetJavaObject() OVERRIDE;
   virtual WebContents* GetWebContents() const OVERRIDE;
   virtual ui::WindowAndroid* GetWindowAndroid() OVERRIDE;
@@ -55,6 +55,8 @@ class ContentViewCoreImpl : public ContentViewCore,
   // Methods called from Java via JNI
   // --------------------------------------------------------------------------
 
+  void OnJavaContentViewCoreDestroyed(JNIEnv* env, jobject obj);
+
   // Notifies the ContentViewCore that items were selected in the currently
   // showing select popup.
   void SelectPopupMenuItems(JNIEnv* env, jobject obj, jintArray indices);
@@ -211,23 +213,22 @@ class ContentViewCoreImpl : public ContentViewCore,
   gfx::Rect GetBounds() const;
 
  private:
+  class ContentViewUserData;
+  friend class ContentViewUserData;
+  virtual ~ContentViewCoreImpl();
+
   // NotificationObserver implementation.
   virtual void Observe(int type,
                        const NotificationSource& source,
                        const NotificationDetails& details) OVERRIDE;
 
-  // --------------------------------------------------------------------------
-  // Private methods that call to Java via JNI
-  // --------------------------------------------------------------------------
-  virtual ~ContentViewCoreImpl();
-
   // --------------------------------------------------------------------------
   // Other private methods and data
   // --------------------------------------------------------------------------
 
   void InitJNI(JNIEnv* env, jobject obj);
 
-  void InitWebContents(content::WebContents* web_contents);
+  void InitWebContents();
 
   RenderWidgetHostViewAndroid* GetRenderWidgetHostViewAndroid();
 

content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java

@@ -40,7 +40,6 @@ import org.chromium.base.WeakContext;
 import org.chromium.content.app.AppResource;
 import org.chromium.content.browser.ContentViewGestureHandler.MotionEventDelegate;
 import org.chromium.content.browser.accessibility.AccessibilityInjector;
-import org.chromium.content.common.CleanupReference;
 import org.chromium.content.common.TraceEvent;
 import org.chromium.ui.gfx.NativeWindow;
 
@@ -132,19 +131,6 @@ public class ContentViewCore implements MotionEventDelegate {
         boolean super_awakenScrollBars(int startDelay, boolean invalidate);
     }
 
-    private static final class DestroyRunnable implements Runnable {
-        private final int mNativeContentViewCore;
-        private DestroyRunnable(int nativeContentViewCore) {
-            mNativeContentViewCore = nativeContentViewCore;
-        }
-        @Override
-        public void run() {
-            nativeDestroy(mNativeContentViewCore);
-        }
-    }
-
-    private CleanupReference mCleanupReference;
-
     private final Context mContext;
     private ViewGroup mContainerView;
     private InternalAccessDelegate mContainerViewInternals;
@@ -450,8 +436,6 @@ public class ContentViewCore implements MotionEventDelegate {
         mContainerView = containerView;
         mNativeContentViewCore = nativeInit(mHardwareAccelerated, nativeWebContents,
                 nativeWindow.getNativePointer());
-        mCleanupReference = new CleanupReference(
-                this, new DestroyRunnable(mNativeContentViewCore));
         mContentSettings = new ContentSettings(
                 this, mNativeContentViewCore, isAccessFromFileURLsGrantedByDefault);
         initializeContainerView(internalDispatcher);
@@ -478,6 +462,12 @@ public class ContentViewCore implements MotionEventDelegate {
         };
     }
 
+    @CalledByNative
+    void onNativeContentViewCoreDestroyed(int nativeContentViewCore) {
+        assert nativeContentViewCore == mNativeContentViewCore;
+        mNativeContentViewCore = 0;
+    }
+
     /**
      * Initializes the View that will contain all Views created by the ContentViewCore.
      *
@@ -566,7 +556,9 @@ public class ContentViewCore implements MotionEventDelegate {
      */
     public void destroy() {
         hidePopupDialog();
-        mCleanupReference.cleanupNow();
+        if (mNativeContentViewCore != 0) {
+            nativeOnJavaContentViewCoreDestroyed(mNativeContentViewCore);
+        }
         mNativeContentViewCore = 0;
         // Do not propagate the destroy() to settings, as the client may still hold a reference to
         // that and could still be using it.
@@ -2082,7 +2074,7 @@ public class ContentViewCore implements MotionEventDelegate {
     private native int nativeInit(boolean hardwareAccelerated, int webContentsPtr,
             int windowAndroidPtr);
 
-    private static native void nativeDestroy(int nativeContentViewCore);
+    private native void nativeOnJavaContentViewCoreDestroyed(int nativeContentViewCoreImpl);
 
     private native void nativeLoadUrl(
             int nativeContentViewCoreImpl,

content/public/browser/android/content_view_core.h

@@ -22,11 +22,10 @@ class WebContents;
 // public interface used by native code outside of the content module.
 class ContentViewCore {
  public:
-  static ContentViewCore* Create(
-      JNIEnv* env, jobject obj, WebContents* web_contents);
+  // Returns the existing ContentViewCore for |web_contents|, or NULL.
+  static ContentViewCore* FromWebContents(WebContents* web_contents);
   static ContentViewCore* GetNativeContentViewCore(JNIEnv* env, jobject obj);
 
-  virtual void Destroy(JNIEnv* env, jobject obj) = 0;
   virtual WebContents* GetWebContents() const = 0;
   virtual base::android::ScopedJavaLocalRef<jobject> GetJavaObject() = 0;
   virtual ui::WindowAndroid* GetWindowAndroid() = 0;