Upstream ShouldOverrideUrlLoading changes

android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.cc

 // Copyright (c) 2012 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 "android_webview/browser/renderer_host/aw_resource_dispatcher_host_delegate.h"
 
 #include <string>
 
 #include "android_webview/browser/aw_contents_io_thread_client.h"
 #include "android_webview/browser/aw_login_delegate.h"
@@ skipping 49 matching lines @@
   IoThreadClientThrottle(int child_id,
                          int route_id,
                          net::URLRequest* request);
   virtual ~IoThreadClientThrottle();
 
   // From content::ResourceThrottle
   virtual void WillStartRequest(bool* defer) OVERRIDE;
   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();
   int get_child_id() const { return child_id_; }
   int get_route_id() const { return route_id_; }
 
 private:
   int child_id_;
   int route_id_;
   net::URLRequest* request_;
 };
 
 IoThreadClientThrottle::IoThreadClientThrottle(int child_id,
                                                int route_id,
                                                net::URLRequest* request)
     : child_id_(child_id),
       route_id_(route_id),
       request_(request) { }
 
 IoThreadClientThrottle::~IoThreadClientThrottle() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   g_webview_resource_dispatcher_host_delegate.Get().
       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) {
     // OPTIONS is used for preflighted requests which are generated internally.
     DCHECK_EQ("OPTIONS", request_->method());
     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
   // client so that the client will get a chance to override requests.
   scoped_ptr<AwContentsIoThreadClient> io_client =
       AwContentsIoThreadClient::FromID(child_id_, route_id_);
   if (io_client && io_client->PendingAssociation()) {
     *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,
                                                    int new_route_id) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   if (!MaybeBlockRequest()) {
     controller()->Resume();
   }
 }
@@ skipping 68 matching lines @@
 }
 
 void AwResourceDispatcherHostDelegate::RequestBeginning(
     net::URLRequest* request,
     content::ResourceContext* resource_context,
     appcache::AppCacheService* appcache_service,
     ResourceType::Type resource_type,
     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(
     net::URLRequest* request,
     content::ResourceContext* resource_context,
     int child_id,
     int route_id,
     int request_id,
     bool is_content_initiated,
     bool must_download,
@@ skipping 148 matching lines @@
       ChildRouteIDPair(new_child_id, new_route_id));
 
   if (it != pending_throttles_.end()) {
     IoThreadClientThrottle* throttle = it->second;
     throttle->OnIoThreadClientReady(new_child_id, new_route_id);
     pending_throttles_.erase(it);
   }
 }
 
 }  // namespace android_webview