Allow autoupdate to update disabled extensions.

Allow autoupdate to update disabled extensions.

BUG=26834
TEST=ExtensionUpdaterTest.TestNonAutoUpdateableLocations


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

[Matt Tytel, 2012-03-19 23:17:29]

You mentioned in the issue that we need to test flags like "this item was
disabled due to escalated permissions and needs user re-confirmation" but I
haven't found where that flag is set yet. Any direction you can point me?

[USE_CHROMIUM_ACCT_INSTEAD, 2012-03-19 23:47:25]

The permissions escalation bit gets set in
ExtensionPrefs::SetDidExtensionEscalatePermissions : http://goo.gl/Z0B7C

The general approach for testing this is to have 3 versions of a test
extension:

V1: has no permissions
V2: adds the "cookies" permission
V3: has some uninteresting change (change in description or something)

First install V1. Then update to V2, which should cause the extension to be
disabled. Then update to V3 - the extension should remain disabled.


The best way to test this might be to add a unit test in
extension_service_unittest.cc using the ExtensionService::UpdateExtension
method.

Also, having an end-to-end browsertest might be good in addition. There is
already a browsertest for autoupdate that is sort of close to this that you
could copy from (and potentially refactor to share some code with):
http://goo.gl/bzf7B.



On Mon, Mar 19, 2012 at 4:17 PM, <mtytel@chromium.org> wrote:

> You mentioned in the issue that we need to test flags like "this item was
> disabled due to escalated permissions and needs user re-confirmation" but I
> haven't found where that flag is set yet. Any direction you can point me?
>
>
https://chromiumcodereview.**appspot.com/9718028/<https://chromiumcodereview....
>

[asargent_no_longer_on_chrome, 2012-03-20 00:24:01]

https://chromiumcodereview.appspot.com/9718028/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_updater.h (right):

https://chromiumcodereview.appspot.com/9718028/diff/1/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater.h:22: #include
"chrome/common/extensions/extension_set.h"
Can you just forward declare ExtensionSet and skip the include here?

http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Forward...

https://chromiumcodereview.appspot.com/9718028/diff/1/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater.h:76: const
std::set<std::string>& pending_ids);
Any reason not to have this be private? (Typically helper methods that aren't
called outside a class should be)

https://chromiumcodereview.appspot.com/9718028/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

https://chromiumcodereview.appspot.com/9718028/diff/1/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater_unittest.cc:1366: const
std::string& disabled_id = disabled_extensions[1]->id();
Since this test ("TestNonAutoUpdateableLocations") is about testing extensions
with an empty update url, it would probably be better to just put your code in a
separate test in this file. Within your separate test, you can have two disabled
extensions, one with and one without an update url.

[Matt Tytel, 2012-03-20 20:34:28]

Man, I was a bit sloppy on that first CL.
Fixed the issues and added some tests.

https://chromiumcodereview.appspot.com/9718028/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_updater.h (right):

https://chromiumcodereview.appspot.com/9718028/diff/1/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater.h:22: #include
"chrome/common/extensions/extension_set.h"
On 2012/03/20 00:24:01, Antony Sargent wrote:
> Can you just forward declare ExtensionSet and skip the include here?
> 
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Forward...

Done.

https://chromiumcodereview.appspot.com/9718028/diff/1/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater.h:76: const
std::set<std::string>& pending_ids);
On 2012/03/20 00:24:01, Antony Sargent wrote:
> Any reason not to have this be private? (Typically helper methods that aren't
> called outside a class should be)

Done.

https://chromiumcodereview.appspot.com/9718028/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

https://chromiumcodereview.appspot.com/9718028/diff/1/chrome/browser/extensio...
chrome/browser/extensions/updater/extension_updater_unittest.cc:1366: const
std::string& disabled_id = disabled_extensions[1]->id();
On 2012/03/20 00:24:01, Antony Sargent wrote:
> Since this test ("TestNonAutoUpdateableLocations") is about testing extensions
> with an empty update url, it would probably be better to just put your code in
a
> separate test in this file. Within your separate test, you can have two
disabled
> extensions, one with and one without an update url. 

Done.

[Matt Tytel, 2012-03-20 20:36:37]

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
File chrome/browser/extensions/extension_management_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
chrome/browser/extensions/extension_management_browsertest.cc:368:
IN_PROC_BROWSER_TEST_F(ExtensionManagementTest, AutoUpdateDisabledExtensions) {
There is some overlap with the above extension, but after trying some different
things it didn't help much and made the tests less readable.

[Matt Tytel, 2012-03-20 23:02:30]

I meant the above *test, not the above extension.

[asargent_no_longer_on_chrome, 2012-03-21 18:47:31]

Getting close - a few fixes needed for tests and one or two nits.

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
File chrome/browser/extensions/extension_management_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
chrome/browser/extensions/extension_management_browsertest.cc:391:
ASSERT_EQ(enabled_size_before, service->extensions()->size());
shouldn't this be "enabled_size_before - 1", since you just called
DisableExtension to disable one extension?

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
chrome/browser/extensions/extension_management_browsertest.cc:403:
ASSERT_EQ(enabled_size_before, service->extensions()->size());
same question here

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
chrome/browser/extensions/extension_management_browsertest.cc:411:
ASSERT_FALSE(listener2.was_satisfied());
If there were a bug where the extension did accidentally get re-enabled, this
check is probably a race condition because the message from the extension's
background page might or might not have been sent and processed during the spin
of the message loop when you block inside WaitForExtensionInstall() above. 

Instead, how about you just add a direct assertion that the extension is
disabled right where you assert it has the new version? E.g.

ASSERT_EQ(NULL, service->GetExtensionById(
    "ogjcoiohnmldgjemafoockdghcjciccf", false));

or maybe

ASSERT_TRUE(ContainsKey(service->disabled_extensions(),
                        extension));
just after line 406.

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
File chrome/browser/extensions/updater/extension_updater.h (right):

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
chrome/browser/extensions/updater/extension_updater.h:114: // update URL to the
downloader, ignoring |pending_ids| so the extension isn't
nit: "...and have an update URL.." - actually, we just add records for all
extensions regardless of whether they have an update url. You can just remove
that part from the comment.

(In ExtensionDownloader::AddExtensionData if the update url is empty, we just
fall back to using the default one for the webstore. The reason is that if you
release an extension with no update url but later want to migrate to using the
webstore, this provides an escape hatch to avoid stranding users on that old
version which might have security bugs, etc.)

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
chrome/browser/extensions/updater/extension_updater_unittest.cc:1391: const
std::string& updateable_id = disabled_extensions[1]->id();
I think the test would be better if you removed the extension with
Extension::INVALID as a location (in real scenarios no extension should ever
actually have this value for its location), and had both enabled and disabled
extensions. That is, you should also create a set of enabled extensions which
you register by calling service.set_extensions() below. And then adjust your
expectations so that all of the enabled and disabled extensions got update
checks.

[Matt Tytel, 2012-03-26 05:23:59]

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
File chrome/browser/extensions/extension_management_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
chrome/browser/extensions/extension_management_browsertest.cc:391:
ASSERT_EQ(enabled_size_before, service->extensions()->size());
I create it, then disable it. So the number of enabled extensions should stay
the same.

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
chrome/browser/extensions/extension_management_browsertest.cc:411:
ASSERT_FALSE(listener2.was_satisfied());
Yea I was debating putting this line in for that reason, but it won't fail if it
works and it's a check that's not dependent on service->disabled_extensions().
I slightly prefer it there, but I'll remove it if you don't.

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
File chrome/browser/extensions/updater/extension_updater.h (right):

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
chrome/browser/extensions/updater/extension_updater.h:114: // update URL to the
downloader, ignoring |pending_ids| so the extension isn't
On 2012/03/21 18:47:31, Antony Sargent wrote:
> nit: "...and have an update URL.." - actually, we just add records for all
> extensions regardless of whether they have an update url. You can just remove
> that part from the comment.
> 
> (In ExtensionDownloader::AddExtensionData if the update url is empty, we just
> fall back to using the default one for the webstore. The reason is that if you
> release an extension with no update url but later want to migrate to using the
> webstore, this provides an escape hatch to avoid stranding users on that old
> version which might have security bugs, etc.)
> 
> 

Done.

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
File chrome/browser/extensions/updater/extension_updater_unittest.cc (right):

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
chrome/browser/extensions/updater/extension_updater_unittest.cc:1391: const
std::string& updateable_id = disabled_extensions[1]->id();
On 2012/03/21 18:47:31, Antony Sargent wrote:
> I think the test would be better if you removed the extension with
> Extension::INVALID as a location (in real scenarios no extension should ever
> actually have this value for its location), and had both enabled and disabled
> extensions. That is, you should also create a set of enabled extensions which
> you register by calling service.set_extensions() below. And then adjust your
> expectations so that all of the enabled and disabled extensions got update
> checks. 

Done.

[asargent_no_longer_on_chrome, 2012-03-27 00:35:51]

LGTM!

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
File chrome/browser/extensions/extension_management_browsertest.cc (right):

https://chromiumcodereview.appspot.com/9718028/diff/7001/chrome/browser/exten...
chrome/browser/extensions/extension_management_browsertest.cc:411:
ASSERT_FALSE(listener2.was_satisfied());
On 2012/03/26 05:23:59, Matt Tytel wrote:
> Yea I was debating putting this line in for that reason, but it won't fail if
it
> works and it's a check that's not dependent on service->disabled_extensions().
> I slightly prefer it there, but I'll remove it if you don't.

Seems fine to leave it in since you added the check using GetExtensionById above
- I just didn't think it should be the only line of defense.

[commit-bot: I haz the power, 2012-03-27 00:39:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/9718028/21003

[commit-bot: I haz the power, 2012-03-27 01:02:43]

Try job failure for 9718028-21003 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-03-27 01:28:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/9718028/21005

[commit-bot: I haz the power, 2012-03-27 03:43:19]

Try job failure for 9718028-21005 (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...

[commit-bot: I haz the power, 2012-03-28 01:20:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mtytel@chromium.org/9718028/29001

[commit-bot: I haz the power, 2012-03-28 04:06:14]

Change committed as 129365