Native ContentSettings is now owned by WebContents

This makes it consistent with how ContentViewCore now works, and also fixes a possible
GC leak (native content settings was holding a global ref on the java counterpart).
Also fixes up all usages so both sides become no-ops if the other part has been destroyed.
Also fix a couple small synchronization nits.

BUG=


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

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

4 files changed, 54 insertions, 53 deletions

diff --git a/content/browser/android/content_settings.cc b/content/browser/android/content_settings.cc
index fd59512..611c8c1 100644
--- a/content/browser/android/content_settings.cc
+++ b/content/browser/android/content_settings.cc
@@ -106,11 +106,17 @@ ContentSettings::ContentSettings(JNIEnv* env,
                          WebContents* contents,
                          bool is_master_mode)
     : WebContentsObserver(contents),
-      is_master_mode_(is_master_mode) {
-  content_settings_.Reset(env, obj);
+      is_master_mode_(is_master_mode),
+      content_settings_(env, obj) {
 }
 
 ContentSettings::~ContentSettings() {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  ScopedJavaLocalRef<jobject> obj = content_settings_.get(env);
+  if (obj.obj()) {
+    Java_ContentSettings_onNativeContentSettingsDestroyed(env, obj.obj(),
+        reinterpret_cast<jint>(this));
+  }
 }
 
 // static
@@ -118,17 +124,16 @@ bool ContentSettings::RegisterContentSettings(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
-void ContentSettings::Destroy(JNIEnv* env, jobject obj) {
-  delete this;
-}
-
 void ContentSettings::SyncFromNativeImpl() {
   JNIEnv* env = base::android::AttachCurrentThread();
   CHECK(env);
   if (!field_ids_.get())
     field_ids_.reset(new FieldIds(env));
 
-  jobject obj = content_settings_.obj();
+  ScopedJavaLocalRef<jobject> scoped_obj = content_settings_.get(env);
+  jobject obj = scoped_obj.obj();
+  if (!obj)
+    return;
   RenderViewHost* render_view_host = web_contents()->GetRenderViewHost();
   WebPreferences prefs = render_view_host->GetDelegate()->GetWebkitPrefs();
 
@@ -236,7 +241,10 @@ void ContentSettings::SyncToNativeImpl() {
   if (!field_ids_.get())
     field_ids_.reset(new FieldIds(env));
 
-  jobject obj = content_settings_.obj();
+  ScopedJavaLocalRef<jobject> scoped_obj = content_settings_.get(env);
+  jobject obj = scoped_obj.obj();
+  if (!obj)
+    return;
   RenderViewHost* render_view_host = web_contents()->GetRenderViewHost();
   WebPreferences prefs = render_view_host->GetDelegate()->GetWebkitPrefs();
 
@@ -332,6 +340,10 @@ void ContentSettings::RenderViewCreated(RenderViewHost* render_view_host) {
     SyncToNativeImpl();
 }
 
+void ContentSettings::WebContentsDestroyed(WebContents* web_contents) {
+  delete this;
+}
+
 static jint Init(JNIEnv* env, jobject obj, jint nativeContentViewCore,
                  jboolean is_master_mode) {
   WebContents* web_contents =
diff --git a/content/browser/android/content_settings.h b/content/browser/android/content_settings.h
index 4061e50..3a9128e 100644
--- a/content/browser/android/content_settings.h
+++ b/content/browser/android/content_settings.h
@@ -7,7 +7,7 @@
 
 #include <jni.h>
 
-#include "base/android/scoped_java_ref.h"
+#include "base/android/jni_helper.h"
 #include "base/memory/scoped_ptr.h"
 #include "content/public/browser/web_contents_observer.h"
 
@@ -18,12 +18,9 @@ class ContentSettings : public WebContentsObserver {
   ContentSettings(JNIEnv* env, jobject obj,
                   WebContents* contents,
                   bool is_master_mode);
-  virtual ~ContentSettings();
 
   static bool RegisterContentSettings(JNIEnv* env);
 
-  void Destroy(JNIEnv* env, jobject obj);
-
   // Synchronizes the Java settings from native settings.
   void SyncFromNative(JNIEnv* env, jobject obj);
   // Synchronizes the native settings from Java settings.
@@ -31,12 +28,15 @@ class ContentSettings : public WebContentsObserver {
 
  private:
   struct FieldIds;
+  // Self-deletes when the underlying WebContents is destroyed.
+  virtual ~ContentSettings();
 
   void SyncFromNativeImpl();
   void SyncToNativeImpl();
 
   // WebContentsObserver overrides:
   virtual void RenderViewCreated(RenderViewHost* render_view_host) OVERRIDE;
+  virtual void WebContentsDestroyed(WebContents* web_contents) OVERRIDE;
 
   // Determines whether a sync to native should be triggered when a new render
   // view is created.
@@ -46,7 +46,7 @@ class ContentSettings : public WebContentsObserver {
   scoped_ptr<FieldIds> field_ids_;
 
   // The Java counterpart to this class.
-  base::android::ScopedJavaGlobalRef<jobject> content_settings_;
+  JavaObjectWeakGlobalRef content_settings_;
 };
 
 }  // namespace content
diff --git a/content/public/android/java/src/org/chromium/content/browser/ContentSettings.java b/content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
index fa79fb6..416bdcd 100644
--- a/content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
+++ b/content/public/android/java/src/org/chromium/content/browser/ContentSettings.java
@@ -13,7 +13,6 @@ import android.webkit.WebView;
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
 import org.chromium.base.ThreadUtils;
-import org.chromium.content.common.CleanupReference;
 
 /**
  * Manages settings state for a ContentView. A ContentSettings instance is obtained
@@ -29,23 +28,12 @@ public class ContentSettings {
     // used from any thread. Internally, the class uses a message queue
     // to call native code on the UI thread only.
 
+    // The native side of this object. Ownership is retained native-side by the WebContents
+    // instance that backs the associated ContentViewCore.
     private int mNativeContentSettings = 0;
 
     private ContentViewCore mContentViewCore;
 
-    private static final class DestroyRunnable implements Runnable {
-        private int mNativeContentSettings;
-        private DestroyRunnable(int nativeContentSettings) {
-            mNativeContentSettings = nativeContentSettings;
-        }
-        @Override
-        public void run() {
-            nativeDestroy(mNativeContentSettings);
-        }
-    }
-
-    private final CleanupReference mCleanupReference;
-
     // When ContentView is used in PERSONALITY_CHROME mode, settings can't
     // be modified through the ContentSettings instance.
     private boolean mCanModifySettings;
@@ -107,37 +95,33 @@ public class ContentSettings {
 
         EventHandler() {
             mHandler = mContentViewCore.isPersonalityView() ?
-                    new Handler() {
+                    new Handler(Looper.getMainLooper()) {
                         @Override
                         public void handleMessage(Message msg) {
                             switch (msg.what) {
                                 case SYNC:
                                     synchronized (mContentSettingsLock) {
-                                        nativeSyncToNative(mNativeContentSettings);
+                                        syncToNativeOnUiThread();
                                         mIsSyncMessagePending = false;
-                                        mContentSettingsLock.notify();
+                                        mContentSettingsLock.notifyAll();
                                     }
                                     break;
                                 case UPDATE_UA:
-                                    synchronized (mContentViewCore) {
-                                        mContentViewCore.setAllUserAgentOverridesInHistory();
-                                    }
+                                    mContentViewCore.setAllUserAgentOverridesInHistory();
                                     break;
                                 case UPDATE_MULTI_TOUCH:
-                                    synchronized (mContentViewCore) {
-                                        mContentViewCore.updateMultiTouchZoomSupport();
-                                    }
+                                    mContentViewCore.updateMultiTouchZoomSupport();
                                     break;
                             }
                         }
                     } :
-                    new Handler() {
+                    new Handler(Looper.getMainLooper()) {
                         @Override
                         public void handleMessage(Message msg) {
                             switch (msg.what) {
                                 case SYNC:
                                     synchronized (mContentSettingsLock) {
-                                        nativeSyncFromNative(mNativeContentSettings);
+                                        syncFromNativeOnUiThread();
                                         mIsSyncMessagePending = false;
                                     }
                                     break;
@@ -148,9 +132,10 @@ public class ContentSettings {
 
         private void syncSettingsLocked() {
             assert Thread.holdsLock(mContentSettingsLock);
+            if (mNativeContentSettings == 0) return;
             if (mContentViewCore.isPersonalityView()) {
                 if (Looper.myLooper() == mHandler.getLooper()) {
-                    nativeSyncToNative(mNativeContentSettings);
+                    syncToNativeOnUiThread();
                 } else {
                     // We're being called on a background thread, so post a message.
                     if (mIsSyncMessagePending) {
@@ -178,11 +163,13 @@ public class ContentSettings {
 
         private void sendUpdateUaMessageLocked() {
             assert Thread.holdsLock(mContentSettingsLock);
+            if (mNativeContentSettings == 0) return;
             mHandler.sendMessage(Message.obtain(null, UPDATE_UA));
         }
 
         private void sendUpdateMultiTouchMessageLocked() {
             assert Thread.holdsLock(mContentSettingsLock);
+            if (mNativeContentSettings == 0) return;
             mHandler.sendMessage(Message.obtain(null, UPDATE_MULTI_TOUCH));
         }
     }
@@ -198,8 +185,6 @@ public class ContentSettings {
         mCanModifySettings = mContentViewCore.isPersonalityView();
         mNativeContentSettings = nativeInit(nativeContentView, mCanModifySettings);
         assert mNativeContentSettings != 0;
-        mCleanupReference = new CleanupReference(this,
-                new DestroyRunnable(mNativeContentSettings));
 
         if (isAccessFromFileURLsGrantedByDefault) {
             mAllowUniversalAccessFromFileURLs = true;
@@ -211,24 +196,24 @@ public class ContentSettings {
             // PERSONALITY_VIEW
             mDefaultUserAgent = nativeGetDefaultUserAgent();
             mUserAgent = mDefaultUserAgent;
-            nativeSyncToNative(mNativeContentSettings);
+            syncToNativeOnUiThread();
         } else {
             // PERSONALITY_CHROME
             // Chrome has zooming enabled by default. These settings are not
             // set by the native code.
             mBuiltInZoomControls = true;
             mDisplayZoomControls = false;
-            nativeSyncFromNative(mNativeContentSettings);
+            syncFromNativeOnUiThread();
         }
     }
 
     /**
-     * Destroys the native side of the ContentSettings. This ContentSettings object
-     * cannot be used after this method has been called. Should only be called
-     * when related ContentView is destroyed.
+     * Notification from the native side that it is being destroyed.
+     * @param nativeContentSettings the native instance that is going away.
      */
-    void destroy() {
-        mCleanupReference.cleanupNow();
+    @CalledByNative
+    private void onNativeContentSettingsDestroyed(int nativeContentSettings) {
+        assert mNativeContentSettings == nativeContentSettings;
         mNativeContentSettings = 0;
     }
 
@@ -946,11 +931,17 @@ public class ContentSettings {
         }
     }
 
+    void syncToNativeOnUiThread() {
+        if (mNativeContentSettings != 0) nativeSyncToNative(mNativeContentSettings);
+    }
+
+    void syncFromNativeOnUiThread() {
+        if (mNativeContentSettings != 0) nativeSyncFromNative(mNativeContentSettings);
+    }
+
     // Initialize the ContentSettings native side.
     private native int nativeInit(int contentViewPtr, boolean isMasterMode);
 
-    private static native void nativeDestroy(int nativeContentSettings);
-
     private static native String nativeGetDefaultUserAgent();
 
     // Synchronize Java settings from native settings.
diff --git a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
index 2c3d026..b077e45 100644
--- a/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
+++ b/content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java
@@ -560,8 +560,6 @@ public class ContentViewCore implements MotionEventDelegate {
             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.
         mContentSettings = null;
         mVSyncMonitor.stop();
     }
@@ -1842,7 +1840,7 @@ public class ContentViewCore implements MotionEventDelegate {
      */
     @Override
     public void invokeZoomPicker() {
-        if (mContentSettings.supportZoom()) {
+        if (mContentSettings != null && mContentSettings.supportZoom()) {
             mZoomManager.invokeZoomPicker();
         }
     }