Modifies how Arc's throttle handles redirections

chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc

Index: chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
diff --git a/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc b/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
index 521c5f1f4d6d2c25b67067fc98fb7f59ca3a52cf..a89b9ec96302a18356fdec6952aad2023673ef93 100644
--- a/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
+++ b/chrome/browser/chromeos/arc/intent_helper/arc_navigation_throttle.cc
@@ -18,10 +18,10 @@
 #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 {
 
@@ -53,6 +53,13 @@ bool ShouldOverrideUrlLoading(const GURL& previous_url,
     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);
@@ -126,6 +133,7 @@ ArcNavigationThrottle::~ArcNavigationThrottle() = default;
 content::NavigationThrottle::ThrottleCheckResult
 ArcNavigationThrottle::WillStartRequest() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  starting_gurl_ = GetStartingGURL();
   // We must not handle navigations started from the context menu.
   if (navigation_handle()->WasStartedFromContextMenu())
     return content::NavigationThrottle::PROCEED;
@@ -135,7 +143,6 @@ ArcNavigationThrottle::WillStartRequest() {
 content::NavigationThrottle::ThrottleCheckResult
 ArcNavigationThrottle::WillRedirectRequest() {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
-
   switch (previous_user_action_) {
     case CloseReason::ERROR:
     case CloseReason::DIALOG_DEACTIVATED:
@@ -146,9 +153,13 @@ ArcNavigationThrottle::WillRedirectRequest() {
     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 even after
+      // seeing the UI and choosing an action. Previously we didn't consider
+      // this as a valid scenario since we didn't enforce having a
+      // |starting_gurl_| and so ShouldOverrideUrlLoading silently took care of
+      // those cases.
+      return content::NavigationThrottle::PROCEED;

[Yusuke Sato, 2016/10/31 20:26:51]

Thanks for removing the member variable. However, I guess I'm still a bit
confused... When do we have to return PROCEED? What's wrong with NOTREACHED? Can
you provide an example or two as a code comment (or as a CL description,
whichever you prefer)?

[djacobo_, 2016/10/31 21:12:24]

Hopefully this is better :)

 
     case CloseReason::INVALID:
       // No picker has previously been popped up for this - continue.
@@ -172,23 +183,7 @@ ArcNavigationThrottle::HandleRequest() {
                              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();
@@ -211,6 +206,24 @@ ArcNavigationThrottle::HandleRequest() {
   return content::NavigationThrottle::DEFER;
 }
 
+GURL ArcNavigationThrottle::GetStartingGURL() const {
+  // 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()->GetStartingSiteInstance()->GetSiteURL();
+}
+
 // We received the array of app candidates to handle this URL (even the Chrome
 // app is included).
 void ArcNavigationThrottle::OnAppCandidatesReceived(