Extensions: Store disable reasons in Sync

Extensions: Store disable reasons in Sync

This will allow us (in a follow-up CL) to *not* grant permissions to extensions that come in via Sync disabled due to a permission increase.

BUG=484214
Committed: https://crrev.com/c11923254ee7e930b2cd3431d94a6937982906f9
Cr-Commit-Position: refs/heads/master@{#330727}

[Marc Treib, 2015-05-11 17:51:31]

treib@chromium.org changed reviewers:
+ kalman@chromium.org

[Marc Treib, 2015-05-11 17:51:32]

PTAL! Lots of touched files, but interesting changes really only in
extension_service.cc (+ _unittest) and extension_sync_service.cc.
Thanks!

[not at google - send to devlin, 2015-05-12 18:04:17]

Sorry took so long, didn't get to it yesterday. I think basically lg but needs
that extra test.

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:543: // If the extension came in
disabled due to a permission increase, then
Is it reasonable to defer this change until after this patch has gone in? Less
to think about that way: first patch to migrate to the new model, second patch
to fix the bugs that it enables fixing?

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:884:
extension_prefs_->AddDisableReason(extension_id, disable_reasons);
If AddDisableReason() does support multiple reasons, it would be nice to rename
it AddDisableReasons() (and take an int). Or, leave AddDisableReason() which
takes an enum, and add a second function AddDisableReasons() which takes an int
bitmask.

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.h:285: // Note that
|disable_reasons| may be a bitmask of multiple reasons.
Could you change the argument to be an int? Bitmasks of enums don't make sense
as that type's enum, and can lead to funky state behaviour if they're
accidentally used inside a switch statement.

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_sync_service.cc:531: disable_reasons =
Extension::DISABLE_UNKNOWN_FROM_SYNC;
This is important to test, and rather than lumping it in the single new test
case you've added, try to test in isolation.

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_extension_helper.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_extension_helper.cc:213:
extension_state_map[id].enabled_state =
Consider saving a reference to extension_state_map[id] so that you don't need to
write it every time.

https://codereview.chromium.org/1136543003/diff/80001/sync/protocol/extension...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/1136543003/diff/80001/sync/protocol/extension...
sync/protocol/extension_specifics.proto:53: // Extension::DisableReason). Only
relevant when enabled == false.
You might also want to add that older clients won't set this, even if enabled is
false. I see that you've added logic that deals with that already, but I don't
see it tested.

https://codereview.chromium.org/1136543003/diff/80001/sync/protocol/proto_val...
File sync/protocol/proto_value_conversions.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/sync/protocol/proto_val...
sync/protocol/proto_value_conversions.cc:463: SET_STR(name);
Kinda weird that the name is initialised out of order with the other items.

[Marc Treib, 2015-05-19 12:12:03]

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:543: // If the extension came in
disabled due to a permission increase, then
On 2015/05/12 18:04:17, kalman wrote:
> Is it reasonable to defer this change until after this patch has gone in? Less
> to think about that way: first patch to migrate to the new model, second patch
> to fix the bugs that it enables fixing?

Yup, that makes sense. Reverted this change.

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.cc:884:
extension_prefs_->AddDisableReason(extension_id, disable_reasons);
On 2015/05/12 18:04:17, kalman wrote:
> If AddDisableReason() does support multiple reasons, it would be nice to
rename
> it AddDisableReasons() (and take an int). Or, leave AddDisableReason() which
> takes an enum, and add a second function AddDisableReasons() which takes an
int
> bitmask.

I've added ExtensionPrefs::AddDisableReasons(int). I think there's a few other
places where Extensions::DisableReason is used as a bitmask, though. I'll clean
them up if I come across any...

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.h:285: // Note that
|disable_reasons| may be a bitmask of multiple reasons.
On 2015/05/12 18:04:17, kalman wrote:
> Could you change the argument to be an int? Bitmasks of enums don't make sense
> as that type's enum, and can lead to funky state behaviour if they're
> accidentally used inside a switch statement.

Done.

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_sync_service.cc:531: disable_reasons =
Extension::DISABLE_UNKNOWN_FROM_SYNC;
On 2015/05/12 18:04:17, kalman wrote:
> This is important to test, and rather than lumping it in the single new test
> case you've added, try to test in isolation.

"This" being the legacy case, where no disable reasons come in through Sync?
I've added a test for that. If that's not what you meant, please clarify :)

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_extension_helper.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_extension_helper.cc:213:
extension_state_map[id].enabled_state =
On 2015/05/12 18:04:17, kalman wrote:
> Consider saving a reference to extension_state_map[id] so that you don't need
to
> write it every time.

Done. (Should it be a pointer instead of a reference? I was under the impression
that non-const references aren't allowed, but now can't find a rule in the style
guide that would forbid them.)

https://codereview.chromium.org/1136543003/diff/80001/sync/protocol/extension...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/1136543003/diff/80001/sync/protocol/extension...
sync/protocol/extension_specifics.proto:53: // Extension::DisableReason). Only
relevant when enabled == false.
On 2015/05/12 18:04:17, kalman wrote:
> You might also want to add that older clients won't set this, even if enabled
is
> false. I see that you've added logic that deals with that already, but I don't
> see it tested.

Done.

https://codereview.chromium.org/1136543003/diff/80001/sync/protocol/proto_val...
File sync/protocol/proto_value_conversions.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/sync/protocol/proto_val...
sync/protocol/proto_value_conversions.cc:463: SET_STR(name);
On 2015/05/12 18:04:17, kalman wrote:
> Kinda weird that the name is initialised out of order with the other items.

Indeed. I've moved it inline to correspond to the order in the .proto.

[not at google - send to devlin, 2015-05-19 14:32:11]

lgtm

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.h:285: // Note that
|disable_reasons| may be a bitmask of multiple reasons.
On 2015/05/19 12:12:03, Marc Treib wrote:
> On 2015/05/12 18:04:17, kalman wrote:
> > Could you change the argument to be an int? Bitmasks of enums don't make
sense
> > as that type's enum, and can lead to funky state behaviour if they're
> > accidentally used inside a switch statement.
> 
> Done.

Thanks. One last request: add a quick note how multiple disables makes sense /
when it might occur. It seems odd at first glance.

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_sync_service.cc:524: //
DISABLE_REMOTE_INSTALL should be there already.
It might be because I just woke up, but why is this the case? Might there not
already be synced data that sets remote install but didn't set the disable bit?
Or have we really never pushed a sync node that has remove install but not
disable reasons?

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_sync_service.cc:530: // Legacy case, from
before we synced disable reasons.
You might want to link to the bug# or mention the milestone or something - i.e.
define "legacy" in a concrete way - so that somebody reading the code doesn't
need to look too much into it. It will probably be legacy that we can never get
rid of.

// Legacy pre-M45 case, from before we synced disable reasons (see
crbug.com/...).

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_sync_service.cc:531: disable_reasons =
Extension::DISABLE_UNKNOWN_FROM_SYNC;
On 2015/05/19 12:12:03, Marc Treib wrote:
> On 2015/05/12 18:04:17, kalman wrote:
> > This is important to test, and rather than lumping it in the single new test
> > case you've added, try to test in isolation.
> 
> "This" being the legacy case, where no disable reasons come in through Sync?
> I've added a test for that. If that's not what you meant, please clarify :)

yeah that's what I mean, thanks.

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_extension_helper.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_extension_helper.cc:213:
extension_state_map[id].enabled_state =
On 2015/05/19 12:12:03, Marc Treib wrote:
> On 2015/05/12 18:04:17, kalman wrote:
> > Consider saving a reference to extension_state_map[id] so that you don't
need
> to
> > write it every time.
> 
> Done. (Should it be a pointer instead of a reference? I was under the
impression
> that non-const references aren't allowed, but now can't find a rule in the
style
> guide that would forbid them.)

I use non-const references for references into STL collections to avoid copying
/ convenient mutability, but that's about all. Some builder patterns use it too,
like DictionaryBuilder:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

Anything that leaves the immediate scope I use const reference or a pointer.
*This* is in the style guide:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar....

https://codereview.chromium.org/1136543003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/1136543003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_unittest.cc:6586: // Legacy case: No
disable reasons are set.
"The disable reason should be set to "DISABLE_UNKNOWN_FROM_SYNC".

https://codereview.chromium.org/1136543003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_unittest.cc:6601:
prefs->GetDisableReasons(good_crx));
Why is this the only place that GetDisableReasons is checked? I.e. could there
be a similar line up at ~6578 which has
EXPECT_EQ(Extension::DISABLE_USER_ACTION, prefs->GetDisableReasons(good_crx))?

https://codereview.chromium.org/1136543003/diff/100001/sync/protocol/extensio...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/1136543003/diff/100001/sync/protocol/extensio...
sync/protocol/extension_specifics.proto:54: // old clients (<M45) won't set
this, even when enabled is false.
Nice. Got to this file at the end. I guess my comment now would be "see places
where you wrote "legacy" and amend with "<M45".

[Marc Treib, 2015-05-19 15:30:56]

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service.h (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_service.h:285: // Note that
|disable_reasons| may be a bitmask of multiple reasons.
On 2015/05/19 14:32:10, kalman wrote:
> On 2015/05/19 12:12:03, Marc Treib wrote:
> > On 2015/05/12 18:04:17, kalman wrote:
> > > Could you change the argument to be an int? Bitmasks of enums don't make
> sense
> > > as that type's enum, and can lead to funky state behaviour if they're
> > > accidentally used inside a switch statement.
> > 
> > Done.
> 
> Thanks. One last request: add a quick note how multiple disables makes sense /
> when it might occur. It seems odd at first glance.

Done.

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/extension_sync_service.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_sync_service.cc:524: //
DISABLE_REMOTE_INSTALL should be there already.
On 2015/05/19 14:32:10, kalman wrote:
> It might be because I just woke up, but why is this the case? Might there not
> already be synced data that sets remote install but didn't set the disable
bit?
> Or have we really never pushed a sync node that has remove install but not
> disable reasons?

What I'm checking is: If there are any disable reasons at all, then
REMOTE_INSTALL must be among them. Note the first line of the comment, which
tries (not very successfully) to explain that :)
I've tried to make the comment a bit clearer.

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_sync_service.cc:530: // Legacy case, from
before we synced disable reasons.
On 2015/05/19 14:32:10, kalman wrote:
> You might want to link to the bug# or mention the milestone or something -
i.e.
> define "legacy" in a concrete way - so that somebody reading the code doesn't
> need to look too much into it. It will probably be legacy that we can never
get
> rid of.
> 
> // Legacy pre-M45 case, from before we synced disable reasons (see
> crbug.com/...).

Done.

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/sync_extension_helper.cc (right):

https://codereview.chromium.org/1136543003/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/sync_extension_helper.cc:213:
extension_state_map[id].enabled_state =
On 2015/05/19 14:32:10, kalman wrote:
> On 2015/05/19 12:12:03, Marc Treib wrote:
> > On 2015/05/12 18:04:17, kalman wrote:
> > > Consider saving a reference to extension_state_map[id] so that you don't
> need
> > to
> > > write it every time.
> > 
> > Done. (Should it be a pointer instead of a reference? I was under the
> impression
> > that non-const references aren't allowed, but now can't find a rule in the
> style
> > guide that would forbid them.)
> 
> I use non-const references for references into STL collections to avoid
copying
> / convenient mutability, but that's about all. Some builder patterns use it
too,
> like DictionaryBuilder:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> 
> Anything that leaves the immediate scope I use const reference or a pointer.
> *This* is in the style guide:
>
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar....

Yup, I only found that as well.
Alright, thanks for the explanation! This use of non-const references should be
okay then :) (though I've seen reviewers suggest pointers in similar cases...)

https://codereview.chromium.org/1136543003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/1136543003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_unittest.cc:6586: // Legacy case: No
disable reasons are set.
On 2015/05/19 14:32:10, kalman wrote:
> "The disable reason should be set to "DISABLE_UNKNOWN_FROM_SYNC".

Done.

https://codereview.chromium.org/1136543003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_unittest.cc:6601:
prefs->GetDisableReasons(good_crx));
On 2015/05/19 14:32:10, kalman wrote:
> Why is this the only place that GetDisableReasons is checked? I.e. could there
> be a similar line up at ~6578 which has
> EXPECT_EQ(Extension::DISABLE_USER_ACTION, prefs->GetDisableReasons(good_crx))?

Good point; added.

https://codereview.chromium.org/1136543003/diff/100001/sync/protocol/extensio...
File sync/protocol/extension_specifics.proto (right):

https://codereview.chromium.org/1136543003/diff/100001/sync/protocol/extensio...
sync/protocol/extension_specifics.proto:54: // old clients (<M45) won't set
this, even when enabled is false.
On 2015/05/19 14:32:10, kalman wrote:
> Nice. Got to this file at the end. I guess my comment now would be "see places
> where you wrote "legacy" and amend with "<M45".

Done.

[Marc Treib, 2015-05-19 15:32:46]

treib@chromium.org changed reviewers:
+ pavely@chromium.org

[Marc Treib, 2015-05-19 15:32:47]

+pavely for changes in c/b/sync/ and sync/. PTAL!

[pavely, 2015-05-19 17:32:57]

lgtm % small change.

https://codereview.chromium.org/1136543003/diff/120001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_extension_helper.cc (right):

https://codereview.chromium.org/1136543003/diff/120001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_extension_helper.cc:234:
std::list<std::string>::const_iterator id;
Please remove ...::const_iterator id;

[Marc Treib, 2015-05-20 09:06:38]

https://codereview.chromium.org/1136543003/diff/120001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_extension_helper.cc (right):

https://codereview.chromium.org/1136543003/diff/120001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_extension_helper.cc:234:
std::list<std::string>::const_iterator id;
On 2015/05/19 17:32:57, pavely wrote:
> Please remove ...::const_iterator id;

Done.

[Marc Treib, 2015-05-20 09:06:44]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-05-20 09:06:44]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
pavely@chromium.org
Link to the patchset: https://codereview.chromium.org/1136543003/#ps140001
(title: "pavely review")

[commit-bot: I haz the power, 2015-05-20 09:07:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136543003/140001

[commit-bot: I haz the power, 2015-05-20 10:48:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-05-20 10:48:15]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Marc Treib, 2015-05-20 12:05:40]

https://codereview.chromium.org/1136543003/diff/160001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_unittest.cc (right):

https://codereview.chromium.org/1136543003/diff/160001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_unittest.cc:6561:
prefs->GetGrantedPermissions(good_crx));
Ben, do you know a reason why ExtensionPrefs::GetGrantedPermissions and similar
methods return a raw pointer instead of a scoped_refptr in the first place? If
there isn't one, I'll make a followup CL to change that.

[Marc Treib, 2015-05-20 12:06:23]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-05-20 12:06:23]

The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org,
kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1136543003/#ps160001
(title: "fix memleak in test")

[commit-bot: I haz the power, 2015-05-20 12:06:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1136543003/160001

[commit-bot: I haz the power, 2015-05-20 12:56:15]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-05-20 12:56:59]

Patchset 9 (id:??) landed as
https://crrev.com/c11923254ee7e930b2cd3431d94a6937982906f9
Cr-Commit-Position: refs/heads/master@{#330727}