From 6966bd5329817655db19c2c8b754abcfe17091df Mon Sep 17 00:00:00 2001 From: Clark DuVall Date: Mon, 23 Mar 2020 17:50:07 +0000 Subject: [PATCH] [WebLayer] Switch geolocation tests to use permission prompt These tests were previously using the fake permission manager which always grants permissions. Now we can use the real permission manager, and the fake permission manager can be removed. Fixes a strict mode violation in permission dialogs (similar to fixes from https://crrev.com/c/2108603). Bug: 1025625, 1025609 Change-Id: I8b756c61ee213151e53d68a375ffa4d3f8fd7643 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2111061 Commit-Queue: Clark DuVall Reviewed-by: John Abd-El-Malek Cr-Commit-Position: refs/heads/master@{#752494} --- .../permissions/PermissionDialogModel.java | 12 ++- weblayer/BUILD.gn | 2 - .../weblayer/test/GeolocationTest.java | 23 +++++- weblayer/browser/browser_context_impl.cc | 9 --- weblayer/browser/browser_context_impl.h | 2 - .../browser/content_browser_client_impl.cc | 9 --- .../browser/content_browser_client_impl.h | 2 - .../fake_permission_controller_delegate.cc | 77 ------------------- .../fake_permission_controller_delegate.h | 63 --------------- weblayer/browser/java/BUILD.gn | 3 + .../test/TestWebLayerImpl.java | 27 ++++++- .../test_interfaces/ITestWebLayer.aidl | 6 ++ weblayer/public/common/switches.cc | 4 - weblayer/public/common/switches.h | 1 - .../org/chromium/weblayer/TestWebLayer.java | 8 ++ 15 files changed, 74 insertions(+), 174 deletions(-) delete mode 100644 weblayer/browser/fake_permission_controller_delegate.cc delete mode 100644 weblayer/browser/fake_permission_controller_delegate.h diff --git a/components/permissions/android/java/src/org/chromium/components/permissions/PermissionDialogModel.java b/components/permissions/android/java/src/org/chromium/components/permissions/PermissionDialogModel.java index 0b6c34276f9dd5..d9bc61cbb047d9 100644 --- a/components/permissions/android/java/src/org/chromium/components/permissions/PermissionDialogModel.java +++ b/components/permissions/android/java/src/org/chromium/components/permissions/PermissionDialogModel.java @@ -12,6 +12,7 @@ import androidx.core.widget.TextViewCompat; +import org.chromium.base.StrictModeContext; import org.chromium.components.browser_ui.modaldialog.R; import org.chromium.ui.modaldialog.ModalDialogProperties; import org.chromium.ui.modelutil.PropertyModel; @@ -24,8 +25,7 @@ public static PropertyModel getModel( ModalDialogProperties.Controller controller, PermissionDialogDelegate delegate) { Context context = delegate.getWindow().getContext().get(); assert context != null; - LayoutInflater inflater = LayoutInflater.from(context); - View customView = inflater.inflate(R.layout.permission_dialog, null); + View customView = loadDialogView(context); String messageText = delegate.getMessageText(); assert !TextUtils.isEmpty(messageText); @@ -44,4 +44,12 @@ public static PropertyModel getModel( .with(ModalDialogProperties.FILTER_TOUCH_FOR_SECURITY, true) .build(); } + + private static View loadDialogView(Context context) { + // LayoutInflater may access the disk. + try (StrictModeContext ignored = StrictModeContext.allowDiskReads()) { + LayoutInflater inflater = LayoutInflater.from(context); + return inflater.inflate(R.layout.permission_dialog, null); + } + } } diff --git a/weblayer/BUILD.gn b/weblayer/BUILD.gn index ad1e1a342babf7..2152e8215b1380 100644 --- a/weblayer/BUILD.gn +++ b/weblayer/BUILD.gn @@ -108,8 +108,6 @@ source_set("weblayer_lib_base") { "browser/download_impl.h", "browser/download_manager_delegate_impl.cc", "browser/download_manager_delegate_impl.h", - "browser/fake_permission_controller_delegate.cc", - "browser/fake_permission_controller_delegate.h", "browser/feature_list_creator.cc", "browser/feature_list_creator.h", "browser/file_select_helper.cc", diff --git a/weblayer/browser/android/javatests/src/org/chromium/weblayer/test/GeolocationTest.java b/weblayer/browser/android/javatests/src/org/chromium/weblayer/test/GeolocationTest.java index 8be2e7d56f7b26..2bcf0a32d81690 100644 --- a/weblayer/browser/android/javatests/src/org/chromium/weblayer/test/GeolocationTest.java +++ b/weblayer/browser/android/javatests/src/org/chromium/weblayer/test/GeolocationTest.java @@ -14,7 +14,6 @@ import org.junit.Test; import org.junit.runner.RunWith; -import org.chromium.base.test.util.CommandLineFlags; import org.chromium.content_public.browser.test.util.CriteriaHelper; import org.chromium.content_public.browser.test.util.TestThreadUtils; import org.chromium.net.test.util.TestWebServer; @@ -27,7 +26,6 @@ * Tests that Geolocation Web API works as expected. */ @RunWith(WebLayerJUnit4ClassRunner.class) -@CommandLineFlags.Add("weblayer-fake-permissions") public final class GeolocationTest { @Rule public InstrumentationActivityTestRule mActivityTestRule = @@ -94,6 +92,8 @@ public void tearDown() throws Throwable { @MediumTest public void testGeolocation_getPosition() throws Throwable { mActivityTestRule.executeScriptSync("initiate_getCurrentPosition();", false); + waitForDialog(); + mTestWebLayer.clickPermissionDialogButton(true); waitForCountEqual("positionCount", 1); mActivityTestRule.executeScriptSync("initiate_getCurrentPosition();", false); waitForCountEqual("positionCount", 2); @@ -107,6 +107,8 @@ public void testGeolocation_getPosition() throws Throwable { @MediumTest public void testGeolocation_watchPosition() throws Throwable { mActivityTestRule.executeScriptSync("initiate_watchPosition();", false); + waitForDialog(); + mTestWebLayer.clickPermissionDialogButton(true); waitForCountGreaterThan("positionCount", 1); ensureGeolocationIsRunning(true); Assert.assertEquals(0, getCountFromJS("errorCount")); @@ -119,6 +121,8 @@ public void testGeolocation_watchPosition() throws Throwable { @MediumTest public void testGeolocation_destroyTabStopsGeolocation() throws Throwable { mActivityTestRule.executeScriptSync("initiate_watchPosition();", false); + waitForDialog(); + mTestWebLayer.clickPermissionDialogButton(true); ensureGeolocationIsRunning(true); TestThreadUtils.runOnUiThreadBlocking(() -> { Browser browser = mActivity.getBrowser(); @@ -143,6 +147,16 @@ public void testGeolocation_denyOnInsecureOrigins() throws Throwable { Assert.assertEquals(0, getCountFromJS("positionCount")); } + @Test + @MediumTest + public void testGeolocation_denyFromPrompt() throws Throwable { + mActivityTestRule.executeScriptSync("initiate_watchPosition();", false); + waitForDialog(); + mTestWebLayer.clickPermissionDialogButton(false); + waitForCountEqual("errorCount", 1); + Assert.assertEquals(0, getCountFromJS("positionCount")); + } + // helper methods private void waitForCountEqual(String variableName, int count) { @@ -150,6 +164,11 @@ private void waitForCountEqual(String variableName, int count) { () -> { return getCountFromJS(variableName) == count; }); } + private void waitForDialog() { + CriteriaHelper.pollInstrumentationThread( + () -> { return mTestWebLayer.isPermissionDialogShown(); }); + } + private void waitForCountGreaterThan(String variableName, int count) { CriteriaHelper.pollInstrumentationThread( () -> { return getCountFromJS(variableName) > count; }); diff --git a/weblayer/browser/browser_context_impl.cc b/weblayer/browser/browser_context_impl.cc index f53516fc0ba372..07f58b384f483f 100644 --- a/weblayer/browser/browser_context_impl.cc +++ b/weblayer/browser/browser_context_impl.cc @@ -23,7 +23,6 @@ #include "content/public/browser/device_service.h" #include "content/public/browser/download_request_utils.h" #include "content/public/browser/resource_context.h" -#include "weblayer/browser/fake_permission_controller_delegate.h" #include "weblayer/browser/permissions/permission_manager_factory.h" #include "weblayer/browser/stateful_ssl_host_state_delegate_factory.h" #include "weblayer/public/common/switches.h" @@ -153,14 +152,6 @@ content::SSLHostStateDelegate* BrowserContextImpl::GetSSLHostStateDelegate() { content::PermissionControllerDelegate* BrowserContextImpl::GetPermissionControllerDelegate() { - if (base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kWebLayerFakePermissions)) { - if (!permission_controller_delegate_) { - permission_controller_delegate_ = - std::make_unique(); - } - return permission_controller_delegate_.get(); - } return PermissionManagerFactory::GetForBrowserContext(this); } diff --git a/weblayer/browser/browser_context_impl.h b/weblayer/browser/browser_context_impl.h index c416405f02db39..90d65f562904e4 100644 --- a/weblayer/browser/browser_context_impl.h +++ b/weblayer/browser/browser_context_impl.h @@ -86,8 +86,6 @@ class BrowserContextImpl : public content::BrowserContext { resource_context_; DownloadManagerDelegateImpl download_delegate_; std::unique_ptr user_pref_service_; - std::unique_ptr - permission_controller_delegate_; std::unique_ptr weblayer_variations_client_; }; } // namespace weblayer diff --git a/weblayer/browser/content_browser_client_impl.cc b/weblayer/browser/content_browser_client_impl.cc index d223e8e832e903..f6b8011c96d0dc 100644 --- a/weblayer/browser/content_browser_client_impl.cc +++ b/weblayer/browser/content_browser_client_impl.cc @@ -176,15 +176,6 @@ ContentBrowserClientImpl::CreateBrowserMainParts( return browser_main_parts; } -void ContentBrowserClientImpl::AppendExtraCommandLineSwitches( - base::CommandLine* command_line, - int child_process_id) { - if (base::CommandLine::ForCurrentProcess()->HasSwitch( - switches::kWebLayerFakePermissions)) { - command_line->AppendSwitch(switches::kWebLayerFakePermissions); - } -} - std::string ContentBrowserClientImpl::GetApplicationLocale() { return i18n::GetApplicationLocale(); } diff --git a/weblayer/browser/content_browser_client_impl.h b/weblayer/browser/content_browser_client_impl.h index 254e2721224850..f3641d797503f5 100644 --- a/weblayer/browser/content_browser_client_impl.h +++ b/weblayer/browser/content_browser_client_impl.h @@ -29,8 +29,6 @@ class ContentBrowserClientImpl : public content::ContentBrowserClient { // ContentBrowserClient overrides. std::unique_ptr CreateBrowserMainParts( const content::MainFunctionParams& parameters) override; - void AppendExtraCommandLineSwitches(base::CommandLine* command_line, - int child_process_id) override; std::string GetApplicationLocale() override; std::string GetAcceptLangs(content::BrowserContext* context) override; content::WebContentsViewDelegate* GetWebContentsViewDelegate( diff --git a/weblayer/browser/fake_permission_controller_delegate.cc b/weblayer/browser/fake_permission_controller_delegate.cc deleted file mode 100644 index bc9564d16e2e0f..00000000000000 --- a/weblayer/browser/fake_permission_controller_delegate.cc +++ /dev/null @@ -1,77 +0,0 @@ -// Copyright 2020 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. - -#include "weblayer/browser/fake_permission_controller_delegate.h" - -#include "base/callback.h" -#include "content/public/browser/permission_controller.h" -#include "content/public/browser/permission_type.h" -#include "content/public/browser/web_contents.h" - -namespace weblayer { - -FakePermissionControllerDelegate::FakePermissionControllerDelegate() = default; - -FakePermissionControllerDelegate::~FakePermissionControllerDelegate() = default; - -int FakePermissionControllerDelegate::RequestPermission( - content::PermissionType permission, - content::RenderFrameHost* render_frame_host, - const GURL& requesting_origin, - bool user_gesture, - base::OnceCallback callback) { - std::move(callback).Run(blink::mojom::PermissionStatus::GRANTED); - return content::PermissionController::kNoPendingOperation; -} - -int FakePermissionControllerDelegate::RequestPermissions( - const std::vector& permissions, - content::RenderFrameHost* render_frame_host, - const GURL& requesting_origin, - bool user_gesture, - base::OnceCallback&)> - callback) { - std::vector result( - permissions.size(), blink::mojom::PermissionStatus::GRANTED); - std::move(callback).Run(result); - return content::PermissionController::kNoPendingOperation; -} - -void FakePermissionControllerDelegate::ResetPermission( - content::PermissionType permission, - const GURL& requesting_origin, - const GURL& embedding_origin) {} - -blink::mojom::PermissionStatus -FakePermissionControllerDelegate::GetPermissionStatus( - content::PermissionType permission, - const GURL& requesting_origin, - const GURL& embedding_origin) { - return blink::mojom::PermissionStatus::GRANTED; -} - -blink::mojom::PermissionStatus -FakePermissionControllerDelegate::GetPermissionStatusForFrame( - content::PermissionType permission, - content::RenderFrameHost* render_frame_host, - const GURL& requesting_origin) { - return GetPermissionStatus( - permission, requesting_origin, - content::WebContents::FromRenderFrameHost(render_frame_host) - ->GetLastCommittedURL() - .GetOrigin()); -} - -int FakePermissionControllerDelegate::SubscribePermissionStatusChange( - content::PermissionType permission, - content::RenderFrameHost* render_frame_host, - const GURL& requesting_origin, - base::RepeatingCallback callback) { - return content::PermissionController::kNoPendingOperation; -} - -void FakePermissionControllerDelegate::UnsubscribePermissionStatusChange( - int subscription_id) {} - -} // namespace weblayer diff --git a/weblayer/browser/fake_permission_controller_delegate.h b/weblayer/browser/fake_permission_controller_delegate.h deleted file mode 100644 index 018ffd99d6b42f..00000000000000 --- a/weblayer/browser/fake_permission_controller_delegate.h +++ /dev/null @@ -1,63 +0,0 @@ -// Copyright 2020 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. - -#ifndef WEBLAYER_BROWSER_FAKE_PERMISSION_CONTROLLER_DELEGATE_H_ -#define WEBLAYER_BROWSER_FAKE_PERMISSION_CONTROLLER_DELEGATE_H_ - -#include "base/callback_forward.h" -#include "base/macros.h" -#include "content/public/browser/permission_controller_delegate.h" - -namespace weblayer { - -// Temporary permission controller delegate which grants all permissions. Once -// permissions have been implemented, this will be removed. This is only used if -// the --weblayer-fake-permissions switch is passed on the command line. -class FakePermissionControllerDelegate - : public content::PermissionControllerDelegate { - public: - FakePermissionControllerDelegate(); - ~FakePermissionControllerDelegate() override; - - // PermissionControllerDelegate implementation. - int RequestPermission(content::PermissionType permission, - content::RenderFrameHost* render_frame_host, - const GURL& requesting_origin, - bool user_gesture, - base::OnceCallback - callback) override; - int RequestPermissions( - const std::vector& permission, - content::RenderFrameHost* render_frame_host, - const GURL& requesting_origin, - bool user_gesture, - base::OnceCallback< - void(const std::vector&)> callback) - override; - void ResetPermission(content::PermissionType permission, - const GURL& requesting_origin, - const GURL& embedding_origin) override; - blink::mojom::PermissionStatus GetPermissionStatus( - content::PermissionType permission, - const GURL& requesting_origin, - const GURL& embedding_origin) override; - blink::mojom::PermissionStatus GetPermissionStatusForFrame( - content::PermissionType permission, - content::RenderFrameHost* render_frame_host, - const GURL& requesting_origin) override; - int SubscribePermissionStatusChange( - content::PermissionType permission, - content::RenderFrameHost* render_frame_host, - const GURL& requesting_origin, - base::RepeatingCallback callback) - override; - void UnsubscribePermissionStatusChange(int subscription_id) override; - - private: - DISALLOW_COPY_AND_ASSIGN(FakePermissionControllerDelegate); -}; - -} // namespace weblayer - -#endif // WEBLAYER_BROWSER_FAKE_PERMISSION_CONTROLLER_DELEGATE_H_ diff --git a/weblayer/browser/java/BUILD.gn b/weblayer/browser/java/BUILD.gn index e67d4cbad69bbb..aef77fc7b7c52e 100644 --- a/weblayer/browser/java/BUILD.gn +++ b/weblayer/browser/java/BUILD.gn @@ -143,9 +143,12 @@ android_library("test_java") { sources = [ "org/chromium/weblayer_private/test/TestWebLayerImpl.java" ] deps = [ ":weblayer_test_resources", + "//components/permissions/android:java", + "//content/public/test/android:content_java_test_support", "//net/android:net_java", "//services/device/public/java:geolocation_java", "//services/device/public/java:geolocation_java_test_support", + "//ui/android:ui_full_java", ] srcjar_deps = [ ":test_aidl" ] } diff --git a/weblayer/browser/java/org/chromium/weblayer_private/test/TestWebLayerImpl.java b/weblayer/browser/java/org/chromium/weblayer_private/test/TestWebLayerImpl.java index 1fc308463dfbe7..9b6c3b0aa888a3 100644 --- a/weblayer/browser/java/org/chromium/weblayer_private/test/TestWebLayerImpl.java +++ b/weblayer/browser/java/org/chromium/weblayer_private/test/TestWebLayerImpl.java @@ -7,11 +7,16 @@ import android.os.IBinder; import org.chromium.base.annotations.UsedByReflection; +import org.chromium.components.permissions.PermissionDialogController; +import org.chromium.content_public.browser.test.util.TestThreadUtils; import org.chromium.device.geolocation.LocationProviderOverrider; import org.chromium.device.geolocation.MockLocationProvider; import org.chromium.net.NetworkChangeNotifier; +import org.chromium.ui.modaldialog.ModalDialogProperties; import org.chromium.weblayer_private.test_interfaces.ITestWebLayer; +import java.util.concurrent.ExecutionException; + /** * Root implementation class for TestWebLayer. */ @@ -46,4 +51,24 @@ public void setMockLocationProvider(boolean enable) { public boolean isMockLocationProviderRunning() { return mMockLocationProvider.isRunning(); } -} \ No newline at end of file + + @Override + public boolean isPermissionDialogShown() { + try { + return TestThreadUtils.runOnUiThreadBlocking(() -> { + return PermissionDialogController.getInstance().isDialogShownForTest(); + }); + } catch (ExecutionException e) { + return false; + } + } + + @Override + public void clickPermissionDialogButton(boolean allow) { + TestThreadUtils.runOnUiThreadBlocking(() -> { + PermissionDialogController.getInstance().clickButtonForTest(allow + ? ModalDialogProperties.ButtonType.POSITIVE + : ModalDialogProperties.ButtonType.NEGATIVE); + }); + } +} diff --git a/weblayer/browser/java/org/chromium/weblayer_private/test_interfaces/ITestWebLayer.aidl b/weblayer/browser/java/org/chromium/weblayer_private/test_interfaces/ITestWebLayer.aidl index 6a2a36bd26d8d2..7000fdcba929e8 100644 --- a/weblayer/browser/java/org/chromium/weblayer_private/test_interfaces/ITestWebLayer.aidl +++ b/weblayer/browser/java/org/chromium/weblayer_private/test_interfaces/ITestWebLayer.aidl @@ -10,4 +10,10 @@ interface ITestWebLayer { // set mock location provider void setMockLocationProvider(in boolean enable) = 2; boolean isMockLocationProviderRunning() = 3; + + // Whether or not a permission dialog is currently showing. + boolean isPermissionDialogShown() = 4; + + // Clicks a button on the permission dialog. + void clickPermissionDialogButton(in boolean allow) = 5; } diff --git a/weblayer/public/common/switches.cc b/weblayer/public/common/switches.cc index 3cf247589cc362..21fdccb938d211 100644 --- a/weblayer/public/common/switches.cc +++ b/weblayer/public/common/switches.cc @@ -9,8 +9,4 @@ namespace switches { // Makes WebLayer Shell use the given path for its data directory. const char kWebLayerUserDataDir[] = "weblayer-user-data-dir"; -// Makes WebLayer use a fake permission controller delegate which grants all -// permissions. -const char kWebLayerFakePermissions[] = "weblayer-fake-permissions"; - } // namespace switches diff --git a/weblayer/public/common/switches.h b/weblayer/public/common/switches.h index 05f3c69639dc24..208d09f0ec1391 100644 --- a/weblayer/public/common/switches.h +++ b/weblayer/public/common/switches.h @@ -8,7 +8,6 @@ namespace switches { extern const char kWebLayerUserDataDir[]; -extern const char kWebLayerFakePermissions[]; } // namespace switches diff --git a/weblayer/public/javatestutil/org/chromium/weblayer/TestWebLayer.java b/weblayer/public/javatestutil/org/chromium/weblayer/TestWebLayer.java index 7e6c44a620101e..4e0126e950434e 100644 --- a/weblayer/public/javatestutil/org/chromium/weblayer/TestWebLayer.java +++ b/weblayer/public/javatestutil/org/chromium/weblayer/TestWebLayer.java @@ -63,4 +63,12 @@ public void setMockLocationProvider(boolean enabled) throws RemoteException { public boolean isMockLocationProviderRunning() throws RemoteException { return mITestWebLayer.isMockLocationProviderRunning(); } + + public boolean isPermissionDialogShown() throws RemoteException { + return mITestWebLayer.isPermissionDialogShown(); + } + + public void clickPermissionDialogButton(boolean allow) throws RemoteException { + mITestWebLayer.clickPermissionDialogButton(allow); + } }