[Android WebView] Fix thread unsafety in accessing Java side getters

android_webview/java/src/org/chromium/android_webview/AwSettings.java

Index: android_webview/java/src/org/chromium/android_webview/AwSettings.java
diff --git a/android_webview/java/src/org/chromium/android_webview/AwSettings.java b/android_webview/java/src/org/chromium/android_webview/AwSettings.java
index 645ec6a6beb01986456cab9958833ca26d857e52..187fa8c3aae42cf0f3829f3b08c7103d8a6d12d3 100644
--- a/android_webview/java/src/org/chromium/android_webview/AwSettings.java
+++ b/android_webview/java/src/org/chromium/android_webview/AwSettings.java
@@ -131,6 +131,8 @@ public class AwSettings {
     private class EventHandler {
         // Message id for updating Webkit preferences
         private static final int UPDATE_WEBKIT_PREFERENCES = 0;
+        // Message id for running a Runnable

[benm (inactive), 2014/01/24 02:20:21]

nit: ... with the AwSettingsLock held.

[mnaganov (inactive), 2014/01/24 10:01:47]

Done.

+        private static final int RUN_RUNNABLE_BLOCKING = 1;
         // Actual UI thread handler
         private Handler mHandler;
 
@@ -150,14 +152,29 @@ public class AwSettings {
                                 mAwSettingsLock.notifyAll();
                             }
                             break;
+                        case RUN_RUNNABLE_BLOCKING:
+                            synchronized (mAwSettingsLock) {
+                                ((Runnable)msg.obj).run();
+                                mAwSettingsLock.notifyAll();
+                            }
+                            break;
                     }
                 }
             };
         }
 
         void maybeRunOnUiThreadBlocking(Runnable r) {
-            if (mHandler != null) {
-                ThreadUtils.runOnUiThreadBlocking(r);
+            assert Thread.holdsLock(mAwSettingsLock);
+            if (mHandler == null) return;
+            if (ThreadUtils.runningOnUiThread()) {
+                r.run();
+            } else {
+                mHandler.sendMessage(Message.obtain(null, RUN_RUNNABLE_BLOCKING, r));
+                try {
+                    mAwSettingsLock.wait();

[benm (inactive), 2014/01/24 02:20:21]

shouldn't we have this in a while loop and set a bit when the runnable executes
in the handler above?

[mnaganov (inactive), 2014/01/24 10:01:47]

Ah, of course! Even better -- fixing this makes updateWebkitPreferencesLocked to
be a special case of maybeRunOnUiThreadBlocking. Since we sync to native
synchronously after each preference change, the role of
mIsUpdateWebkitPrefsMessagePending to prevent sending superfluous messages is
obsolete -- it is only used for syncing between threads. So I can throw away
some duplicate code!

+                } catch (InterruptedException e) {
+                    Log.e(TAG, "Interrupted waiting a Runnable to complete", e);
+                }
             }
         }
 
@@ -238,6 +255,7 @@ public class AwSettings {
 
     @CalledByNative
     private double getDIPScaleLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mDIPScale;
     }
 
@@ -382,6 +400,7 @@ public class AwSettings {
 
     @CalledByNative
     private float getInitialPageScalePercentLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mInitialPageScalePercent;
     }
 
@@ -396,6 +415,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getSpatialNavigationLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mSpatialNavigationEnabled;
     }
 
@@ -410,6 +430,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getEnableSupportedHardwareAcceleratedFeaturesLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mEnableSupportedHardwareAcceleratedFeatures;
     }
 
@@ -472,6 +493,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getSaveFormDataLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mAutoCompleteEnabled;
     }
 
@@ -518,6 +540,7 @@ public class AwSettings {
 
     @CalledByNative
     private String getUserAgentLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mUserAgent;
     }
 
@@ -552,6 +575,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getLoadWithOverviewModeLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mLoadWithOverviewMode;
     }
 
@@ -579,6 +603,7 @@ public class AwSettings {
 
     @CalledByNative
     private int getTextSizePercentLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mTextSizePercent;
     }
 
@@ -605,6 +630,7 @@ public class AwSettings {
 
     @CalledByNative
     private String getStandardFontFamilyLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mStandardFontFamily;
     }
 
@@ -631,6 +657,7 @@ public class AwSettings {
 
     @CalledByNative
     private String getFixedFontFamilyLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mFixedFontFamily;
     }
 
@@ -657,6 +684,7 @@ public class AwSettings {
 
     @CalledByNative
     private String getSansSerifFontFamilyLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mSansSerifFontFamily;
     }
 
@@ -683,6 +711,7 @@ public class AwSettings {
 
     @CalledByNative
     private String getSerifFontFamilyLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mSerifFontFamily;
     }
 
@@ -709,6 +738,7 @@ public class AwSettings {
 
     @CalledByNative
     private String getCursiveFontFamilyLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mCursiveFontFamily;
     }
 
@@ -735,6 +765,7 @@ public class AwSettings {
 
     @CalledByNative
     private String getFantasyFontFamilyLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mFantasyFontFamily;
     }
 
@@ -762,6 +793,7 @@ public class AwSettings {
 
     @CalledByNative
     private int getMinimumFontSizeLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mMinimumFontSize;
     }
 
@@ -789,6 +821,7 @@ public class AwSettings {
 
     @CalledByNative
     private int getMinimumLogicalFontSizeLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mMinimumLogicalFontSize;
     }
 
@@ -816,6 +849,7 @@ public class AwSettings {
 
     @CalledByNative
     private int getDefaultFontSizeLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mDefaultFontSize;
     }
 
@@ -843,6 +877,7 @@ public class AwSettings {
 
     @CalledByNative
     private int getDefaultFixedFontSizeLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mDefaultFixedFontSize;
     }
 
@@ -905,6 +940,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getLoadsImagesAutomaticallyLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mLoadsImagesAutomatically;
     }
 
@@ -931,6 +967,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getImagesEnabledLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mImagesEnabled;
     }
 
@@ -945,6 +982,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getJavaScriptEnabledLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mJavaScriptEnabled;
     }
 
@@ -959,6 +997,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getAllowUniversalAccessFromFileURLsLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mAllowUniversalAccessFromFileURLs;
     }
 
@@ -973,6 +1012,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getAllowFileAccessFromFileURLsLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mAllowFileAccessFromFileURLs;
     }
 
@@ -1007,10 +1047,10 @@ public class AwSettings {
     /**
      * Return true if plugins are disabled.
      * @return True if plugins are disabled.
-     * @hide
      */
     @CalledByNative
     private boolean getPluginsDisabledLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mPluginState == PluginState.OFF;
     }
 
@@ -1047,6 +1087,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getJavaScriptCanOpenWindowsAutomaticallyLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mJavaScriptCanOpenWindowsAutomatically;
     }
 
@@ -1075,10 +1116,10 @@ public class AwSettings {
      * Gets whether Text Auto-sizing layout algorithm is enabled.
      *
      * @return true if Text Auto-sizing layout algorithm is enabled
-     * @hide
      */
     @CalledByNative
     private boolean getTextAutosizingEnabledLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mLayoutAlgorithm == LayoutAlgorithm.TEXT_AUTOSIZING;
     }
 
@@ -1105,11 +1146,13 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getSupportMultipleWindowsLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mSupportMultipleWindows;
     }
 
     @CalledByNative
     private boolean getSupportLegacyQuirksLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mSupportLegacyQuirks;
     }
 
@@ -1136,11 +1179,13 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getUseWideViewportLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mUseWideViewport;
     }
 
     @CalledByNative
     private boolean getPasswordEchoEnabledLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mPasswordEchoEnabled;
     }
 
@@ -1182,10 +1227,10 @@ public class AwSettings {
      * Gets whether Application Cache is enabled.
      *
      * @return true if Application Cache is enabled
-     * @hide
      */
     @CalledByNative
     private boolean getAppCacheEnabledLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         if (!mAppCacheEnabled) {
             return false;
         }
@@ -1217,6 +1262,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getDomStorageEnabledLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mDomStorageEnabled;
     }
 
@@ -1243,6 +1289,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getDatabaseEnabledLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mDatabaseEnabled;
     }
 
@@ -1269,6 +1316,7 @@ public class AwSettings {
 
     @CalledByNative
     private String getDefaultTextEncodingLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mDefaultTextEncoding;
     }
 
@@ -1295,6 +1343,7 @@ public class AwSettings {
 
     @CalledByNative
     private boolean getMediaPlaybackRequiresUserGestureLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mMediaPlaybackRequiresUserGesture;
     }
 
@@ -1322,6 +1371,7 @@ public class AwSettings {
 
     @CalledByNative
     private String getDefaultVideoPosterURLLocked() {
+        assert Thread.holdsLock(mAwSettingsLock);
         return mDefaultVideoPosterURL;
     }
 
@@ -1432,6 +1482,13 @@ public class AwSettings {
         }
     }
 
+    @CalledByNative
+    private void populateWebPreferences(long webPrefsPtr) {
+        synchronized (mAwSettingsLock) {
+            nativePopulateWebPreferencesLocked(mNativeAwSettings, webPrefsPtr);
+        }
+    }
+
     private void updateWebkitPreferencesOnUiThreadLocked() {
         if (mNativeAwSettings != 0) {
             assert mEventHandler.mHandler != null;
@@ -1444,6 +1501,8 @@ public class AwSettings {
 
     private native void nativeDestroy(long nativeAwSettings);
 
+    private native void nativePopulateWebPreferencesLocked(long nativeAwSettings, long webPrefsPtr);
+
     private native void nativeResetScrollAndScaleState(long nativeAwSettings);
 
     private native void nativeUpdateEverythingLocked(long nativeAwSettings);