Cherry-pick: Deny the geolocation permission request by default.

michaelbai · Commit bot · commit 709a38766d8e · 2015-08-05T18:38:05.000Z
This patch denies the request if there is no response when GeolocationPermission.Callback object is gc-ed. TBR=boliu BUG=513752 NOTRY=true NOPRESUBMIT=true Review URL: https://codereview.chromium.org/1267103003 Cr-Commit-Position: refs/heads/master@{#341760} (cherry picked from commit 97d22bb) Review URL: https://codereview.chromium.org/1277563002 Cr-Commit-Position: refs/branch-heads/2454@{#235} Cr-Branched-From: 12bfc33-refs/heads/master@{#338390}
diff --git a/android_webview/java/src/org/chromium/android_webview/AwContents.java b/android_webview/java/src/org/chromium/android_webview/AwContents.java
@@ -37,11 +37,11 @@
 import android.view.animation.AnimationUtils;
 import android.view.inputmethod.EditorInfo;
 import android.view.inputmethod.InputConnection;
-import android.webkit.GeolocationPermissions;
 import android.webkit.JavascriptInterface;
 import android.webkit.ValueCallback;
 import android.widget.OverScroller;
 
+import org.chromium.android_webview.permission.AwGeolocationCallback;
 import org.chromium.android_webview.permission.AwPermissionRequest;
 import org.chromium.base.CalledByNative;
 import org.chromium.base.JNINamespace;
@@ -2410,25 +2410,13 @@ private void onReceivedHttpAuthRequest(AwHttpAuthHandler handler, String host, S
         mContentsClient.onReceivedHttpAuthRequest(handler, host, realm);
     }
 
-    private class AwGeolocationCallback implements GeolocationPermissions.Callback {
+    public AwGeolocationPermissions getGeolocationPermissions() {
+        return mBrowserContext.getGeolocationPermissions();
+    }
 
-        @Override
-        public void invoke(final String origin, final boolean allow, final boolean retain) {
-            ThreadUtils.runOnUiThread(new Runnable() {
-                @Override
-                public void run() {
-                    if (retain) {
-                        if (allow) {
-                            mBrowserContext.getGeolocationPermissions().allow(origin);
-                        } else {
-                            mBrowserContext.getGeolocationPermissions().deny(origin);
-                        }
-                    }
-                    if (isDestroyed()) return;
-                    nativeInvokeGeolocationCallback(mNativeAwContents, allow, origin);
-                }
-            });
-        }
+    public void invokeGeolocationCallback(boolean value, String requestingFrame) {
+        if (isDestroyed()) return;
+        nativeInvokeGeolocationCallback(mNativeAwContents, value, requestingFrame);
     }
 
     @CalledByNative
@@ -2447,7 +2435,7 @@ private void onGeolocationPermissionsShowPrompt(String origin) {
             return;
         }
         mContentsClient.onGeolocationPermissionsShowPrompt(
-                origin, new AwGeolocationCallback());
+                origin, new AwGeolocationCallback(origin, this));
     }
 
     @CalledByNative
diff --git a/android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java b/android_webview/java/src/org/chromium/android_webview/permission/AwGeolocationCallback.java
@@ -0,0 +1,76 @@
+// Copyright 2015 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.
+
+package org.chromium.android_webview.permission;
+
+import android.webkit.GeolocationPermissions;
+
+import org.chromium.android_webview.AwContents;
+import org.chromium.base.Log;
+import org.chromium.base.ThreadUtils;
+import org.chromium.content.common.CleanupReference;
+
+import java.lang.ref.WeakReference;
+
+/**
+ * This class implements GeolocationPermissions.Callback, and will be sent to
+ * WebView applications through WebChromeClient.onGeolocationPermissionsShowPrompt().
+ */
+public class AwGeolocationCallback implements GeolocationPermissions.Callback {
+    private static final String TAG = "cr.Geolocation";
+
+    private CleanupRunable mCleanupRunable;
+    private CleanupReference mCleanupReference;
+
+    private static class CleanupRunable implements Runnable {
+        private WeakReference<AwContents> mAwContents;
+        private boolean mAllow;
+        private boolean mRetain;
+        private String mOrigin;
+
+        public CleanupRunable(AwContents awContents, String origin) {
+            mAwContents = new WeakReference<AwContents>(awContents);
+            mOrigin = origin;
+        }
+
+        @Override
+        public void run() {
+            assert ThreadUtils.runningOnUiThread();
+            AwContents awContents = mAwContents.get();
+            if (awContents == null) return;
+            if (mRetain) {
+                if (mAllow) {
+                    awContents.getGeolocationPermissions().allow(mOrigin);
+                } else {
+                    awContents.getGeolocationPermissions().deny(mOrigin);
+                }
+            }
+            awContents.invokeGeolocationCallback(mAllow, mOrigin);
+        }
+
+        public void setResponse(String origin, boolean allow, boolean retain) {
+            mOrigin = origin;
+            mAllow = allow;
+            mRetain = retain;
+        }
+    }
+
+    public AwGeolocationCallback(String origin, AwContents awContents) {
+        mCleanupRunable = new CleanupRunable(awContents, origin);
+        mCleanupReference = new CleanupReference(this, mCleanupRunable);
+    }
+
+    @Override
+    public void invoke(String origin, boolean allow, boolean retain) {
+        if (mCleanupRunable == null || mCleanupReference == null) {
+            Log.w(TAG, "Response for this geolocation request has been received."
+                    + " Ignoring subsequent responses");
+            return;
+        }
+        mCleanupRunable.setResponse(origin, allow, retain);
+        mCleanupReference.cleanupNow();
+        mCleanupReference = null;
+        mCleanupRunable = null;
+    }
+}
diff --git a/android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java b/android_webview/javatests/src/org/chromium/android_webview/test/GeolocationTest.java
@@ -6,9 +6,11 @@
 
 import android.os.Build;
 import android.test.suitebuilder.annotation.MediumTest;
+import android.test.suitebuilder.annotation.SmallTest;
 import android.webkit.GeolocationPermissions;
 
 import org.chromium.android_webview.AwContents;
+import org.chromium.base.annotations.SuppressFBWarnings;
 import org.chromium.base.test.util.Feature;
 import org.chromium.base.test.util.MinAndroidSdkLevel;
 import org.chromium.content.browser.LocationProviderFactory;
@@ -37,52 +39,65 @@ public class GeolocationTest extends AwTestBase {
             + "      function gotPos(position) {\n"
             + "        positionCount++;\n"
             + "      }\n"
+            + "      function errorCallback(error){"
+            + "        window.document.title = 'deny';"
+            + "        console.log('navigator.getCurrentPosition error: ', error);"
+            + "      }"
             + "      function initiate_getCurrentPosition() {\n"
             + "        navigator.geolocation.getCurrentPosition(\n"
-            + "            gotPos, function() { }, { });\n"
+            + "            gotPos, errorCallback, { });\n"
             + "      }\n"
             + "      function initiate_watchPosition() {\n"
             + "        navigator.geolocation.watchPosition(\n"
-            + "            gotPos, function() { }, { });\n"
+            + "            gotPos, errorCallback, { });\n"
             + "      }\n"
             + "    </script>\n"
             + "  </head>\n"
             + "  <body>\n"
             + "  </body>\n"
             + "</html>";
 
-    @Override
-    public void setUp() throws Exception {
-        super.setUp();
-        mContentsClient = new TestAwContentsClient() {
-                @Override
-                public void onGeolocationPermissionsShowPrompt(String origin,
-                        GeolocationPermissions.Callback callback) {
-                    callback.invoke(origin, true, true);
-                }
-        };
-        mAwContents = createAwTestContainerViewOnMainSync(mContentsClient).getAwContents();
-        enableJavaScriptOnUiThread(mAwContents);
-        setupGeolocation();
+    private static class GrantPermisionAwContentClient extends TestAwContentsClient {
+        @Override
+        public void onGeolocationPermissionsShowPrompt(String origin,
+                GeolocationPermissions.Callback callback) {
+            callback.invoke(origin, true, true);
+        }
     }
 
-    @Override
-    public void tearDown() throws Exception {
-        mMockLocationProvider.stopUpdates();
-        super.tearDown();
+    private static class DefaultPermisionAwContentClient extends TestAwContentsClient {
+        @Override
+        public void onGeolocationPermissionsShowPrompt(String origin,
+                GeolocationPermissions.Callback callback) {
+            // This method is empty intentionally to simulate callback is not referenced.
+        }
     }
 
-    private void setupGeolocation() {
+    private void initAwContents(TestAwContentsClient contentsClient) throws Exception {
+        mContentsClient = contentsClient;
+        mAwContents = createAwTestContainerViewOnMainSync(mContentsClient).getAwContents();
+        enableJavaScriptOnUiThread(mAwContents);
         getInstrumentation().runOnMainSync(new Runnable() {
             @Override
             public void run() {
                 mAwContents.getSettings().setGeolocationEnabled(true);
             }
         });
+    }
+
+    @Override
+    public void setUp() throws Exception {
+        super.setUp();
         mMockLocationProvider = new MockLocationProvider();
         LocationProviderFactory.setLocationProviderImpl(mMockLocationProvider);
     }
 
+    @Override
+    public void tearDown() throws Exception {
+        mMockLocationProvider.stopUpdates();
+        super.tearDown();
+    }
+
     private int getPositionCountFromJS() {
         int result = -1;
         try {
@@ -110,6 +125,7 @@ public Boolean call() throws Exception {
     @MediumTest
     @Feature({"AndroidWebView"})
     public void testGetPosition() throws Throwable {
+        initAwContents(new GrantPermisionAwContentClient());
         loadDataSync(mAwContents, mContentsClient.getOnPageFinishedHelper(),
                 RAW_HTML, "text/html", false);
 
@@ -137,6 +153,7 @@ public Boolean call() throws Exception {
     @MediumTest
     @Feature({"AndroidWebView"})
     public void testWatchPosition() throws Throwable {
+        initAwContents(new GrantPermisionAwContentClient());
         loadDataSync(mAwContents, mContentsClient.getOnPageFinishedHelper(),
                 RAW_HTML, "text/html", false);
 
@@ -153,6 +170,7 @@ public Boolean call() throws Exception {
     @MediumTest
     @Feature({"AndroidWebView"})
     public void testPauseGeolocationOnPause() throws Throwable {
+        initAwContents(new GrantPermisionAwContentClient());
         // Start a watch going.
         loadDataSync(mAwContents, mContentsClient.getOnPageFinishedHelper(),
                 RAW_HTML, "text/html", false);
@@ -204,6 +222,7 @@ public Boolean call() throws Exception {
     @MediumTest
     @Feature({"AndroidWebView"})
     public void testPauseAwContentsBeforeNavigating() throws Throwable {
+        initAwContents(new GrantPermisionAwContentClient());
         getInstrumentation().runOnMainSync(new Runnable() {
             @Override
             public void run() {
@@ -241,6 +260,7 @@ public Boolean call() throws Exception {
     @MediumTest
     @Feature({"AndroidWebView"})
     public void testResumeWhenNotStarted() throws Throwable {
+        initAwContents(new GrantPermisionAwContentClient());
         getInstrumentation().runOnMainSync(new Runnable() {
             @Override
             public void run() {
@@ -261,4 +281,23 @@ public void run() {
         ensureGeolocationRunning(false);
     }
 
+    @Feature({"AndroidWebView"})
+    @SmallTest
+    public void testDenyAccessByDefault() throws Throwable {
+        initAwContents(new DefaultPermisionAwContentClient());
+        loadDataSync(mAwContents, mContentsClient.getOnPageFinishedHelper(),
+                RAW_HTML, "text/html", false);
+
+        mAwContents.evaluateJavaScriptForTests("initiate_getCurrentPosition();", null);
+
+        poll(new Callable<Boolean>() {
+            @SuppressFBWarnings("DM_GC")
+            @Override
+            public Boolean call() throws Exception {
+                Runtime.getRuntime().gc();
+                return "deny".equals(getTitleOnUiThread(mAwContents));
+            }
+        });
+    }
+
 }
diff --git a/android_webview/native/aw_contents.cc b/android_webview/native/aw_contents.cc
@@ -491,11 +491,8 @@ void AwContents::InvokeGeolocationCallback(JNIEnv* env,
                                            jboolean value,
                                            jstring origin) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (pending_geolocation_prompts_.empty()) {
-    LOG(WARNING) << "Response for this geolocation request has been received."
-                 << " Ignoring subsequent responses";
+  if (pending_geolocation_prompts_.empty())
     return;
-  }
 
   GURL callback_origin(base::android::ConvertJavaStringToUTF16(env, origin));
   if (callback_origin.GetOrigin() ==
@@ -506,9 +503,6 @@ void AwContents::InvokeGeolocationCallback(JNIEnv* env,
       ShowGeolocationPromptHelper(java_ref_,
                                   pending_geolocation_prompts_.front().first);
     }
-  } else {
-    LOG(WARNING) << "Response for this geolocation request has been received."
-                 << " Ignoring subsequent responses";
   }
 }
 
