Upstream ShouldOverrideUrlLoading changes

android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc

Index: android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
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
index c88722bc9950603db4545b66a6a51e6ff8ba0518..8ecfb2b7ec0521f24b24caf725f8cb862e12a940 100644
--- a/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
+++ b/android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc
@@ -67,7 +67,6 @@ class IoThreadClientThrottle : public content::ResourceThrottle {
   virtual void WillRedirectRequest(const GURL& new_url, bool* defer) OVERRIDE;
   virtual const char* GetNameForLogging() const OVERRIDE;
 
-  bool MaybeDeferRequest(bool* defer);
   void OnIoThreadClientReady(int new_child_id, int new_route_id);
   bool MaybeBlockRequest();
   bool ShouldBlockRequest();
@@ -93,6 +92,10 @@ IoThreadClientThrottle::~IoThreadClientThrottle() {
       RemovePendingThrottleOnIoThread(this);
 }
 
+const char* IoThreadClientThrottle::GetNameForLogging() const {
+  return "IoThreadClientThrottle";
+}
+
 void IoThreadClientThrottle::WillStartRequest(bool* defer) {
   // TODO(sgurun): This block can be removed when crbug.com/277937 is fixed.
   if (route_id_ < 1) {
@@ -101,21 +104,6 @@ void IoThreadClientThrottle::WillStartRequest(bool* defer) {
     return;
   }
   DCHECK(child_id_);
-  if (!MaybeDeferRequest(defer)) {
-    MaybeBlockRequest();
-  }
-}
-
-void IoThreadClientThrottle::WillRedirectRequest(const GURL& new_url,
-                                                 bool* defer) {
-  WillStartRequest(defer);
-}
-
-const char* IoThreadClientThrottle::GetNameForLogging() const {
-  return "IoThreadClientThrottle";
-}
-
-bool IoThreadClientThrottle::MaybeDeferRequest(bool* defer) {
   *defer = false;
 
   // Defer all requests of a pop up that is still not associated with Java
@@ -126,8 +114,14 @@ bool IoThreadClientThrottle::MaybeDeferRequest(bool* defer) {
     *defer = true;
     AwResourceDispatcherHostDelegate::AddPendingThrottle(
         child_id_, route_id_, this);
+  } else {
+    MaybeBlockRequest();
   }
-  return *defer;
+}
+
+void IoThreadClientThrottle::WillRedirectRequest(const GURL& new_url,
+                                                 bool* defer) {
+  WillStartRequest(defer);

[boliu, 2013/12/06 20:28:23]

This can skip the if (io_client && io_client->PendingAssociation()) block in
WillStartRequest.

Also this runs on IO thread right? Can we add a DCHECK? Actually this whole file
seem to be lacking these.

[sgurun-gerrit only, 2013/12/07 00:11:07]

Sorry, not sure I understand.
sure.

[boliu, 2013/12/07 01:22:18]

I meant if (io_client && io_client->PendingAssociation()) returns true, then
there is no need to call AwResourceDispatcherHostDelegate::AddPendingThrottle
again, since WillStartRequest must have been called before WillRedirectRequest.
But it doesn't really hurt either.

 }
 
 void IoThreadClientThrottle::OnIoThreadClientReady(int new_child_id,
@@ -216,35 +210,21 @@ void AwResourceDispatcherHostDelegate::RequestBeginning(
     int child_id,
     int route_id,
     ScopedVector<content::ResourceThrottle>* throttles) {
-  // If io_client is NULL, then the browser side objects have already been
-  // destroyed, so do not do anything to the request. Conversely if the
-  // request relates to a not-yet-created popup window, then the client will
-  // be non-NULL but PopupPendingAssociation() will be set.
-  scoped_ptr<AwContentsIoThreadClient> io_client =
-      AwContentsIoThreadClient::FromID(child_id, route_id);
-  if (!io_client)
-    return;
 
+  // We always push the throttles here. Checking the existence of io_client
+  // is racy when a popup window is created. That is because RequestBeginning
+  // is called whether or not requests are blocked via BlockRequestForRoute()
+  // however io_client may or may not be ready at the time depending on whether
+  // webcontents is created.
   throttles->push_back(new IoThreadClientThrottle(
       child_id, route_id, request));

[boliu, 2013/12/06 20:28:23]

Trying to make sure requests of webviews that already have been destroyed will
go through uninterrupted:

This adds throttle, and in throttle, WillStartRequest should fall into calling
MaybeBlockRequest, which calls ShouldBlockRequest, which does not have an
io_client so just early outs and this whole thing does nothing. Same for
WillRedirectRequest. Correct?

So freaking need a unit test for this.

[sgurun-gerrit only, 2013/12/07 00:11:07]

Yes this logic is pretty delicate.
The CTS that I added (testShouldOverrideUrlLoadingOnCreateWindow) partially
tests this logic -- in fact this is how we discovered the raciness issue and
then ordered things this way.
But I agree on a unit test. created crbug.com/326676

 
-  bool allow_intercepting =
-      // We allow intercepting navigations within subframes, but only if the
-      // scheme other than http or https. This is because the embedder
-      // can't distinguish main frame and subframe callbacks (which could lead
-      // to broken content if the embedder decides to not ignore the main frame
-      // navigation, but ignores the subframe navigation).
-      // The reason this is supported at all is that certain JavaScript-based
-      // frameworks use iframe navigation as a form of communication with the
-      // embedder.
-      (resource_type == ResourceType::MAIN_FRAME ||
-       (resource_type == ResourceType::SUB_FRAME &&
-        !request->url().SchemeIs(content::kHttpScheme) &&
-        !request->url().SchemeIs(content::kHttpsScheme)));
-  if (allow_intercepting) {
+  // We allow intercepting only navigations within main frames. This

[boliu, 2013/12/06 20:28:23]

How about this (assuming I understood the code correctly):

We add InterceptNavigationDelegate throttle for main frames only because the
throttle is only used to handle onPageStarted callback. shouldOverrideUrlLoading
is handled separately by a sync IPC.

[sgurun-gerrit only, 2013/12/07 00:11:07]

But it is not only used for onPageStarted anymore. For popup windows, the
shouldoverrideurlloading callback is posted from the throttle (reminder: in
popup case we were sending the shouldoverrideurlloading callback to wrong
AwContents)

[boliu, 2013/12/07 01:22:18]

How about

We add InterceptNavigationDelegate throttle for main frames only because the
throttle is only used to handle onPageStarted callback and the
shouldOverrideUrlLoading callback of the initial main frame navigation of a pop
up.

+  // is used to post onPageStarted. We handle shouldOverrideUrlLoading
+  // via a sync IPC.
+  if (resource_type == ResourceType::MAIN_FRAME)
     throttles->push_back(InterceptNavigationDelegate::CreateThrottleFor(
         request));
-  }
 }
 
 void AwResourceDispatcherHostDelegate::DownloadStarting(