Allow chrome.management.setEnabled() to re-enable crashed extensions.

Allow chrome.management.setEnabled() to re-enable crashed extensions.

crrev.com/10832237, take two. Removed code that treated crashed extensions as a special case. Now, developers can reload their crashed extensions like so: setEnabled(false); setEnabled(true);

BUG=131977
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=155456

[asargent_no_longer_on_chrome, 2012-09-06 16:30:48]

I'm swamped at the moment and need to defer some reviews for a few days -
if this is time sensitive please send to another reviewer.

On Wed, Sep 5, 2012 at 12:29 PM, <mitchellwrosen@chromium.org> wrote:

> Reviewers: Antony Sargent,
>
> Description:
> Allow chrome.management.setEnabled() to re-enable crashed extensions.
>
> crrev.com/10832237, take two. Removed code that treated crashed
> extensions as a
> special case. Now, developers can reload their crashed extensions like so:
> setEnabled(false); setEnabled(true);
>
> BUG=131977
>
>
> Please review this at
https://chromiumcodereview.**appspot.com/10912105/<https://chromiumcodereview...
>
> SVN Base:
http://git.chromium.org/**chromium/src.git@master<http://git.chromium.org/chr...
>
> Affected files:
>   M chrome/browser/extensions/api/**management/management_api.cc
>   M chrome/browser/extensions/api/**management/management_api_**
> browsertest.cc
>   M chrome/browser/extensions/**extension_service.cc
>
>
> Index: chrome/browser/extensions/api/**management/management_api.cc
> diff --git a/chrome/browser/extensions/**api/management/management_api.**cc
> b/chrome/browser/extensions/**api/management/management_api.**cc
> index bfe42aebb2960408182bbee525440d**1a851ce0f8..**
> d21d35a589000253435bb2a4bcb7c4**34b9afd4ec 100644
> --- a/chrome/browser/extensions/**api/management/management_api.**cc
> +++ b/chrome/browser/extensions/**api/management/management_api.**cc
> @@ -454,20 +454,6 @@ bool SetEnabledFunction::RunImpl() {
>      return false;
>    }
>
> -  // The extension may have been unloaded due to a crash.
> -  if (service()->**GetTerminatedExtension(**extension_id_)) {
> -    if (params->enabled) {
> -      service()->ReloadExtension(**extension_id_);
> -    } else {
> -      // Treat setEnabled(false) when the extension has crashed as a
> no-op.
> -      BrowserThread::PostTask(
> -          BrowserThread::UI,
> -          FROM_HERE,
> -          base::Bind(&**SetEnabledFunction::**SendResponse, this, true));
> -      return true;
> -    }
> -  }
> -
>    const extensions::ManagementPolicy* policy =
> extensions::ExtensionSystem::**Get(
>        profile())->management_policy(**);
>    if (!policy->**UserMayModifySettings(**extension, NULL)) {
> Index: chrome/browser/extensions/api/**management/management_api_**
> browsertest.cc
> diff --git
a/chrome/browser/extensions/**api/management/management_api_**browsertest.cc
> b/chrome/browser/extensions/**api/management/management_api_**
> browsertest.cc
> index 86841564b186df7a26946afb6e9eee**02a4d2861e..**
> 84c1b45e799ff316cda51407329749**e58182e73a 100644
> --- a/chrome/browser/extensions/**api/management/management_api_**
> browsertest.cc
> +++ b/chrome/browser/extensions/**api/management/management_api_**
> browsertest.cc
> @@ -222,16 +222,12 @@ IN_PROC_BROWSER_TEST_F(**
> ExtensionManagementApiEscalati**onTest,
>        switches::**kAppsGalleryInstallAutoConfirm**ForTests, "accept");
>    SetEnabled(true, true, "");
>
> -  // Crash the extension and try to enable. The extension should be
> reloaded and
> -  // enabled.
> -  ASSERT_TRUE(**CrashEnabledExtension(kId));
> -  SetEnabled(true, true, "");
> -
> -  // Crash the extension again and try to disable. This call should be a
> no-op
> -  // and the extension should remain unloaded.
> +  // Crash the extension. Mock a reload by disabling and then enabling.
> The
> +  // extension should be reloaded and enabled.
>    ASSERT_TRUE(**CrashEnabledExtension(kId));
>    SetEnabled(false, true, "");
> +  SetEnabled(true, true, "");
>    const extensions::Extension* extension = browser()->profile()->
> -      GetExtensionService()->**GetTerminatedExtension(kId);
> -  ASSERT_TRUE(extension);
> +      GetExtensionService()->**GetExtensionById(kId, false);
> +  EXPECT_TRUE(extension);
>  }
> Index: chrome/browser/extensions/**extension_service.cc
> diff --git a/chrome/browser/extensions/**extension_service.cc
> b/chrome/browser/extensions/**extension_service.cc
> index 354a661b1e95d9ab7c56e1e8c87aca**e1218fdf22..**
> 6b551202350d5a426614ea207eaffc**272e9d621e 100644
> --- a/chrome/browser/extensions/**extension_service.cc
> +++ b/chrome/browser/extensions/**extension_service.cc
> @@ -897,14 +897,16 @@ void ExtensionService::**DisableExtension(
>    if (!extension)
>      return;
>
> -  // Move it over to the disabled list.
> +  // Move it over to the disabled list. Don't send a second unload
> notification
> +  // for terminated extensions being disabled.
> +  // extensions that
>    disabled_extensions_.Insert(**make_scoped_refptr(extension))**;
> -  if (extensions_.Contains(**extension->id()))
> +  if (extensions_.Contains(**extension->id())) {
>      extensions_.Remove(extension->**id());
> -  else
> +    NotifyExtensionUnloaded(**extension, extension_misc::UNLOAD_REASON_**
> DISABLE);
> +  } else {
>      terminated_extensions_.Remove(**extension->id());
> -
> -  NotifyExtensionUnloaded(**extension, extension_misc::UNLOAD_REASON_**
> DISABLE);
> +  }
>
>    SyncExtensionChangeIfNeeded(***extension);
>
>
>
>

[mitchellwrosen, 2012-09-06 18:34:35]

On 2012/09/06 16:30:48, Antony Sargent wrote:
> I'm swamped at the moment and need to defer some reviews for a few days -
> if this is time sensitive please send to another reviewer.

Sure thing. Not time sensitive, but I'll pass it off to yoz to lighten your
load.

[Yoyo Zhou, 2012-09-06 20:59:22]

LGTM

[commit-bot: I haz the power, 2012-09-06 21:22:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10912105/3001

[commit-bot: I haz the power, 2012-09-06 23:07:02]

Try job failure for 10912105-3001 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-09-07 01:12:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10912105/3001

[commit-bot: I haz the power, 2012-09-07 01:14:23]

Try job failure for 10912105-3001 on win for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-09-07 17:14:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mitchellwrosen@chromium.org/10912105/3001

[commit-bot: I haz the power, 2012-09-07 19:44:55]

Change committed as 155456