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..dcdda39acd11a5933b06385b343eb1168eb4483a 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

[Yusuke Sato, 2016/10/29 00:39:57]

Please update the unit test.

[djacobo_, 2016/10/31 20:14:43]

Done.

+  // 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);
@@ -119,6 +126,8 @@ ArcNavigationThrottle::ArcNavigationThrottle(
     : content::NavigationThrottle(navigation_handle),
       show_intent_picker_callback_(show_intent_picker_cb),
       previous_user_action_(CloseReason::INVALID),
+      starting_gurl_(GURL()),

[Yusuke Sato, 2016/10/29 00:39:57]

redundant initialization, please remove the line.

[djacobo_, 2016/10/31 20:14:44]

Done.

+      redirected_to_arc_(false),
       weak_ptr_factory_(this) {}
 
 ArcNavigationThrottle::~ArcNavigationThrottle() = default;
@@ -126,6 +135,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 +145,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 +155,14 @@ 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 siltently took care of
+      // those cases.
+      return redirected_to_arc_ ? content::NavigationThrottle::CANCEL_AND_IGNORE

[Yusuke Sato, 2016/10/29 00:39:57]

Is this path really taken with redirected_to_arc_ == true? If it is, how?

I'm asking because 'redirected_to_arc_ == true' means that CANCEL_AND_IGNORE
should have already been called at line 349-350.

[djacobo_, 2016/10/31 20:14:44]

Thanks for catching this, I misunderstood what PROCEED does, the path
redirect_to_arc_ == true will never be taken as CANCEL_AND_IGNORE already took
place after selecting an app from the UI, so it's safe to  always return PROCEED
here. Done.

+                                : content::NavigationThrottle::PROCEED;
 
     case CloseReason::INVALID:
       // No picker has previously been popped up for this - continue.
@@ -172,23 +186,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 +209,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(
@@ -329,6 +345,7 @@ void ArcNavigationThrottle::OnIntentPickerClosed(
         handle->Resume();
       } else {
         instance->HandleUrl(url.spec(), selected_app_package);
+        redirected_to_arc_ = true;
         handle->CancelDeferredNavigation(
             content::NavigationThrottle::CANCEL_AND_IGNORE);
         if (handle->GetWebContents()->GetController().IsInitialNavigation())