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

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

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

[asargent_no_longer_on_chrome, 2012-08-10 20:26:21]

https://chromiumcodereview.appspot.com/10832237/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://chromiumcodereview.appspot.com/10832237/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/management/management_api.cc:413:
unloaded_extension_paths().find(extension_id_);
I'm not sure operating on unloaded extensions is correct, because that list can
contain both extensions that crashed ("terminated" is the term used in
ExtensionService) but also extensions that are in the process of being
uninstalled. I'm concerned that this could be a way for an extension to prevent
uninstallation of other extensions. 

Can you instead grab the terminated_extensions() list from ExtensionService and
check if the extension is in there?

https://chromiumcodereview.appspot.com/10832237/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/management/management_api_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10832237/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/management/management_api_browsertest.cc:226:
ReEnable(true, "");
can you also add a test here that calls SetEnabled(false) on a crashed
extension?

[mitchellwrosen, 2012-08-10 23:15:26]

https://chromiumcodereview.appspot.com/10832237/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://chromiumcodereview.appspot.com/10832237/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/management/management_api.cc:413:
unloaded_extension_paths().find(extension_id_);
On 2012/08/10 20:26:21, Antony Sargent wrote:
> I'm not sure operating on unloaded extensions is correct, because that list
can
> contain both extensions that crashed ("terminated" is the term used in
> ExtensionService) but also extensions that are in the process of being
> uninstalled. I'm concerned that this could be a way for an extension to
prevent
> uninstallation of other extensions. 
> 
> Can you instead grab the terminated_extensions() list from ExtensionService
and
> check if the extension is in there?

Thanks, I didn't catch the "terminated" keyword. Much simpler, now.

https://chromiumcodereview.appspot.com/10832237/diff/3001/chrome/browser/exte...
File chrome/browser/extensions/api/management/management_api_browsertest.cc
(right):

https://chromiumcodereview.appspot.com/10832237/diff/3001/chrome/browser/exte...
chrome/browser/extensions/api/management/management_api_browsertest.cc:226:
ReEnable(true, "");
On 2012/08/10 20:26:21, Antony Sargent wrote:
> can you also add a test here that calls SetEnabled(false) on a crashed
> extension?

Ya. The expected behavior is that the extension is reloaded and left disabled,
right?

[asargent_no_longer_on_chrome, 2012-08-13 19:10:34]

lgtm w/ one nit


> The expected behavior is that the extension is reloaded and > left disabled,
right?

No, I think it should be a no-op.

https://chromiumcodereview.appspot.com/10832237/diff/6/chrome/browser/extensi...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://chromiumcodereview.appspot.com/10832237/diff/6/chrome/browser/extensi...
chrome/browser/extensions/api/management/management_api.cc:445:
service()->ReloadExtension(extension_id_);
nit: I think we should only call ReloadExtension if params->enabled is true, and
make it a no-op if false. (It looks like ReloadExtension might enable the
extension, and then we'd turn right around and disable it, and I'm worried that
might result in some strange edge cases for developers who wouldn't expect that
calling setEnabled(false) might momentarily re-enable an extension).

[mitchellwrosen, 2012-08-14 00:51:31]

On 2012/08/13 19:10:34, Antony Sargent wrote:
> lgtm w/ one nit
> 
> 
> > The expected behavior is that the extension is reloaded and > left disabled,
> right?
> 
> No, I think it should be a no-op.
> 
>
https://chromiumcodereview.appspot.com/10832237/diff/6/chrome/browser/extensi...
> File chrome/browser/extensions/api/management/management_api.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10832237/diff/6/chrome/browser/extensi...
> chrome/browser/extensions/api/management/management_api.cc:445:
> service()->ReloadExtension(extension_id_);
> nit: I think we should only call ReloadExtension if params->enabled is true,
and
> make it a no-op if false. (It looks like ReloadExtension might enable the
> extension, and then we'd turn right around and disable it, and I'm worried
that
> might result in some strange edge cases for developers who wouldn't expect
that
> calling setEnabled(false) might momentarily re-enable an extension).

I took a vacation day today so I can verify this tomorrow, but if I recall
correctly ReloadExtension reloads an extension and doesn't enable it. A no-op
makes sense to me too, though.

[commit-bot: I haz the power, 2012-08-14 17:42:32]

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

[asargent_no_longer_on_chrome, 2012-08-14 17:51:38]

If ReloadExtension doesn't also cause the extension to become enabled, then
that's probably ok too, but no-op feels safer if there isn't some other
advantage to calling ReloadExtension.

BTW, can you verify that crashed extensions still show up in the list returned
by management.getAll? Also, we have a "disabled_reason"

[asargent_no_longer_on_chrome, 2012-08-14 17:52:56]

Sorry, didn't complete that last sentence. It was going to be the following:

Also, we have a "disabledReason" field in the ExtensionInfo struct that we could
set to "crashed" in the case of crashed extensions. (This could certainly just
be a separate bug that you can work on if you like or not)

[mitchellwrosen, 2012-08-14 17:54:39]

On 2012/08/14 17:52:56, Antony Sargent wrote:
> Sorry, didn't complete that last sentence. It was going to be the following:
> 
> Also, we have a "disabledReason" field in the ExtensionInfo struct that we
could
> set to "crashed" in the case of crashed extensions. (This could certainly just
> be a separate bug that you can work on if you like or not)

Sure, I'll do that right quick. Right now (off the top of my head) getAll won't
return any crashed extensions with disabled_reason UNKNOWN. I could be wrong
though. Mind if I open a new bug? Or should I cancel this commit and append it
here?

[asargent_no_longer_on_chrome, 2012-08-14 17:55:51]

A new bug is fine, this CL should be good to go as-is.


On Tue, Aug 14, 2012 at 10:54 AM, <mitchellwrosen@chromium.org> wrote:

> On 2012/08/14 17:52:56, Antony Sargent wrote:
>
>> Sorry, didn't complete that last sentence. It was going to be the
>> following:
>>
>
>  Also, we have a "disabledReason" field in the ExtensionInfo struct that we
>>
> could
>
>> set to "crashed" in the case of crashed extensions. (This could certainly
>> just
>> be a separate bug that you can work on if you like or not)
>>
>
> Sure, I'll do that right quick. Right now (off the top of my head) getAll
> won't
> return any crashed extensions with disabled_reason UNKNOWN. I could be
> wrong
> though. Mind if I open a new bug? Or should I cancel this commit and
> append it
> here?
>
>
https://chromiumcodereview.**appspot.com/10832237/<https://chromiumcodereview...
>

[commit-bot: I haz the power, 2012-08-14 20:39:22]

Try job failure for 10832237-10001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[mitchellwrosen, 2012-08-14 22:23:06]

Hrm, investigating this problem on Windows now. It's hanging on the call to
WaitForExtensionCrash(), after base::KillProcess succeeds.

[mitchellwrosen, 2012-08-14 23:10:54]

On 2012/08/14 22:23:06, mitchellwrosen wrote:
> Hrm, investigating this problem on Windows now. It's hanging on the call to
> WaitForExtensionCrash(), after base::KillProcess succeeds.

I'm leaving on vacation for a week and a half starting tomorrow, but hopefully I
can work from home at some point so I can get this in. 

I switched from using WaitForExtensionCrash to using a
WindowedNotificationObserver because the former is racy. Turns out that a
NOTIFICATION_EXTENSION_PROCESS_TERMINATED is never fired (Windows only), even
though KillProcess is succeeding. I thought it might be because the process
handle has insufficient permission (needs PROCESS_TERMINATE), but then
KillProcess would fail and I would see a log (process_win_util.cc:482). I'm
stumped.

[asargent_no_longer_on_chrome, 2012-08-14 23:35:37]

It might work to post a task to crash the extension, and then call
WaitForExtensionCrash. That way your listener for the notification about the
crash has already been setup before the extension gets crashed. E.g.

void CrashExtension(const std::string& extension_id) { ... }


... inside your test ...

BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, 
    base::Callback<void(const std::string&)>(CrashExtension, extension_id));
WaitForExtensionCrash(extension_id);

[mitchellwrosen, 2012-08-22 22:40:41]

On 2012/08/14 23:35:37, Antony Sargent wrote:
> It might work to post a task to crash the extension, and then call
> WaitForExtensionCrash. That way your listener for the notification about the
> crash has already been setup before the extension gets crashed. E.g.
> 
> void CrashExtension(const std::string& extension_id) { ... }
> 
> 
> ... inside your test ...
> 
> BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, 
>     base::Callback<void(const std::string&)>(CrashExtension, extension_id));
> WaitForExtensionCrash(extension_id);

Doesn't a WindowedNotificationObserver accomplish the same thing? The listener
is established before the crash.

[asargent_no_longer_on_chrome, 2012-08-23 17:01:57]

> Doesn't a WindowedNotificationObserver accomplish the same thing? The listener
> is established before the crash.

I think you're right, they seem similar. 

Here's another thing to consider - it looks like the way we handle terminated
extensions in ExtensionService is to listen for
NOTIFICATION_EXTENSION_PROCESS_TERMINATED, and then when we see it post a task
to add the extension to the terminated list. Any chance your test code is
assuming that has already happened when your listener sees the
NOTIFICATION_EXTENSION_PROCESS_TERMINATED message? 

Regarding killing the process, you might also investigate if you can use the
trick of navigating the background page to "about:crash" and see if that makes
any difference in whether you actually see the notification.

[mitchellwrosen, 2012-08-23 17:04:45]

On 2012/08/23 17:01:57, Antony Sargent wrote:
> > Doesn't a WindowedNotificationObserver accomplish the same thing? The
listener
> > is established before the crash.
> 
> I think you're right, they seem similar. 
> 
> Here's another thing to consider - it looks like the way we handle terminated
> extensions in ExtensionService is to listen for
> NOTIFICATION_EXTENSION_PROCESS_TERMINATED, and then when we see it post a task
> to add the extension to the terminated list. Any chance your test code is
> assuming that has already happened when your listener sees the
> NOTIFICATION_EXTENSION_PROCESS_TERMINATED message? 
> 
> Regarding killing the process, you might also investigate if you can use the
> trick of navigating the background page to "about:crash" and see if that makes
> any difference in whether you actually see the notification.

If I recall correctly (I'm on vacation right now), the
NOTIFICATION_EXTENSION_PROCESS_TERMINATED is never fired, but only on Windows.
I'll try navigating the background page to "about:crash" once I get back on
Monday.

[commit-bot: I haz the power, 2012-08-28 18:14:15]

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

[commit-bot: I haz the power, 2012-08-28 20:11:55]

Try job failure for 10832237-23001 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-28 20:24:30]

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

[commit-bot: I haz the power, 2012-08-28 23:40:14]

Try job failure for 10832237-23001 (retry) on win_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-29 15:33:01]

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

[commit-bot: I haz the power, 2012-08-29 17:36:18]

Try job failure for 10832237-23001 (retry) on linux_rel for step
"interactive_ui_tests".
It's a second try, previously, step "interactive_ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-08-29 19:59:34]

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

[commit-bot: I haz the power, 2012-08-29 22:21:08]

Change committed as 153992

[Aaron Boodman, 2012-09-04 22:47:19]

https://chromiumcodereview.appspot.com/10832237/diff/23001/chrome/browser/ext...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://chromiumcodereview.appspot.com/10832237/diff/23001/chrome/browser/ext...
chrome/browser/extensions/api/management/management_api.cc:457: // The extension
may have been unloaded due to a crash.
Does not seem right that this code is before policy check below.

https://chromiumcodereview.appspot.com/10832237/diff/23001/chrome/browser/ext...
chrome/browser/extensions/api/management/management_api.cc:460:
service()->ReloadExtension(extension_id_);
Why this special case? Just make this extension affect terminated extensions
exactly as it does non-terminated ones.

[mitchellwrosen, 2012-09-04 23:00:08]

https://chromiumcodereview.appspot.com/10832237/diff/23001/chrome/browser/ext...
File chrome/browser/extensions/api/management/management_api.cc (right):

https://chromiumcodereview.appspot.com/10832237/diff/23001/chrome/browser/ext...
chrome/browser/extensions/api/management/management_api.cc:460:
service()->ReloadExtension(extension_id_);
On 2012/09/04 22:47:19, Aaron Boodman wrote:
> Why this special case? Just make this extension affect terminated extensions
> exactly as it does non-terminated ones.

Isn't it a special case, though? Terminated extensions have to be reloaded.