From e93d9b009f39dcc5ba220b073b53f6897a13fbe3 Mon Sep 17 00:00:00 2001 From: "joth@chromium.org" Date: Wed, 17 Oct 2012 23:54:02 +0000 Subject: 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 --- content/browser/android/content_settings.cc | 28 +++++++--- content/browser/android/content_settings.h | 10 ++-- .../chromium/content/browser/ContentSettings.java | 65 ++++++++++------------ .../chromium/content/browser/ContentViewCore.java | 4 +- 4 files changed, 54 insertions(+), 53 deletions(-) (limited to 'content') 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 obj = content_settings_.get(env); + if (obj.obj()) { + Java_ContentSettings_onNativeContentSettingsDestroyed(env, obj.obj(), + reinterpret_cast(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 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 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 -#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 field_ids_; // The Java counterpart to this class. - base::android::ScopedJavaGlobalRef 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(); } } -- cgit v1.1