Remove dependency on net::URLRequest in InterceptNavigationDelegate

This moves the UpdateUserGestureCarryoverInfo calls from ResourceDispatcherHostDelegate (which is not used with network service enabled) to ResourceDispatcher. ResourceDispatcher will then pass the info back to the browser through a mojo call. We should not depend on net::URLRequest anywhere outside of the network service. This fixes InterceptNavigationDelegateTests when run with network service enabled. Bug: 882060 Cq-Include-Trybots: luci.chromium.try:linux_mojo Change-Id: Ib737185d5e1f09063bdad0be20a68171a920c975 Reviewed-on: https://chromium-review.googlesource.com/1217347 Reviewed-by: Jochen Eisinger <jochen@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Clark DuVall <cduvall@chromium.org> Cr-Commit-Position: refs/heads/master@{#591711}
Denger-Network · Sep 17, 2018 · a0e4299 · a0e4299
1 parent 15b59a4
commit a0e4299
Show file tree

Hide file tree

Showing 19 changed files with 96 additions and 67 deletions.
diff --git a/android_webview/browser/aw_web_contents_delegate.cc b/android_webview/browser/aw_web_contents_delegate.cc
@@ -20,6 +20,7 @@
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "components/navigation_interception/intercept_navigation_delegate.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
@@ -266,6 +267,14 @@ bool AwWebContentsDelegate::IsFullscreenForTabOrPending(
   return is_fullscreen_;
 }
 
+void AwWebContentsDelegate::UpdateUserGestureCarryoverInfo(
+    content::WebContents* web_contents) {
+  auto* intercept_navigation_delegate =
+      navigation_interception::InterceptNavigationDelegate::Get(web_contents);
+  if (intercept_navigation_delegate)
+    intercept_navigation_delegate->UpdateLastUserGestureCarryoverTimestamp();
+}
+
 static void JNI_AwWebContentsDelegate_FilesSelectedInChooser(
     JNIEnv* env,
     const JavaParamRef<jclass>& clazz,

diff --git a/android_webview/browser/aw_web_contents_delegate.h b/android_webview/browser/aw_web_contents_delegate.h
@@ -62,6 +62,8 @@ class AwWebContentsDelegate
   void ExitFullscreenModeForTab(content::WebContents* web_contents) override;
   bool IsFullscreenForTabOrPending(
       const content::WebContents* web_contents) const override;
+  void UpdateUserGestureCarryoverInfo(
+      content::WebContents* web_contents) override;
 
  private:
   bool is_fullscreen_;

diff --git a/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc b/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
@@ -16,7 +16,6 @@
 #include "android_webview/browser/net/aw_web_resource_request.h"
 #include "android_webview/browser/renderer_host/auto_login_parser.h"
 #include "android_webview/common/url_constants.h"
-#include "components/navigation_interception/intercept_navigation_delegate.h"
 #include "components/safe_browsing/android/safe_browsing_api_handler.h"
 #include "components/safe_browsing/features.h"
 #include "components/web_restrictions/browser/web_restrictions_resource_throttle.h"
@@ -38,7 +37,6 @@ using android_webview::AwWebResourceRequest;
 using content::BrowserThread;
 using content::ResourceType;
 using content::WebContents;
-using navigation_interception::InterceptNavigationDelegate;
 
 namespace {
 
@@ -324,8 +322,6 @@ void AwResourceDispatcherHostDelegate::RequestBeginning(
   throttles->push_back(std::move(ioThreadThrottle));
 
   bool is_main_frame = resource_type == content::RESOURCE_TYPE_MAIN_FRAME;
-  if (!is_main_frame)
-    InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request);
   throttles->push_back(
       std::make_unique<web_restrictions::WebRestrictionsResourceThrottle>(
           AwBrowserContext::GetDefault()->GetWebRestrictionProvider(),

diff --git a/chrome/browser/android/tab_web_contents_delegate_android.cc b/chrome/browser/android/tab_web_contents_delegate_android.cc
@@ -41,6 +41,7 @@
 #include "chrome/common/url_constants.h"
 #include "components/app_modal/javascript_dialog_manager.h"
 #include "components/infobars/core/infobar.h"
+#include "components/navigation_interception/intercept_navigation_delegate.h"
 #include "components/security_state/content/content_utils.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_details.h"
@@ -455,6 +456,14 @@ void TabWebContentsDelegateAndroid::OnDidBlockFramebust(
   ShowFramebustBlockInfobarInternal(web_contents, url);
 }
 
+void TabWebContentsDelegateAndroid::UpdateUserGestureCarryoverInfo(
+    content::WebContents* web_contents) {
+  auto* intercept_navigation_delegate =
+      navigation_interception::InterceptNavigationDelegate::Get(web_contents);
+  if (intercept_navigation_delegate)
+    intercept_navigation_delegate->UpdateLastUserGestureCarryoverTimestamp();
+}
+
 }  // namespace android
 
 void JNI_TabWebContentsDelegateAndroid_OnRendererUnresponsive(

diff --git a/chrome/browser/android/tab_web_contents_delegate_android.h b/chrome/browser/android/tab_web_contents_delegate_android.h
@@ -89,6 +89,8 @@ class TabWebContentsDelegateAndroid
       content::WebContents* web_contents) override;
   void OnDidBlockFramebust(content::WebContents* web_contents,
                            const GURL& url) override;
+  void UpdateUserGestureCarryoverInfo(
+      content::WebContents* web_contents) override;
 
  private:
   // NotificationObserver implementation.

diff --git a/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc b/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
@@ -111,7 +111,6 @@
 #if defined(OS_ANDROID)
 #include "chrome/browser/android/download/intercept_download_resource_throttle.h"
 #include "chrome/browser/loader/data_reduction_proxy_resource_throttle_android.h"
-#include "components/navigation_interception/intercept_navigation_delegate.h"
 #endif
 
 #if defined(OS_CHROMEOS)
@@ -130,10 +129,6 @@ using extensions::Extension;
 using extensions::StreamsPrivateAPI;
 #endif
 
-#if defined(OS_ANDROID)
-using navigation_interception::InterceptNavigationDelegate;
-#endif
-
 namespace {
 
 prerender::PrerenderManager* GetPrerenderManager(
@@ -370,11 +365,6 @@ void ChromeResourceDispatcherHostDelegate::RequestBeginning(
                                          info->GetResourceType()));
 #endif  // BUILDFLAG(ENABLE_OFFLINE_PAGES)
 
-#if defined(OS_ANDROID)
-  if (resource_type != content::RESOURCE_TYPE_MAIN_FRAME)
-    InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(request);
-#endif
-
 #if defined(OS_CHROMEOS)
   // Check if we need to add merge session throttle. This throttle will postpone
   // loading of XHR requests.

diff --git a/components/navigation_interception/DEPS b/components/navigation_interception/DEPS
@@ -1,6 +1,5 @@
 include_rules = [
   "+jni",
-  "+net",
   "+ui/base",
 
   "+content/public/browser",

diff --git a/components/navigation_interception/intercept_navigation_delegate.cc b/components/navigation_interception/intercept_navigation_delegate.cc
@@ -18,7 +18,6 @@
 #include "content/public/browser/resource_request_info.h"
 #include "content/public/browser/web_contents.h"
 #include "jni/InterceptNavigationDelegate_jni.h"
-#include "net/url_request/url_request.h"
 #include "url/gurl.h"
 
 using base::android::ConvertUTF8ToJavaString;
@@ -50,24 +49,6 @@ bool CheckIfShouldIgnoreNavigationOnUIThread(WebContents* source,
   return intercept_navigation_delegate->ShouldIgnoreNavigation(params);
 }
 
-void UpdateUserGestureCarryoverInfoOnUIThread(int render_process_id,
-                                              int render_frame_id) {
-  content::RenderFrameHost* render_frame_host =
-      content::RenderFrameHost::FromID(render_process_id, render_frame_id);
-  if (!render_frame_host)
-    return;
-
-  content::WebContents* web_contents =
-      content::WebContents::FromRenderFrameHost(render_frame_host);
-
-  InterceptNavigationDelegate* intercept_navigation_delegate =
-      InterceptNavigationDelegate::Get(web_contents);
-
-  if (intercept_navigation_delegate) {
-    intercept_navigation_delegate->UpdateLastUserGestureCarryoverTimestamp();
-  }
-}
-
 }  // namespace
 
 // static
@@ -93,23 +74,6 @@ InterceptNavigationDelegate::CreateThrottleFor(
       handle, base::Bind(&CheckIfShouldIgnoreNavigationOnUIThread));
 }
 
-// static
-void InterceptNavigationDelegate::UpdateUserGestureCarryoverInfo(
-    net::URLRequest* request) {
-  const content::ResourceRequestInfo* info =
-      content::ResourceRequestInfo::ForRequest(request);
-  if (!info || !info->HasUserGesture())
-    return;
-
-  int render_process_id, render_frame_id;
-  if (!info->GetAssociatedRenderFrame(&render_process_id, &render_frame_id))
-    return;
-
-  BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
-                          base::Bind(&UpdateUserGestureCarryoverInfoOnUIThread,
-                                     render_process_id, render_frame_id));
-}
-
 InterceptNavigationDelegate::InterceptNavigationDelegate(
     JNIEnv* env, jobject jdelegate)
     : weak_jdelegate_(env, jdelegate) {

diff --git a/components/navigation_interception/intercept_navigation_delegate.h b/components/navigation_interception/intercept_navigation_delegate.h
@@ -17,10 +17,6 @@ class NavigationThrottle;
 class WebContents;
 }
 
-namespace net {
-class URLRequest;
-}
-
 namespace navigation_interception {
 
 class NavigationParams;
@@ -55,10 +51,6 @@ class InterceptNavigationDelegate : public base::SupportsUserData::Data {
   static std::unique_ptr<content::NavigationThrottle> CreateThrottleFor(
       content::NavigationHandle* handle);
 
-  // Updates information to determine whether to have user gesture carryover or
-  // not.
-  static void UpdateUserGestureCarryoverInfo(net::URLRequest* request);
-
   virtual bool ShouldIgnoreNavigation(
       const NavigationParams& navigation_params);
 

diff --git a/content/browser/frame_host/render_frame_host_delegate.h b/content/browser/frame_host/render_frame_host_delegate.h
@@ -240,6 +240,14 @@ class CONTENT_EXPORT RenderFrameHostDelegate {
   virtual void FullscreenStateChanged(RenderFrameHost* rfh,
                                       bool is_fullscreen) {}
 
+#if defined(OS_ANDROID)
+  // Updates information to determine whether a user gesture should carryover to
+  // future navigations. This is needed so navigations within a certain
+  // timeframe of a request initiated by a gesture will be treated as if they
+  // were initiated by a gesture too, otherwise the navigation may be blocked.
+  virtual void UpdateUserGestureCarryoverInfo() {}
+#endif
+
   // Let the delegate decide whether postMessage should be delivered to
   // |target_rfh| from a source frame in the given SiteInstance.  This defaults
   // to false and overrides the RenderFrameHost's decision if true.

diff --git a/content/browser/frame_host/render_frame_host_impl.cc b/content/browser/frame_host/render_frame_host_impl.cc
@@ -2614,6 +2614,12 @@ void RenderFrameHostImpl::FullscreenStateChanged(bool is_fullscreen) {
   delegate_->FullscreenStateChanged(this, is_fullscreen);
 }
 
+#if defined(OS_ANDROID)
+void RenderFrameHostImpl::UpdateUserGestureCarryoverInfo() {
+  delegate_->UpdateUserGestureCarryoverInfo();
+}
+#endif
+
 void RenderFrameHostImpl::OnDidBlockFramebust(const GURL& url) {
   delegate_->OnDidBlockFramebust(url);
 }

diff --git a/content/browser/frame_host/render_frame_host_impl.h b/content/browser/frame_host/render_frame_host_impl.h
@@ -1011,6 +1011,9 @@ class CONTENT_EXPORT RenderFrameHostImpl
   void UpdateEncoding(const std::string& encoding) override;
   void FrameSizeChanged(const gfx::Size& frame_size) override;
   void FullscreenStateChanged(bool is_fullscreen) override;
+#if defined(OS_ANDROID)
+  void UpdateUserGestureCarryoverInfo() override;
+#endif
 
   // Registers Mojo interfaces that this frame host makes available.
   void RegisterMojoInterfaces();

diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
@@ -2416,6 +2416,13 @@ void WebContentsImpl::FullscreenFrameSetUpdated() {
     display_cutout_host_impl_->DidAcquireFullscreen(new_fullscreen_frame);
 }
 
+#if defined(OS_ANDROID)
+void WebContentsImpl::UpdateUserGestureCarryoverInfo() {
+  if (delegate_)
+    delegate_->UpdateUserGestureCarryoverInfo(this);
+}
+#endif
+
 bool WebContentsImpl::IsFullscreenForCurrentTab() const {
   return delegate_ ? delegate_->IsFullscreenForTabOrPending(this) : false;
 }

diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
@@ -538,6 +538,9 @@ class CONTENT_EXPORT WebContentsImpl : public WebContents,
   void ExitFullscreenMode(bool will_cause_resize) override;
   void FullscreenStateChanged(RenderFrameHost* rfh,
                               bool is_fullscreen) override;
+#if defined(OS_ANDROID)
+  void UpdateUserGestureCarryoverInfo() override;
+#endif
   bool ShouldRouteMessageEvent(
       RenderFrameHost* target_rfh,
       SiteInstance* source_site_instance) const override;

diff --git a/content/common/frame.mojom b/content/common/frame.mojom
@@ -358,4 +358,11 @@ interface FrameHost {
   // Notifies the browser that the current frame has either become or is no
   // longer fullscreen.
   FullscreenStateChanged(bool is_fullscreen);
+
+  // Updates information to determine whether a user gesture should carryover to
+  // future navigations. This is needed so navigations within a certain
+  // timeframe of a request initiated by a gesture will be treated as if they
+  // were initiated by a gesture too, otherwise the navigation may be blocked.
+  [EnableIf=is_android]
+  UpdateUserGestureCarryoverInfo();
 };
diff --git a/content/public/browser/web_contents_delegate.h b/content/public/browser/web_contents_delegate.h
@@ -594,6 +594,14 @@ class CONTENT_EXPORT WebContentsDelegate {
   // Picture-in-Picture mode has ended.
   virtual void ExitPictureInPicture();
 
+#if defined(OS_ANDROID)
+  // Updates information to determine whether a user gesture should carryover to
+  // future navigations. This is needed so navigations within a certain
+  // timeframe of a request initiated by a gesture will be treated as if they
+  // were initiated by a gesture too, otherwise the navigation may be blocked.
+  virtual void UpdateUserGestureCarryoverInfo(WebContents* web_contents) {}
+#endif
+
  protected:
   virtual ~WebContentsDelegate();
 

diff --git a/content/renderer/loader/resource_dispatcher.cc b/content/renderer/loader/resource_dispatcher.cc
@@ -196,6 +196,26 @@ void NotifyResourceTransferSizeUpdate(
   render_frame->DidReceiveTransferSizeUpdate(request_id, transfer_size_diff);
 }
 
+#if defined(OS_ANDROID)
+void NotifyUpdateUserGestureCarryoverInfo(int render_frame_id) {
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner =
+      RenderThreadImpl::DeprecatedGetMainTaskRunner();
+  if (!task_runner->BelongsToCurrentThread()) {
+    task_runner->PostTask(
+        FROM_HERE,
+        base::BindOnce(NotifyUpdateUserGestureCarryoverInfo, render_frame_id));
+    return;
+  }
+
+  RenderFrameImpl* render_frame =
+      RenderFrameImpl::FromRoutingID(render_frame_id);
+  if (!render_frame)
+    return;
+
+  render_frame->GetFrameHost()->UpdateUserGestureCarryoverInfo();
+}
+#endif
+
 // Returns true if the headers indicate that this resource should always be
 // revalidated or not cached.
 bool AlwaysAccessNetwork(
@@ -716,6 +736,13 @@ int ResourceDispatcher::StartAsync(
     base::OnceClosure* continue_navigation_function) {
   CheckSchemeForReferrerPolicy(*request);
 
+#if defined(OS_ANDROID)
+  if (request->resource_type != RESOURCE_TYPE_MAIN_FRAME &&
+      request->has_user_gesture) {
+    NotifyUpdateUserGestureCarryoverInfo(request->render_frame_id);
+  }
+#endif
+
   bool override_url_loader =
       !!response_override_params &&
       !!response_override_params->url_loader_client_endpoints;

diff --git a/content/test/test_render_frame.cc b/content/test/test_render_frame.cc
@@ -10,6 +10,7 @@
 
 #include "base/debug/stack_trace.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "build/build_config.h"
 #include "content/common/frame_messages.h"
 #include "content/common/navigation_params.h"
 #include "content/common/navigation_params.mojom.h"
@@ -112,6 +113,10 @@ class MockFrameHost : public mojom::FrameHost {
 
   void FullscreenStateChanged(bool is_fullscreen) override {}
 
+#if defined(OS_ANDROID)
+  void UpdateUserGestureCarryoverInfo() override {}
+#endif
+
  private:
   std::unique_ptr<FrameHostMsg_DidCommitProvisionalLoad_Params>
       last_commit_params_;

diff --git a/testing/buildbot/filters/mojo.fyi.network_chrome_public_test_apk.filter b/testing/buildbot/filters/mojo.fyi.network_chrome_public_test_apk.filter
@@ -17,11 +17,3 @@
 
 # https://crbug.com/721403
 -org.chromium.chrome.browser.infobar.InfoBarTest.testDataReductionPromoInfoBar
-
-# http://crbug.com/882060
--org.chromium.chrome.browser.externalnav.UrlOverridingTest.testNavigationFromXHRCallback
--org.chromium.chrome.browser.externalnav.UrlOverridingTest.testNavigationFromXHRCallbackAndShortTimeout
--org.chromium.chrome.browser.externalnav.UrlOverridingTest.testNavigationFromXHRCallbackInSubFrame
--org.chromium.chrome.browser.tab.InterceptNavigationDelegateTest.testNavigationFromImageOnLoad
--org.chromium.chrome.browser.tab.InterceptNavigationDelegateTest.testNavigationFromXHRCallback
--org.chromium.chrome.browser.tab.InterceptNavigationDelegateTest.testNavigationFromXHRCallbackAndShortTimeout