Modifies how Arc's throttle handles redirections

chrome/browser/chromeos/arc/intent_helper/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/intent_helper/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 "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 "components/arc/intent_helper/page_transition_util.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

[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);
 }
 
 // Returns true if |handlers| contain one or more apps. When this function is
 // called from OnAppCandidatesReceived, |handlers| always contain Chrome (aka
 // intent_helper), but the function doesn't treat it as an app.
 bool IsAppAvailable(const mojo::Array<mojom::IntentHandlerInfoPtr>& handlers) {
   return handlers.size() > 1 || (handlers.size() == 1 &&
@@ skipping 46 matching lines @@
 }
 
 }  // 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),
+      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;
 
 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;
   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
       // 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 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.
       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;
 
   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 {
+  // 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(
     mojo::Array<mojom::IntentHandlerInfoPtr> handlers) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   if (!IsAppAvailable(handlers)) {
     // This scenario shouldn't be accesed as ArcNavigationThrottle is created
     // iff there are ARC apps which can actually handle the given URL.
     DVLOG(1) << "There are no app candidates for this URL: "
              << navigation_handle()->GetURL();
@@ skipping 98 matching lines @@
       }
       // fall through.
     }
     case CloseReason::JUST_ONCE_PRESSED:
     case CloseReason::PREFERRED_ACTIVITY_FOUND: {
       if (ArcIntentHelperBridge::IsIntentHelperPackage(
               handlers[selected_app_index]->package_name)) {
         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())
           handle->GetWebContents()->Close();
       }
       break;
     }
     case CloseReason::INVALID: {
       NOTREACHED();
       return;
@@ skipping 36 matching lines @@
 }
 
 // static
 bool ArcNavigationThrottle::IsSwapElementsNeededForTesting(
     const mojo::Array<mojom::IntentHandlerInfoPtr>& handlers,
     std::pair<size_t, size_t>* out_indices) {
   return IsSwapElementsNeeded(handlers, out_indices);
 }
 
 }  // namespace arc