Modifies how Arc's throttle handles redirections

chrome/browser/chromeos/arc/arc_navigation_throttle.cc

 // Copyright 2016 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 "chrome/browser/chromeos/arc/arc_navigation_throttle.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
 #include "base/metrics/histogram_macros.h"
 #include "chrome/browser/chromeos/arc/page_transition_util.h"
 #include "components/arc/arc_bridge_service.h"
 #include "components/arc/arc_service_manager.h"
 #include "components/arc/intent_helper/arc_intent_helper_bridge.h"
 #include "components/arc/intent_helper/local_activity_resolver.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/site_instance.h"
 #include "content/public/browser/web_contents.h"
 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "ui/base/page_transition_types.h"
-#include "url/gurl.h"
 
 namespace arc {
 
 namespace {
 
 constexpr uint32_t kMinVersionForHandleUrl = 2;
 constexpr uint32_t kMinVersionForRequestUrlHandlerList = 2;
 constexpr uint32_t kMinVersionForAddPreferredPackage = 7;
 
 scoped_refptr<ActivityIconLoader> GetIconLoader() {
@@ skipping 11 matching lines @@
   // it in the desktop browser.
   if (!previous_url.is_valid() || previous_url.is_empty())
     return false;
 
   // Also check |current_url| just in case.
   if (!current_url.is_valid() || current_url.is_empty()) {
     DVLOG(1) << "Unexpected URL: " << current_url << ", opening it in Chrome.";
     return false;
   }
 
+  // Check the scheme for both |previous_url| and |current_url| since an
+  // extension could have referred us (e.g. Google Docs).
+  if (!current_url.SchemeIsHTTPOrHTTPS() ||
+      !previous_url.SchemeIsHTTPOrHTTPS()) {
+    return false;
+  }
+
   return !net::registry_controlled_domains::SameDomainOrHost(
       current_url, previous_url,
       net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
 }
 
 }  // namespace
 
 ArcNavigationThrottle::ArcNavigationThrottle(
     content::NavigationHandle* navigation_handle,
     const ShowIntentPickerCallback& show_intent_picker_cb)
     : content::NavigationThrottle(navigation_handle),
       show_intent_picker_callback_(show_intent_picker_cb),
       previous_user_action_(CloseReason::INVALID),
       weak_ptr_factory_(this) {}
 
 ArcNavigationThrottle::~ArcNavigationThrottle() = default;
 
 content::NavigationThrottle::ThrottleCheckResult
 ArcNavigationThrottle::WillStartRequest() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-  // We must not handle navigations started from the context menu.
-  if (navigation_handle()->WasStartedFromContextMenu())
-    return content::NavigationThrottle::PROCEED;
+  starting_gurl_ = GetStartingGURL();
   return HandleRequest();
 }
 
 content::NavigationThrottle::ThrottleCheckResult
 ArcNavigationThrottle::WillRedirectRequest() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   switch (previous_user_action_) {
     case CloseReason::ERROR:
     case CloseReason::DIALOG_DEACTIVATED:
-      // User dismissed the dialog, or some error occurred before.  Don't
+      // User dismissed the dialog, or some error occurred before. Don't
       // repeatedly pop up the dialog.
       return content::NavigationThrottle::PROCEED;
 
     case CloseReason::ALWAYS_PRESSED:
     case CloseReason::JUST_ONCE_PRESSED:
     case CloseReason::PREFERRED_ACTIVITY_FOUND:
-      // Should never get here - if the user selected one of these previously,
-      // Chrome should not see a redirect.
-      NOTREACHED();
+      // We must never show the intent picker for the same throttle more than
+      // once and we must considerate that we may have redirections within the
+      // same ArcNavigationThrottle even after seeing the UI and selecting an
+      // app to handle the navigation. This section can be reached iff the user
+      // selected Chrome to continue the navigation, since Resume() tells the
+      // throttle to continue with the chain of redirections.
+      //
+      // For example, by clicking a youtube link on gmail you can see the
+      // following URLs, assume our |starting_gurl_| is "http://www.google.com":
+      //
+      // 1) https://www.google.com/url?hl=en&q=https://youtube.com/watch?v=fake
+      // 2) https://youtube.com/watch?v=fake
+      // 3) https://www.youtube.com/watch?v=fake
+      //
+      // 1) was caught via WillStartRequest() and 2) and 3) are caught via
+      // WillRedirectRequest().Step 2) triggers the intent picker and step 3)
+      // will be seen iff the user picks Chrome, or if Chrome was marked as the
+      // preferred app for this kind of URL. This happens since after choosing
+      // Chrome we tell the throttle to Resume(), thus allowing for further
+      // redirections.
+      return content::NavigationThrottle::PROCEED;
 
     case CloseReason::INVALID:
       // No picker has previously been popped up for this - continue.
       break;
   }
   return HandleRequest();
 }
 
 content::NavigationThrottle::ThrottleCheckResult
 ArcNavigationThrottle::HandleRequest() {
   const GURL& url = navigation_handle()->GetURL();
 
   // Always handle http(s) <form> submissions in Chrome for two reasons: 1) we
   // don't have a way to send POST data to ARC, and 2) intercepting http(s) form
   // submissions is not very important because such submissions are usually
   // done within the same domain. ShouldOverrideUrlLoading() below filters out
   // such submissions anyway.
   constexpr bool kAllowFormSubmit = false;
 
+  // We must not handle navigations started from the context menu.

[Yusuke Sato, 2016/11/03 19:57:11]

Please rebase once I finish landing the first one.

[djacobo_, 2016/11/03 20:22:01]

Done.

+  if (navigation_handle()->WasStartedFromContextMenu())
+    return content::NavigationThrottle::PROCEED;
+
   if (ShouldIgnoreNavigation(navigation_handle()->GetPageTransition(),
                              kAllowFormSubmit))
     return content::NavigationThrottle::PROCEED;
 
-  const GURL referrer_url = navigation_handle()->GetReferrer().url;
-  const GURL current_url = navigation_handle()->GetURL();
-  const GURL last_committed_url =
-      navigation_handle()->GetWebContents()->GetLastCommittedURL();
-
-  // For navigations from http to https we clean up the Referrer as part of the
-  // sanitization proccess, however we may still have access to the last
-  // committed URL. On the other hand, navigations started within a new tab
-  // (e.g. due to target="_blank") will keep no track of any previous entries
-  // and so GetLastCommittedURL() can be seen empty sometimes, this is why we
-  // use one or the other accordingly. Also we don't use GetVisibleURL() since
-  // it may contain a still non-committed URL (i.e. it can be the same as
-  // GetURL()).
-  const GURL previous_url =
-      referrer_url.is_empty() ? last_committed_url : referrer_url;
-
-  if (!ShouldOverrideUrlLoading(previous_url, current_url))
+  if (!ShouldOverrideUrlLoading(starting_gurl_, url))
     return content::NavigationThrottle::PROCEED;
 
   ArcServiceManager* arc_service_manager = ArcServiceManager::Get();
   DCHECK(arc_service_manager);
   scoped_refptr<LocalActivityResolver> local_resolver =
       arc_service_manager->activity_resolver();
   if (local_resolver->ShouldChromeHandleUrl(url)) {
     // Allow navigation to proceed if there isn't an android app that handles
     // the given URL.
     return content::NavigationThrottle::PROCEED;
   }
 
   auto* instance = ArcIntentHelperBridge::GetIntentHelperInstance(
       "RequestUrlHandlerList", kMinVersionForRequestUrlHandlerList);
   if (!instance)
     return content::NavigationThrottle::PROCEED;
   instance->RequestUrlHandlerList(
       url.spec(), base::Bind(&ArcNavigationThrottle::OnAppCandidatesReceived,
                              weak_ptr_factory_.GetWeakPtr()));
   return content::NavigationThrottle::DEFER;
 }
 
+GURL ArcNavigationThrottle::GetStartingGURL() const {

[Yusuke Sato, 2016/11/03 19:57:11]

Can you comment which line is different?

[djacobo_, 2016/11/03 20:22:01]

Oh OK didn't see that, let me add a comment.

[Yusuke Sato, 2016/11/03 20:27:31]

Oh sorry I meant comment on rietveld (not code comment) but either is fine.  The
merge process for this file will always be manual because of the directory
change and the additional code comment won't make it worse.

+  // This helps us determine a reference GURL for the current NavigationHandle.
+  // This is the order or preferrence: Referrer > LastCommittedURL > SiteURL,
+  // GetSiteURL *should* only be used on very rare cases, e.g. when the
+  // navigation goes from https: to http: on a new tab, thus losing the other
+  // potential referrers.
+  const GURL referrer_url = navigation_handle()->GetReferrer().url;
+  if (referrer_url.is_valid() && !referrer_url.is_empty())
+    return referrer_url;
+
+  const GURL last_committed_url =
+      navigation_handle()->GetWebContents()->GetLastCommittedURL();
+  if (last_committed_url.is_valid() && !last_committed_url.is_empty())
+    return last_committed_url;
+
+  return navigation_handle()->GetWebContents()->GetSiteInstance()->GetSiteURL();
+}
+
 // We received the array of app candidates to handle this URL (even the Chrome
 // app is included).
 void ArcNavigationThrottle::OnAppCandidatesReceived(
     mojo::Array<mojom::IntentHandlerInfoPtr> handlers) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (handlers.empty() ||
       (handlers.size() == 1 && ArcIntentHelperBridge::IsIntentHelperPackage(
                                    handlers[0]->package_name))) {
     // This scenario shouldn't be accesed as ArcNavigationThrottle is created
     // iff there are ARC apps which can actually handle the given URL.
@@ skipping 152 matching lines @@
 }
 
 // static
 bool ArcNavigationThrottle::ShouldOverrideUrlLoadingForTesting(
     const GURL& previous_url,
     const GURL& current_url) {
   return ShouldOverrideUrlLoading(previous_url, current_url);
 }
 
 }  // namespace arc