Make ArcNavigationThrottle a content::WebContentsObserver.

This CL ensures that querying for ARC apps is gracefully aborted if the
owning WebContents is destroyed during the asynchronous process.

ArcNavigationThrottle is converted to be a self-owning object with a
private constructor and destructor. When the static app query methods
are invoked, an instance of the object is instantiated to handle the
querying. By overriding WebContentsDestroyed(), it can cancel all
in-flight callbacks upon WebContents destruction and safely bail out of
querying.

BUG=824598
TBR=sky@chromium.org

Change-Id: I2ba6d391b9babec46b277cbcebe567786215bf5a
Reviewed-on: https://chromium-review.googlesource.com/1003432
Commit-Queue: Dominick Ng <dominickn@chromium.org>
Reviewed-by: Yusuke Sato <yusukes@chromium.org>
Reviewed-by: David Jacobo <djacobo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#551537}

chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.cc

@@ -133,19 +133,13 @@ AppsNavigationThrottle::MaybeCreate(content::NavigationHandle* handle) {
 
 // static
 void AppsNavigationThrottle::ShowIntentPickerBubble(
-    const Browser* browser,
     content::WebContents* web_contents,
     const GURL& url) {
-  // TODO(crbug.com/824598): remove |browser| as a parameter for this method,
-  // and instead factor out asychronous querying to a helper object that is
-  // a WebContentsObserver and self-deletes when querying is complete or if the
-  // WebContents is destroyed. Always lookup |browser| via the WebContents. This
-  // will avoid lifetime issues.
-  arc::ArcNavigationThrottle::QueryArcApps(
-      browser, url,
+  arc::ArcNavigationThrottle::GetArcAppsForPicker(
+      web_contents, url,
       base::BindOnce(
           &AppsNavigationThrottle::FindPwaForUrlAndShowIntentPickerForApps,
-          browser, web_contents, url));
+          web_contents, url));
 }
 
 // static
@@ -219,8 +213,7 @@ AppsNavigationThrottle::AppsNavigationThrottle(
     content::NavigationHandle* navigation_handle,
     bool arc_enabled)
     : content::NavigationThrottle(navigation_handle),
-      arc_throttle_(arc_enabled ? std::make_unique<arc::ArcNavigationThrottle>()
-                                : nullptr),
+      arc_enabled_(arc_enabled),
       ui_displayed_(false),
       weak_factory_(this) {}
 
@@ -317,15 +310,13 @@ AppsNavigationThrottle::PickerAction AppsNavigationThrottle::GetPickerAction(
 
 // static
 void AppsNavigationThrottle::FindPwaForUrlAndShowIntentPickerForApps(
-    const Browser* browser,
     content::WebContents* web_contents,
     const GURL& url,
     std::vector<IntentPickerAppInfo> apps) {
   std::vector<IntentPickerAppInfo> apps_for_picker =
       FindPwaForUrl(web_contents, url, std::move(apps));
 
-  ShowIntentPickerBubbleForApps(browser, web_contents, url,
-                                std::move(apps_for_picker));
+  ShowIntentPickerBubbleForApps(web_contents, url, std::move(apps_for_picker));
 }
 
 // static
@@ -357,15 +348,17 @@ std::vector<IntentPickerAppInfo> AppsNavigationThrottle::FindPwaForUrl(
 
 // static
 void AppsNavigationThrottle::ShowIntentPickerBubbleForApps(
-    const Browser* browser,
     content::WebContents* web_contents,
     const GURL& url,
     std::vector<IntentPickerAppInfo> apps) {
   if (apps.empty())
     return;
 
+  // It should be safe to bind |web_contents| since closing the current tab will
+  // close the intent picker and run the callback prior to the WebContents being
+  // deallocated.
   chrome::ShowIntentPickerBubble(
-      browser, std::move(apps),
+      chrome::FindBrowserWithWebContents(web_contents), std::move(apps),
       base::BindOnce(&AppsNavigationThrottle::OnIntentPickerClosed,
                      web_contents, url));
 }
@@ -378,7 +371,7 @@ void AppsNavigationThrottle::CancelNavigation() {
     CancelDeferredNavigation(content::NavigationThrottle::CANCEL_AND_IGNORE);
 }
 
-void AppsNavigationThrottle::OnDeferredRequestProcessed(
+void AppsNavigationThrottle::OnDeferredNavigationProcessed(
     AppsNavigationAction action,
     std::vector<IntentPickerAppInfo> apps) {
   if (action == AppsNavigationAction::CANCEL) {
@@ -399,9 +392,7 @@ void AppsNavigationThrottle::OnDeferredRequestProcessed(
   if (apps_for_picker.empty())
     ui_displayed_ = false;
 
-  ShowIntentPickerBubbleForApps(
-      chrome::FindBrowserWithWebContents(web_contents), web_contents, url,
-      std::move(apps_for_picker));
+  ShowIntentPickerBubbleForApps(web_contents, url, std::move(apps_for_picker));
 
   // We are about to resume the navigation, which may destroy this object.
   Resume();
@@ -435,16 +426,16 @@ AppsNavigationThrottle::HandleRequest() {
   if (!ShouldOverrideUrlLoading(starting_url_, url))
     return content::NavigationThrottle::PROCEED;
 
-  if (arc_throttle_ &&
-      arc_throttle_->ShouldDeferRequest(
+  if (arc_enabled_ &&
+      arc::ArcNavigationThrottle::WillGetArcAppsForNavigation(
           handle,
-          base::BindOnce(&AppsNavigationThrottle::OnDeferredRequestProcessed,
+          base::BindOnce(&AppsNavigationThrottle::OnDeferredNavigationProcessed,
                          weak_factory_.GetWeakPtr()))) {
-    // Handling is now deferred to |arc_throttle_|, which asynchronously queries
-    // ARC for apps, and runs OnDeferredRequestProcessed() with an action based
-    // on whether an acceptable app was found and user consent to open received.
-    // We assume the UI is shown or a preferred app was found; reset to false if
-    // we resume the navigation.
+    // Handling is now deferred to ArcNavigationThrottle, which asynchronously
+    // queries ARC for apps, and runs OnDeferredNavigationProcessed() with an
+    // action based on whether an acceptable app was found and user consent to
+    // open received. We assume the UI is shown or a preferred app was found;
+    // reset to false if we resume the navigation.
     ui_displayed_ = true;
     return content::NavigationThrottle::DEFER;
   }
@@ -459,9 +450,7 @@ AppsNavigationThrottle::HandleRequest() {
     if (!apps.empty())
       ui_displayed_ = true;
 
-    ShowIntentPickerBubbleForApps(
-        chrome::FindBrowserWithWebContents(web_contents), web_contents, url,
-        std::move(apps));
+    ShowIntentPickerBubbleForApps(web_contents, url, std::move(apps));
   }
 
   return content::NavigationThrottle::PROCEED;

chrome/browser/chromeos/apps/intent_helper/apps_navigation_throttle.h

@@ -15,12 +15,6 @@
 #include "content/public/browser/navigation_throttle.h"
 #include "url/gurl.h"
 
-class Browser;
-
-namespace arc {
-class ArcNavigationThrottle;
-}
-
 namespace content {
 class NavigationHandle;
 class WebContents;
@@ -43,10 +37,9 @@ class AppsNavigationThrottle : public content::NavigationThrottle {
   static std::unique_ptr<content::NavigationThrottle> MaybeCreate(
       content::NavigationHandle* handle);
 
-  // Queries for installed app which can handle |url|, and displays the intent
-  // picker bubble in |browser|.
-  static void ShowIntentPickerBubble(const Browser* browser,
-                                     content::WebContents* web_contents,
+  // Queries for installed apps which can handle |url|, and displays the intent
+  // picker bubble for |web_contents|.
+  static void ShowIntentPickerBubble(content::WebContents* web_contents,
                                      const GURL& url);
 
   // Called when the intent picker is closed for |url|, in |web_contents|, with
@@ -139,7 +132,6 @@ class AppsNavigationThrottle : public content::NavigationThrottle {
       bool should_persist);
 
   static void FindPwaForUrlAndShowIntentPickerForApps(
-      const Browser* browser,
       content::WebContents* web_contents,
       const GURL& url,
       std::vector<IntentPickerAppInfo> apps);
@@ -152,7 +144,6 @@ class AppsNavigationThrottle : public content::NavigationThrottle {
       std::vector<IntentPickerAppInfo> apps);
 
   static void ShowIntentPickerBubbleForApps(
-      const Browser* browser,
       content::WebContents* web_contents,
       const GURL& url,
       std::vector<IntentPickerAppInfo> apps);
@@ -161,17 +152,18 @@ class AppsNavigationThrottle : public content::NavigationThrottle {
 
   content::NavigationThrottle::ThrottleCheckResult HandleRequest();
 
-  // Passed as a callback to allow app-platform-specific code to asychronously
-  // inform this object of the apps which can handle this URL, and optionally
-  // request that the navigation be completely cancelled (e.g. if a preferred
-  // app has been opened).
-  void OnDeferredRequestProcessed(AppsNavigationAction action,
-                                  std::vector<IntentPickerAppInfo> apps);
+  // Passed as a callback to allow ARC-specific code to asynchronously inform
+  // this object of the apps which can handle this URL, and optionally request
+  // that the navigation be completely cancelled (e.g. if a preferred app has
+  // been opened).
+  void OnDeferredNavigationProcessed(AppsNavigationAction action,
+                                     std::vector<IntentPickerAppInfo> apps);
 
   // A reference to the starting GURL.
   GURL starting_url_;
 
-  std::unique_ptr<arc::ArcNavigationThrottle> arc_throttle_;
+  // True if ARC is enabled, false otherwise.
+  const bool arc_enabled_;
 
   // Keeps track of whether we already shown the UI or preferred app. Since
   // AppsNavigationThrottle cannot wait for the user (due to the non-blocking

chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h

@@ -87,7 +87,7 @@ using AppsNavigationCallback =
 
 // Callback to allow app-platform-specific code to asynchronously provide a list
 // of apps which can handle the navigation.
-using QueryAppsCallback =
+using GetAppsCallback =
     base::OnceCallback<void(std::vector<IntentPickerAppInfo> apps)>;
 
 }  // namespace chromeos

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

@@ -12,11 +12,11 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/chromeos/apps/intent_helper/apps_navigation_types.h"
+#include "components/arc/arc_bridge_service.h"
 #include "components/arc/intent_helper/arc_intent_helper_bridge.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "url/gurl.h"
 
-class Browser;
-
 namespace content {
 class NavigationHandle;
 class WebContents;
@@ -24,19 +24,31 @@ class WebContents;
 
 namespace arc {
 
-// A class that allow us to retrieve ARC app's information and handle URL
-// traffic initiated on Chrome browser, either on Chrome or an ARC's app.
-class ArcNavigationThrottle {
+// A class that allow us to retrieve installed ARC apps which can handle
+// a particular URL.
+class ArcNavigationThrottle : content::WebContentsObserver {
  public:
-  ArcNavigationThrottle();
-  ~ArcNavigationThrottle();
+  // Retrieves ARC apps which can handle |url| for |web_contents|, and runs
+  // |callback| when complete. Does not attempt to open preferred apps.
+  static void GetArcAppsForPicker(content::WebContents* web_contents,
+                                  const GURL& url,
+                                  chromeos::GetAppsCallback callback);
 
   // Returns true if the navigation request represented by |handle| should be
   // deferred while ARC is queried for apps, and if so, |callback| will be run
   // asynchronously with the action for the navigation. |callback| will not be
   // run if false is returned.
-  bool ShouldDeferRequest(content::NavigationHandle* handle,
-                          chromeos::AppsNavigationCallback callback);
+  static bool WillGetArcAppsForNavigation(
+      content::NavigationHandle* handle,
+      chromeos::AppsNavigationCallback callback);
+
+  // Called to launch an ARC app if it was selected by the user, and persist the
+  // preference to launch or stay in Chrome if |should_persist| is true. Returns
+  // true if an app was launched, and false otherwise.
+  static bool MaybeLaunchOrPersistArcApp(const GURL& url,
+                                         const std::string& package_name,
+                                         bool should_launch,
+                                         bool should_persist);
 
   // Finds |selected_app_package| from the |app_candidates| array and returns
   // the index. If the app is not found, returns |app_candidates.size()|.
@@ -57,56 +69,65 @@ class ArcNavigationThrottle {
       const std::vector<mojom::IntentHandlerInfoPtr>& app_candidates);
   static size_t FindPreferredAppForTesting(
       const std::vector<mojom::IntentHandlerInfoPtr>& app_candidates);
-  static void QueryArcApps(const Browser* browser,
-                           const GURL& url,
-                           chromeos::QueryAppsCallback callback);
 
-  // Called to launch an ARC app if it was selected by the user, and persist the
-  // preference to launch or stay in Chrome if |should_persist| is true. Returns
-  // true if an app was launched, and false otherwise.
-  static bool MaybeLaunchOrPersistArcApp(const GURL& url,
-                                         const std::string& package_name,
-                                         bool should_launch,
-                                         bool should_persist);
+  ~ArcNavigationThrottle() override;
 
  private:
-  // Determines whether we should open a preferred app or show the intent
-  // picker. Resume/Cancel the navigation which was put in DEFER. Close the
-  // current tab only if we continue the navigation on ARC and the current tab
-  // was explicitly generated for this navigation.
-  void OnAppCandidatesReceived(
-      content::NavigationHandle* handle,
+  explicit ArcNavigationThrottle(content::WebContents* web_contents);
+
+  // Asychronously queries ARC for apps which can handle |url|. Runs |callback|
+  // with RESUME/CANCEL for the deferred navigation and (if applicable) the list
+  // of handling apps.
+  void GetArcAppsForNavigation(mojom::IntentHelperInstance* instance,
+                               const GURL& url,
+                               chromeos::AppsNavigationCallback callback);
+
+  // Asychronously queries ARC for apps which can handle |url|. Runs |callback|
+  // with the list of handling apps.
+  void GetArcAppsForPicker(mojom::IntentHelperInstance* instance,
+                           const GURL& url,
+                           chromeos::GetAppsCallback callback);
+
+  // Determines if there are apps to show the intent picker, or if we should
+  // open a preferred app. Runs |callback| to RESUME/CANCEL the navigation which
+  // was deferred prior to calling this method, and (if applicable) the list of
+  // apps to show in the picker. The current tab is only closed if we continue
+  // the navigation on ARC and the current tab was explicitly generated for this
+  // navigation.
+  void OnAppCandidatesReceivedForNavigation(
+      const GURL& url,
       chromeos::AppsNavigationCallback callback,
       std::vector<mojom::IntentHandlerInfoPtr> app_candidates);
 
+  // Determines if there are apps to show the intent picker. Runs |callback|
+  // with the list of apps to show in the picker.
+  void OnAppCandidatesReceivedForPicker(
+      const GURL& url,
+      chromeos::GetAppsCallback callback,
+      std::vector<arc::mojom::IntentHandlerInfoPtr> app_candidates);
+
   // Returns true if an app in |app_candidates| is preferred for handling the
   // navigation represented by |handle|, and we are successfully able to launch
   // it.
   bool DidLaunchPreferredArcApp(
       const GURL& url,
-      content::WebContents* web_contents,
       const std::vector<mojom::IntentHandlerInfoPtr>& app_candidates);
 
   // Queries the ArcIntentHelperBridge for ARC app icons for the apps in
   // |app_candidates|. Calls OnAppIconsReceived() when finished.
-  void ArcAppIconQuery(const GURL& url,
-                       content::WebContents* web_contents,
-                       std::vector<mojom::IntentHandlerInfoPtr> app_candidates,
-                       chromeos::QueryAppsCallback callback);
+  void GetArcAppIcons(const GURL& url,
+                      std::vector<mojom::IntentHandlerInfoPtr> app_candidates,
+                      chromeos::GetAppsCallback callback);
 
-  static void AsyncOnAppCandidatesReceived(
-      const Browser* browser,
+  void OnAppIconsReceived(
       const GURL& url,
-      chromeos::QueryAppsCallback callback,
-      std::vector<arc::mojom::IntentHandlerInfoPtr> app_candidates);
-
-  static void OnAppIconsReceived(
-      const Browser* browser,
       std::vector<arc::mojom::IntentHandlerInfoPtr> app_candidates,
-      const GURL& url,
-      chromeos::QueryAppsCallback callback,
+      chromeos::GetAppsCallback callback,
       std::unique_ptr<arc::ArcIntentHelperBridge::ActivityToIconsMap> icons);
 
+  // content::WebContentsObserver overrides.
+  void WebContentsDestroyed() override;
+
   // This has to be the last member of the class.
   base::WeakPtrFactory<ArcNavigationThrottle> weak_ptr_factory_;
 

chrome/browser/ui/views/location_bar/intent_picker_view.cc

@@ -47,8 +47,7 @@ void IntentPickerView::OnExecuting(
         browser_->tab_strip_model()->GetActiveWebContents();
     const GURL& url = chrome::GetURLToBookmark(web_contents);
 
-    chromeos::AppsNavigationThrottle::ShowIntentPickerBubble(browser_,
-                                                             web_contents, url);
+    chromeos::AppsNavigationThrottle::ShowIntentPickerBubble(web_contents, url);
   } else {
     SetVisible(false);
   }