Reland "AppShell: Support reloading"

This is a reland of 65478d12 with
Aura-specific tests moved to a subclass and #ifdef.

Original change's description:
> AppShell: Support reloading
>
> Implement reloading in ExtensionLoader. Uses keep-alives while apps are
> reloading. A future CL will observe these keep-alives to keep app_shell open
> during reload.
>
> Bug: 762642
> Change-Id: Ia20b81378d1aeab4ace119d9d2a8b44c2461fdcf
> Reviewed-on: https://chromium-review.googlesource.com/912694
> Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#540079}

Bug: 762642
Change-Id: I6c516c4736ca2327965e034d10717341e23303dc
Reviewed-on: https://chromium-review.googlesource.com/950342
Commit-Queue: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542195}

extensions/browser/extension_registrar.h

@@ -111,8 +111,8 @@ class ExtensionRegistrar {
   // retains a reference to it, so it can be enabled later.
   void DisableExtension(const ExtensionId& extension_id, int disable_reasons);
 
-  // Reloads the specified extension by disabling it if it is enabled and
-  // requesting the Delegate load it again.
+  // Attempts to reload the specified extension by disabling it if it is enabled
+  // and requesting the Delegate load it again.
   // NOTE: Reloading an extension can invalidate |extension_id| and Extension
   // pointers for the given extension. Consider making a copy of |extension_id|
   // first and retrieving a new Extension pointer afterwards.

extensions/browser/extension_registrar_unittest.cc

@@ -323,6 +323,26 @@ class ExtensionRegistrarTest : public ExtensionsTest {
                   ->GetDisableReasons(extension()->id()));
   }
 
+  // Directs ExtensionRegistrar to reload the terminated extension and verifies
+  // the delegate is invoked correctly.
+  void ReloadTerminatedExtension() {
+    SCOPED_TRACE("ReloadTerminatedExtension");
+    EXPECT_CALL(delegate_,
+                LoadExtensionForReload(extension()->id(), extension()->path(),
+                                       LoadErrorBehavior::kNoisy));
+    registrar()->ReloadExtension(extension()->id(), LoadErrorBehavior::kNoisy);
+    VerifyMock();
+
+    // The extension should remain in the terminated set until the reload
+    // completes successfully.
+    ExpectInSet(ExtensionRegistry::TERMINATED);
+    // Unlike when reloading an enabled extension, the extension hasn't been
+    // disabled and shouldn't have the DISABLE_RELOAD disable reason.
+    EXPECT_EQ(disable_reason::DISABLE_NONE,
+              ExtensionPrefs::Get(browser_context())
+                  ->GetDisableReasons(extension()->id()));
+  }
+
   // Verifies that the extension is in the given set in the ExtensionRegistry
   // and not in other sets.
   void ExpectInSet(ExtensionRegistry::IncludeFlag set_id) {
@@ -442,12 +462,14 @@ TEST_F(ExtensionRegistrarTest, AddForceDisabled) {
 TEST_F(ExtensionRegistrarTest, AddBlacklisted) {
   AddBlacklistedExtension();
 
-  // A blacklisted extension cannot be enabled/disabled.
+  // A blacklisted extension cannot be enabled/disabled/reloaded.
   registrar()->EnableExtension(extension()->id());
   ExpectInSet(ExtensionRegistry::BLACKLISTED);
   registrar()->DisableExtension(extension()->id(),
                                 disable_reason::DISABLE_USER_ACTION);
   ExpectInSet(ExtensionRegistry::BLACKLISTED);
+  registrar()->ReloadExtension(extension()->id(), LoadErrorBehavior::kQuiet);
+  ExpectInSet(ExtensionRegistry::BLACKLISTED);
 
   RemoveBlacklistedExtension();
 }
@@ -489,7 +511,7 @@ TEST_F(ExtensionRegistrarTest, DisableTerminatedExtension) {
   RemoveDisabledExtension();
 }
 
-TEST_F(ExtensionRegistrarTest, ReloadTerminatedExtension) {
+TEST_F(ExtensionRegistrarTest, EnableTerminatedExtension) {
   AddEnabledExtension();
   TerminateExtension();
 
@@ -515,6 +537,23 @@ TEST_F(ExtensionRegistrarTest, RemoveReloadedExtension) {
   // Simulate the delegate failing to load the extension and removing it
   // instead.
   RemoveDisabledExtension();
+
+  // Attempting to reload it silently fails.
+  registrar()->ReloadExtension(extension()->id(), LoadErrorBehavior::kQuiet);
+  ExpectInSet(ExtensionRegistry::NONE);
+}
+
+TEST_F(ExtensionRegistrarTest, ReloadTerminatedExtension) {
+  AddEnabledExtension();
+  TerminateExtension();
+
+  // Reload the terminated extension.
+  ReloadTerminatedExtension();
+
+  // Complete the reload by adding the extension. Expect the extension to be
+  // enabled once re-added to the registrar, since ExtensionPrefs shouldn't say
+  // it's disabled.
+  AddEnabledExtension();
 }
 
 }  // namespace extensions

extensions/shell/browser/shell_extension_loader.cc

@@ -4,10 +4,17 @@
 
 #include "extensions/shell/browser/shell_extension_loader.h"
 
+#include "apps/launcher.h"
+#include "base/auto_reset.h"
+#include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
+#include "base/sequenced_task_runner.h"
+#include "base/task_runner_util.h"
+#include "extensions/browser/extension_file_task_runner.h"
 #include "extensions/browser/extension_prefs.h"
+#include "extensions/browser/extension_registry.h"
 #include "extensions/common/file_util.h"
 
 namespace extensions {
@@ -53,7 +60,9 @@ scoped_refptr<const Extension> LoadUnpacked(
 ShellExtensionLoader::ShellExtensionLoader(
     content::BrowserContext* browser_context)
     : browser_context_(browser_context),
-      extension_registrar_(browser_context, this) {}
+      extension_registrar_(browser_context, this),
+      keep_alive_requester_(browser_context),
+      weak_factory_(this) {}
 
 ShellExtensionLoader::~ShellExtensionLoader() = default;
 
@@ -66,6 +75,47 @@ const Extension* ShellExtensionLoader::LoadExtension(
   return extension.get();
 }
 
+void ShellExtensionLoader::ReloadExtension(ExtensionId extension_id) {
+  const Extension* extension = ExtensionRegistry::Get(browser_context_)
+                                   ->GetInstalledExtension(extension_id);
+  // We shouldn't be trying to reload extensions that haven't been added.
+  DCHECK(extension);
+
+  // This should always start false since it's only set here, or in
+  // LoadExtensionForReload() as a result of the call below.
+  DCHECK_EQ(false, did_schedule_reload_);
+  base::AutoReset<bool> reset_did_schedule_reload(&did_schedule_reload_, false);
+
+  // Set up a keep-alive while the extension reloads. Do this before starting
+  // the reload so that the first step, disabling the extension, doesn't release
+  // the last remaining keep-alive and shut down the application.
+  keep_alive_requester_.StartTrackingReload(extension);
+  extension_registrar_.ReloadExtension(extension_id, LoadErrorBehavior::kQuiet);
+  if (did_schedule_reload_)
+    return;
+
+  // ExtensionRegistrar didn't invoke us to schedule the reload, so the reload
+  // wasn't actually started. Clear the keep-alive so we don't wait forever.
+  keep_alive_requester_.StopTrackingReload(extension_id);
+}
+
+void ShellExtensionLoader::FinishExtensionReload(
+    const ExtensionId old_extension_id,
+    scoped_refptr<const Extension> extension) {
+  if (extension) {
+    extension_registrar_.AddExtension(extension);
+    // If the extension is a platform app, adding it above caused
+    // ShellKeepAliveRequester to create a new keep-alive to wait for the app to
+    // open its first window.
+    // Launch the app now.
+    if (extension->is_platform_app())
+      apps::LaunchPlatformApp(browser_context_, extension.get(), SOURCE_RELOAD);
+  }
+
+  // Whether or not the reload succeeded, we should stop waiting for it.
+  keep_alive_requester_.StopTrackingReload(old_extension_id);
+}
+
 void ShellExtensionLoader::PreAddExtension(const Extension* extension,
                                            const Extension* old_extension) {
   if (old_extension)
@@ -97,7 +147,14 @@ void ShellExtensionLoader::LoadExtensionForReload(
     const ExtensionId& extension_id,
     const base::FilePath& path,
     LoadErrorBehavior load_error_behavior) {
-  // TODO(michaelpg): Support reload.
+  CHECK(!path.empty());
+
+  base::PostTaskAndReplyWithResult(
+      GetExtensionFileTaskRunner().get(), FROM_HERE,
+      base::BindOnce(&LoadUnpacked, path),
+      base::BindOnce(&ShellExtensionLoader::FinishExtensionReload,
+                     weak_factory_.GetWeakPtr(), extension_id));
+  did_schedule_reload_ = true;
 }
 
 bool ShellExtensionLoader::CanEnableExtension(const Extension* extension) {

extensions/shell/browser/shell_extension_loader.h

@@ -13,6 +13,7 @@
 #include "base/memory/weak_ptr.h"
 #include "extensions/browser/extension_registrar.h"
 #include "extensions/common/extension_id.h"
+#include "extensions/shell/browser/shell_keep_alive_requester.h"
 
 namespace base {
 class FilePath;
@@ -26,7 +27,7 @@ namespace extensions {
 
 class Extension;
 
-// Handles extension loading using ExtensionRegistrar.
+// Handles extension loading and reloading using ExtensionRegistrar.
 class ShellExtensionLoader : public ExtensionRegistrar::Delegate {
  public:
   explicit ShellExtensionLoader(content::BrowserContext* browser_context);
@@ -36,7 +37,19 @@ class ShellExtensionLoader : public ExtensionRegistrar::Delegate {
   // extension on success, or nullptr otherwise.
   const Extension* LoadExtension(const base::FilePath& extension_dir);
 
+  // Starts reloading the extension. A keep-alive is maintained until the
+  // reload succeeds/fails. If the extension is an app, it will be launched upon
+  // reloading.
+  // This may invalidate references to the old Extension object, so it takes the
+  // ID by value.
+  void ReloadExtension(ExtensionId extension_id);
+
  private:
+  // If the extension loaded successfully, enables it. If it's an app, launches
+  // it. If the load failed, updates ShellKeepAliveRequester.
+  void FinishExtensionReload(const ExtensionId old_extension_id,
+                             scoped_refptr<const Extension> extension);
+
   // ExtensionRegistrar::Delegate:
   void PreAddExtension(const Extension* extension,
                        const Extension* old_extension) override;
@@ -56,6 +69,16 @@ class ShellExtensionLoader : public ExtensionRegistrar::Delegate {
   // Registers and unregisters extensions.
   ExtensionRegistrar extension_registrar_;
 
+  // Holds keep-alives for relaunching apps.
+  ShellKeepAliveRequester keep_alive_requester_;
+
+  // Indicates that we posted the (asynchronous) task to start reloading.
+  // Used by ReloadExtension() to check whether ExtensionRegistrar calls
+  // LoadExtensionForReload().
+  bool did_schedule_reload_ = false;
+
+  base::WeakPtrFactory<ShellExtensionLoader> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ShellExtensionLoader);
 };
 

extensions/shell/browser/shell_extension_system_factory.cc

@@ -4,6 +4,7 @@
 
 #include "extensions/shell/browser/shell_extension_system_factory.h"
 
+#include "apps/app_lifetime_monitor_factory.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "extensions/browser/extension_prefs_factory.h"
 #include "extensions/browser/extension_registry_factory.h"
@@ -29,6 +30,7 @@ ShellExtensionSystemFactory::ShellExtensionSystemFactory()
                               BrowserContextDependencyManager::GetInstance()) {
   DependsOn(ExtensionPrefsFactory::GetInstance());
   DependsOn(ExtensionRegistryFactory::GetInstance());
+  DependsOn(apps::AppLifetimeMonitorFactory::GetInstance());
 }
 
 ShellExtensionSystemFactory::~ShellExtensionSystemFactory() {

extensions/shell/browser/shell_keep_alive_requester.cc

@@ -47,11 +47,9 @@ void ShellKeepAliveRequester::OnExtensionLoaded(
     return;
 
   // Add a keep-alive to wait for the app to launch its first app window, as
-  // otherwise the Aura desktop controller will exit.
-  // This assumes that all platform apps will be launched when loaded, which is
-  // true in AppShell. (The launched app may decline to create a window, in
-  // which case this keep-alive will be erased once the app's background page
-  // eventually stops.
+  // otherwise the Aura desktop controller may exit. The assumption is that all
+  // apps will create a visible window. If the app doesn't, this keep-alive will
+  // still be erased once the app's background page eventually stops.
   app_launching_keep_alives_[extension->id()] =
       std::make_unique<ScopedKeepAlive>(KeepAliveOrigin::APP_CONTROLLER,
                                         KeepAliveRestartOption::ENABLED);

extensions/shell/test/shell_test_base_aura.cc

@@ -4,15 +4,9 @@
 
 #include "extensions/shell/test/shell_test_base_aura.h"
 
-#include "base/memory/ptr_util.h"
-#include "content/public/browser/web_contents.h"
 #include "content/public/test/test_browser_thread_bundle.h"
-#include "extensions/browser/app_window/app_window.h"
-#include "extensions/browser/app_window/test_app_window_contents.h"
-#include "extensions/shell/browser/shell_app_delegate.h"
 #include "extensions/shell/test/shell_test_extensions_browser_client.h"
 #include "extensions/shell/test/shell_test_helper_aura.h"
-#include "url/gurl.h"
 
 namespace extensions {
 
@@ -37,25 +31,7 @@ void ShellTestBaseAura::TearDown() {
 
 void ShellTestBaseAura::InitAppWindow(AppWindow* app_window,
                                       const gfx::Rect& bounds) {
-  // Create a TestAppWindowContents for the ShellAppDelegate to initialize the
-  // ShellExtensionWebContentsObserver with.
-  std::unique_ptr<content::WebContents> web_contents(
-      content::WebContents::Create(
-          content::WebContents::CreateParams(browser_context())));
-  std::unique_ptr<TestAppWindowContents> app_window_contents =
-      std::make_unique<TestAppWindowContents>(std::move(web_contents));
-
-  // Initialize the web contents and AppWindow.
-  app_window->app_delegate()->InitWebContents(
-      app_window_contents->GetWebContents());
-
-  content::RenderFrameHost* main_frame =
-      app_window_contents->GetWebContents()->GetMainFrame();
-  DCHECK(main_frame);
-
-  AppWindow::CreateParams params;
-  params.content_spec.bounds = bounds;
-  app_window->Init(GURL(), app_window_contents.release(), main_frame, params);
+  helper_->InitAppWindow(app_window, bounds);
 }
 
 }  // namespace extensions

extensions/shell/test/shell_test_helper_aura.cc

@@ -4,9 +4,16 @@
 
 #include "extensions/shell/test/shell_test_helper_aura.h"
 
+#include <memory>
+
+#include "content/public/browser/web_contents.h"
+#include "extensions/browser/app_window/app_window.h"
+#include "extensions/browser/app_window/test_app_window_contents.h"
+#include "extensions/shell/browser/shell_app_delegate.h"
 #include "ui/aura/test/aura_test_helper.h"
 #include "ui/compositor/compositor.h"
 #include "ui/compositor/test/context_factories_for_test.h"
+#include "url/gurl.h"
 
 namespace extensions {
 
@@ -33,4 +40,27 @@ void ShellTestHelperAura::TearDown() {
   ui::TerminateContextFactoryForTests();
 }
 
+void ShellTestHelperAura::InitAppWindow(AppWindow* app_window,
+                                        const gfx::Rect& bounds) {
+  // Create a TestAppWindowContents for the ShellAppDelegate to initialize the
+  // ShellExtensionWebContentsObserver with.
+  std::unique_ptr<content::WebContents> web_contents(
+      content::WebContents::Create(
+          content::WebContents::CreateParams(app_window->browser_context())));
+  auto app_window_contents =
+      std::make_unique<TestAppWindowContents>(std::move(web_contents));
+
+  // Initialize the web contents and AppWindow.
+  app_window->app_delegate()->InitWebContents(
+      app_window_contents->GetWebContents());
+
+  content::RenderFrameHost* main_frame =
+      app_window_contents->GetWebContents()->GetMainFrame();
+  DCHECK(main_frame);
+
+  AppWindow::CreateParams params;
+  params.content_spec.bounds = bounds;
+  app_window->Init(GURL(), app_window_contents.release(), main_frame, params);
+}
+
 }  // namespace extensions

extensions/shell/test/shell_test_helper_aura.h

@@ -8,6 +8,7 @@
 #include <memory>
 
 #include "base/macros.h"
+#include "ui/gfx/geometry/rect.h"
 
 namespace aura {
 namespace test {
@@ -17,6 +18,8 @@ class AuraTestHelper;
 
 namespace extensions {
 
+class AppWindow;
+
 // A helper class that does common Aura initialization required for the shell.
 class ShellTestHelperAura {
  public:
@@ -29,6 +32,9 @@ class ShellTestHelperAura {
   // Cleans up.
   void TearDown();
 
+  // Initializes |app_window| for testing.
+  void InitAppWindow(AppWindow* app_window, const gfx::Rect& bounds = {});
+
  private:
   std::unique_ptr<aura::test::AuraTestHelper> helper_;