[Extensions] Rework inline installation observation

chrome/browser/extensions/tab_helper.cc

 // Copyright (c) 2012 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/extensions/tab_helper.h"
 
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "build/build_config.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/activity_log/activity_log.h"
 #include "chrome/browser/extensions/api/declarative_content/chrome_content_rules_registry.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
-#include "chrome/browser/extensions/api/webstore/webstore_api.h"
 #include "chrome/browser/extensions/bookmark_app_helper.h"
 #include "chrome/browser/extensions/chrome_extension_web_contents_observer.h"
 #include "chrome/browser/extensions/extension_action_runner.h"
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/extensions/extension_util.h"
+#include "chrome/browser/extensions/install_observer.h"
+#include "chrome/browser/extensions/install_tracker.h"
+#include "chrome/browser/extensions/install_tracker_factory.h"
 #include "chrome/browser/extensions/location_bar_controller.h"
 #include "chrome/browser/extensions/webstore_inline_installer.h"
 #include "chrome/browser/extensions/webstore_inline_installer_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "chrome/browser/shell_integration.h"
 #include "chrome/browser/ui/browser_commands.h"
 #include "chrome/browser/ui/browser_dialogs.h"
 #include "chrome/browser/ui/browser_finder.h"
 #include "chrome/browser/web_applications/web_app.h"
+#include "chrome/common/extensions/api/webstore/webstore_api_constants.h"
 #include "chrome/common/extensions/chrome_extension_messages.h"
 #include "chrome/common/extensions/extension_constants.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/render_messages.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/invalidate_type.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/notification_service.h"
@@ skipping 24 matching lines @@
 #endif
 
 using content::NavigationController;
 using content::NavigationEntry;
 using content::WebContents;
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(extensions::TabHelper);
 
 namespace extensions {
 
+// A helper class to watch the progress of inline installation and update the
+// renderer. Owned by the TabHelper.
+class TabHelper::InlineInstallObserver : public InstallObserver {
+ public:
+  InlineInstallObserver(TabHelper* tab_helper,
+                        content::BrowserContext* browser_context,
+                        int routing_id,
+                        const std::string& extension_id,
+                        bool observe_download_progress,
+                        bool observe_install_stage)
+      : tab_helper_(tab_helper),
+        routing_id_(routing_id),
+        extension_id_(extension_id),
+        observe_download_progress_(observe_download_progress),
+        observe_install_stage_(observe_install_stage),
+        install_observer_(this) {
+    DCHECK(observe_download_progress || observe_install_stage);

[lazyboy, 2016/06/29 21:03:03]

DCHECK(tab_helper_)

[Devlin, 2016/06/29 22:32:09]

Sure, can't hurt.

+    InstallTracker* install_tracker =
+        InstallTrackerFactory::GetForBrowserContext(browser_context);
+    if (install_tracker)
+      install_observer_.Add(install_tracker);
+  }
+  ~InlineInstallObserver() override {}
+
+ private:
+  // InstallObserver:
+  void OnBeginExtensionDownload(const std::string& extension_id) override {
+    SendInstallStageChangedMessage(extension_id,
+                                   api::webstore::INSTALL_STAGE_DOWNLOADING);
+  }
+  void OnDownloadProgress(const std::string& extension_id,
+                          int percent_downloaded) override {
+    if (observe_download_progress_ && extension_id == extension_id_) {
+      tab_helper_->Send(new ExtensionMsg_InlineInstallDownloadProgress(
+                            routing_id_, percent_downloaded));
+    }
+  }
+  void OnBeginCrxInstall(const std::string& extension_id) override {
+    SendInstallStageChangedMessage(extension_id,
+                                   api::webstore::INSTALL_STAGE_INSTALLING);
+  }
+  void OnShutdown() override {
+    install_observer_.RemoveAll();
+  }
+
+  void SendInstallStageChangedMessage(const std::string& extension_id,
+                                      api::webstore::InstallStage stage) {
+    if (observe_install_stage_ && extension_id == extension_id_) {
+      tab_helper_->Send(new ExtensionMsg_InlineInstallStageChanged(
+                            routing_id_, stage));
+    }
+  }
+
+  // The owning TabHelper (guaranteed to be valid).
+  TabHelper* tab_helper_;

[lazyboy, 2016/06/29 21:03:04]

Mark this and last two bool members const.

[Devlin, 2016/06/29 22:32:09]

Done (assuming you mean TabHelper* const tab_helper_, and not const TabHelper*
tab_helper_ - Send() is not const so the latter is impossible).

[lazyboy, 2016/06/29 23:10:27]

Yes that's what I meant.
, and not const TabHelper*

+
+  // The routing id to use in sending IPC updates.
+  int routing_id_;
+
+  // The id of the extension to observe.
+  std::string extension_id_;
+
+  // Whether or not to observe download/install progress.
+  bool observe_download_progress_;
+  bool observe_install_stage_;
+
+  ScopedObserver<InstallTracker, InstallObserver> install_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(InlineInstallObserver);
+};
+
 TabHelper::TabHelper(content::WebContents* web_contents)
     : content::WebContentsObserver(web_contents),
       profile_(Profile::FromBrowserContext(web_contents->GetBrowserContext())),
       extension_app_(NULL),
       pending_web_app_action_(NONE),
       last_committed_nav_entry_unique_id_(0),
       update_shortcut_on_load_complete_(false),
       script_executor_(
           new ScriptExecutor(web_contents, &script_execution_observers_)),
       location_bar_controller_(new LocationBarController(web_contents)),
@@ skipping 277 matching lines @@
                                         const GURL& requestor_url,
                                         int listeners_mask) {
   // Check that the listener is reasonable. We should never get anything other
   // than an install stage listener, a download listener, or both.
   if ((listeners_mask & ~(api::webstore::INSTALL_STAGE_LISTENER |
                           api::webstore::DOWNLOAD_PROGRESS_LISTENER)) != 0 ||
       requestor_url.is_empty()) {
     NOTREACHED();
     return;
   }
+
+  if (pending_inline_installations_.count(webstore_item_id) != 0) {

[lazyboy, 2016/06/29 21:03:03]

This seems like a new behavior? Test would be helpful :)

[Devlin, 2016/06/29 22:32:09]

D'oh, I knew you would say that! ;)

So, looking into this, it's actually rather painful.
- We have a check renderer side as well that prevents duplicate installs early.
- But that won't catch all cases - e.g., if two separate JS contexts call it.
- They emit separate error messages. :(

I added a test for the most common case - two calls from the same JS context -
but I'm not sure it's worth it to manufacture a test for the other.  Let me know
what you think.

[lazyboy, 2016/06/29 23:10:27]

Thanks!
Maybe file a bug (GoodFirstBug?) to add the test?
I can clearly see how the test can be painful to write :), I'd file a bug
instead then with some notes about this as this is not an uncommon use case.
Just for a note to put in the bug: I'd say window.open and window.opener might
be helpful for the test to execute 2 install calls from 2 different JS contexts.
Let me know

[Devlin, 2016/06/30 14:46:55]

Filed a bug: 624614

+    Send(new ExtensionMsg_InlineWebstoreInstallResponse(
+        return_route_id,
+        install_id,
+        false,
+        "An install is already in progress",

[lazyboy, 2016/06/29 21:03:03]

This needs to be in a constant, pull out the constant from
webstore_standalone_installer.cc/kInstallInProgressError to
webstore_constants.cc

[Devlin, 2016/06/29 22:32:09]

Done.

+        webstore_install::INSTALL_IN_PROGRESS));
+    return;
+  }
+
+  pending_inline_installations_.insert(webstore_item_id);
   // Inform the Webstore API that an inline install is happening, in case the
   // page requested status updates.
   ExtensionRegistry* registry = ExtensionRegistry::Get(profile_);
   if (registry->disabled_extensions().Contains(webstore_item_id) &&
       (ExtensionPrefs::Get(profile_)->GetDisableReasons(webstore_item_id) &
            Extension::DISABLE_PERMISSIONS_INCREASE) != 0) {
       // The extension was disabled due to permissions increase. Prompt for
       // re-enable.
       // TODO(devlin): We should also prompt for re-enable for other reasons,
       // like user-disabled.
       // For clarity, explicitly end any prior reenable process.
       extension_reenabler_.reset();
       extension_reenabler_ = ExtensionReenabler::PromptForReenable(

[lazyboy, 2016/06/29 21:03:03]

It's not obvious to me how this will end up clearing webstore_item_id from
pending_inline_installations_.

nvm, I see it will eventually call OnInlineInstallComplete

[Devlin, 2016/06/29 22:32:09]

Acknowledged.

           registry->disabled_extensions().GetByID(webstore_item_id),
           profile_,
           web_contents(),
           requestor_url,
           base::Bind(&TabHelper::OnReenableComplete,
                      weak_ptr_factory_.GetWeakPtr(),
                      install_id,
-                     return_route_id));
+                     return_route_id,
+                     webstore_item_id));
   } else {
     // TODO(devlin): We should adddress the case of the extension already

[lazyboy, 2016/06/29 21:03:04]

Doesn't the beginning of the function prevent this (extension already being
installed) now?

[Devlin, 2016/06/29 22:32:09]

Nope.  The beginning prevents two inline installations at the same time, but if
the item is already installed and enabled, it doesn't.

[lazyboy, 2016/06/29 23:10:27]

Acknowledged.

     // being installed and enabled.
-    WebstoreAPI::Get(profile_)->OnInlineInstallStart(
-        return_route_id, this, webstore_item_id, listeners_mask);
+    bool observe_download_progress =
+        (listeners_mask & api::webstore::DOWNLOAD_PROGRESS_LISTENER) != 0;
+    bool observe_install_stage =
+        (listeners_mask & api::webstore::INSTALL_STAGE_LISTENER) != 0;
+    if (observe_install_stage || observe_download_progress) {
+      DCHECK(install_observers_.find(webstore_item_id) ==

[lazyboy, 2016/06/29 21:03:03]

can use install_observers_.count(webstore_item_id) == 0

[Devlin, 2016/06/29 22:32:09]

Done.

+                 install_observers_.end());
+      install_observers_[webstore_item_id] =
+          base::MakeUnique<InlineInstallObserver>(
+              this, web_contents()->GetBrowserContext(), return_route_id,
+              webstore_item_id, observe_download_progress,
+              observe_install_stage);
+    }
 
     WebstoreStandaloneInstaller::Callback callback =
         base::Bind(&TabHelper::OnInlineInstallComplete,
-                   base::Unretained(this),
+                   weak_ptr_factory_.GetWeakPtr(),
                    install_id,
-                   return_route_id);
+                   return_route_id,
+                   webstore_item_id);
     scoped_refptr<WebstoreInlineInstaller> installer(
         webstore_inline_installer_factory_->CreateInstaller(
             web_contents(), host, webstore_item_id, requestor_url, callback));
     installer->BeginInstall();
   }
 }
 
 void TabHelper::OnGetAppInstallState(content::RenderFrameHost* host,
                                      const GURL& requestor_url,
                                      int return_route_id,
@@ skipping 73 matching lines @@
     web_contents()->NotifyNavigationStateChanged(content::INVALIDATE_TYPE_TAB);
   }
 }
 
 WindowController* TabHelper::GetExtensionWindowController() const  {
   return ExtensionTabUtil::GetWindowControllerOfTab(web_contents());
 }
 
 void TabHelper::OnReenableComplete(int install_id,
                                    int return_route_id,
+                                   const std::string& extension_id,
                                    ExtensionReenabler::ReenableResult result) {
-  extension_reenabler_.reset();
   // Map the re-enable results to webstore-install results.
   webstore_install::Result webstore_result = webstore_install::SUCCESS;
   std::string error;
   switch (result) {
     case ExtensionReenabler::REENABLE_SUCCESS:
       break;  // already set
     case ExtensionReenabler::USER_CANCELED:
       webstore_result = webstore_install::USER_CANCELLED;
       error = "User canceled install.";
       break;
     case ExtensionReenabler::NOT_ALLOWED:
       webstore_result = webstore_install::NOT_PERMITTED;
       error = "Install not permitted.";
       break;
     case ExtensionReenabler::ABORTED:
       webstore_result = webstore_install::ABORTED;
       error = "Aborted due to tab closing.";
       break;
   }
 
   OnInlineInstallComplete(install_id,
                           return_route_id,
+                          extension_id,
                           result == ExtensionReenabler::REENABLE_SUCCESS,
                           error,
                           webstore_result);
+  // Note: ExtensionReenabler contained the callback with the curried-in
+  // extension id; delete it last.

[lazyboy, 2016/06/29 21:03:03]

|extension_id|?

[Devlin, 2016/06/29 22:32:09]

Done.

+  extension_reenabler_.reset();
 }
 
 void TabHelper::OnInlineInstallComplete(int install_id,
                                         int return_route_id,
+                                        const std::string& extension_id,
                                         bool success,
                                         const std::string& error,
                                         webstore_install::Result result) {
+  DCHECK_EQ(1u, pending_inline_installations_.count(extension_id));
+  pending_inline_installations_.erase(extension_id);
+  install_observers_.erase(extension_id);
   Send(new ExtensionMsg_InlineWebstoreInstallResponse(
       return_route_id,
       install_id,
       success,
       success ? std::string() : error,
       result));
 }
 
 WebContents* TabHelper::GetAssociatedWebContents() const {
   return web_contents();
@@ skipping 39 matching lines @@
   }
 }
 
 void TabHelper::SetTabId(content::RenderFrameHost* render_frame_host) {
   render_frame_host->Send(
       new ExtensionMsg_SetTabId(render_frame_host->GetRoutingID(),
                                 SessionTabHelper::IdForTab(web_contents())));
 }
 
 }  // namespace extensions