Make runtime.reload() work with component extensions.

Make runtime.reload() work with component extensions.

This eliminates the ENABLED_COMPONENT state, and fixes the path CHECK failure.

BUG=402377
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289616

[benwells, 2014-08-12 04:17:14]

https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:829: // Component extensions need
to be put into the "ENABLED_COMPONENT",
Um. Hmm. ENABLED_COMPONENT feels pretty strange. Do you think we could kill it
totally (i.e. just use ENABLED regardless of it being a component app or not)
and fix the crashing at its source?

[Anand Mistry (off Chromium), 2014-08-12 05:06:42]

https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:829: // Component extensions need
to be put into the "ENABLED_COMPONENT",
On 2014/08/12 04:17:14, benwells wrote:
> Um. Hmm. ENABLED_COMPONENT feels pretty strange. Do you think we could kill it
> totally (i.e. just use ENABLED regardless of it being a component app or not)
> and fix the crashing at its source?

Adding Trent since he introduced this in
https://src.chromium.org/viewvc/chrome?view=revision&revision=170137

[tapted, 2014-08-12 05:39:07]

https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:829: // Component extensions need
to be put into the "ENABLED_COMPONENT",
On 2014/08/12 05:06:42, Anand Mistry wrote:
> On 2014/08/12 04:17:14, benwells wrote:
> > Um. Hmm. ENABLED_COMPONENT feels pretty strange. Do you think we could kill
it
> > totally (i.e. just use ENABLED regardless of it being a component app or
not)
> > and fix the crashing at its source?
> 
> Adding Trent since he introduced this in
> https://src.chromium.org/viewvc/chrome?view=revision&revision=170137

Woww so long ago. That was one of my first chrome patches!

I think when I wrote that, the absence of "state" in the extension prefs was a
signal for that extension info sub-map in $profile/Preferences being for a
component extension, and so some extra validation on keys in that sub-map being
unnecessary/unwanted [since component extensions get exceptions to some manifest
requirements].

But the absence of "state" was achieved by skipping some install tasks that were
actually necessary.

Doing the missing install task required a 'state' to be written, but it had to
be a new state otherwise it would opt all currently installed component
extensions into the more stringent manifest requirement checks, and at the time
of reading the manifest info.


I can't remember if there was an explicit reason for leaning on `state` over,
say, refactoring GetInstalledExtensionInfo and relying instead on `location`,
and doing the same thing there. If that works, I agree with Ben, and we should
be able to ditch ENABLED_COMPONENT.


There might still be a risk.. at the time it was a royal mess because a
first_run would start up two Chrome processes both writing to
$profile/Preferences -- the other-browser-import process would partially load
component extensions and then write out garbage prefs for them that the "real"
browser process would read back and have to handle. That extra process is now
gone (IPC is used instead of writing to disk directly), but the prefs reading
still needs to be robust to handle any existing crap that may have been written
for component extensions :/. That is, it might be possible that mess was writing
prefs for component extensions without a valid `location`.

[Anand Mistry (off Chromium), 2014-08-12 07:30:23]

On 2014/08/12 05:39:07, tapted wrote:
>
https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/extension_service.cc:829: // Component extensions
need
> to be put into the "ENABLED_COMPONENT",
> On 2014/08/12 05:06:42, Anand Mistry wrote:
> > On 2014/08/12 04:17:14, benwells wrote:
> > > Um. Hmm. ENABLED_COMPONENT feels pretty strange. Do you think we could
kill
> it
> > > totally (i.e. just use ENABLED regardless of it being a component app or
> not)
> > > and fix the crashing at its source?
> > 
> > Adding Trent since he introduced this in
> > https://src.chromium.org/viewvc/chrome?view=revision&revision=170137
> 
> Woww so long ago. That was one of my first chrome patches!
> 
> I think when I wrote that, the absence of "state" in the extension prefs was a
> signal for that extension info sub-map in $profile/Preferences being for a
> component extension, and so some extra validation on keys in that sub-map
being
> unnecessary/unwanted [since component extensions get exceptions to some
manifest
> requirements].
> 
> But the absence of "state" was achieved by skipping some install tasks that
were
> actually necessary.
> 
> Doing the missing install task required a 'state' to be written, but it had to
> be a new state otherwise it would opt all currently installed component
> extensions into the more stringent manifest requirement checks, and at the
time
> of reading the manifest info.
> 
> 
> I can't remember if there was an explicit reason for leaning on `state` over,
> say, refactoring GetInstalledExtensionInfo and relying instead on `location`,
> and doing the same thing there. If that works, I agree with Ben, and we should
> be able to ditch ENABLED_COMPONENT.
> 
> 
> There might still be a risk.. at the time it was a royal mess because a
> first_run would start up two Chrome processes both writing to
> $profile/Preferences -- the other-browser-import process would partially load
> component extensions and then write out garbage prefs for them that the "real"
> browser process would read back and have to handle. That extra process is now
> gone (IPC is used instead of writing to disk directly), but the prefs reading
> still needs to be robust to handle any existing crap that may have been
written
> for component extensions :/. That is, it might be possible that mess was
writing
> prefs for component extensions without a valid `location`.

I've removed that state, and tried to make it function the same way as it did
before. Not sure if I got it right though.

[Anand Mistry (off Chromium), 2014-08-12 07:32:10]

arv@chromium.org: for voicesearch_ui.cc

https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:829: // Component extensions need
to be put into the "ENABLED_COMPONENT",
On 2014/08/12 05:39:06, tapted wrote:
> On 2014/08/12 05:06:42, Anand Mistry wrote:
> > On 2014/08/12 04:17:14, benwells wrote:
> > > Um. Hmm. ENABLED_COMPONENT feels pretty strange. Do you think we could
kill
> it
> > > totally (i.e. just use ENABLED regardless of it being a component app or
> not)
> > > and fix the crashing at its source?
> > 
> > Adding Trent since he introduced this in
> > https://src.chromium.org/viewvc/chrome?view=revision&revision=170137
> 
> Woww so long ago. That was one of my first chrome patches!
> 
> I think when I wrote that, the absence of "state" in the extension prefs was a
> signal for that extension info sub-map in $profile/Preferences being for a
> component extension, and so some extra validation on keys in that sub-map
being
> unnecessary/unwanted [since component extensions get exceptions to some
manifest
> requirements].
> 
> But the absence of "state" was achieved by skipping some install tasks that
were
> actually necessary.
> 
> Doing the missing install task required a 'state' to be written, but it had to
> be a new state otherwise it would opt all currently installed component
> extensions into the more stringent manifest requirement checks, and at the
time
> of reading the manifest info.
> 
> 
> I can't remember if there was an explicit reason for leaning on `state` over,
> say, refactoring GetInstalledExtensionInfo and relying instead on `location`,
> and doing the same thing there. If that works, I agree with Ben, and we should
> be able to ditch ENABLED_COMPONENT.
> 
> 
> There might still be a risk.. at the time it was a royal mess because a
> first_run would start up two Chrome processes both writing to
> $profile/Preferences -- the other-browser-import process would partially load
> component extensions and then write out garbage prefs for them that the "real"
> browser process would read back and have to handle. That extra process is now
> gone (IPC is used instead of writing to disk directly), but the prefs reading
> still needs to be robust to handle any existing crap that may have been
written
> for component extensions :/. That is, it might be possible that mess was
writing
> prefs for component extensions without a valid `location`.

Removed the ENABLED_COMPONENT state, but not sure if I got it right. I guess the
trybots will tell us.

[benwells, 2014-08-12 22:51:39]

On 2014/08/12 07:32:10, Anand Mistry wrote:
> mailto:arv@chromium.org: for voicesearch_ui.cc
> 
>
https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
> File chrome/browser/extensions/extension_service.cc (right):
> 
>
https://codereview.chromium.org/462533002/diff/20001/chrome/browser/extension...
> chrome/browser/extensions/extension_service.cc:829: // Component extensions
need
> to be put into the "ENABLED_COMPONENT",
> On 2014/08/12 05:39:06, tapted wrote:
> > On 2014/08/12 05:06:42, Anand Mistry wrote:
> > > On 2014/08/12 04:17:14, benwells wrote:
> > > > Um. Hmm. ENABLED_COMPONENT feels pretty strange. Do you think we could
> kill
> > it
> > > > totally (i.e. just use ENABLED regardless of it being a component app or
> > not)
> > > > and fix the crashing at its source?
> > > 
> > > Adding Trent since he introduced this in
> > > https://src.chromium.org/viewvc/chrome?view=revision&revision=170137
> > 
> > Woww so long ago. That was one of my first chrome patches!
> > 
> > I think when I wrote that, the absence of "state" in the extension prefs was
a
> > signal for that extension info sub-map in $profile/Preferences being for a
> > component extension, and so some extra validation on keys in that sub-map
> being
> > unnecessary/unwanted [since component extensions get exceptions to some
> manifest
> > requirements].
> > 
> > But the absence of "state" was achieved by skipping some install tasks that
> were
> > actually necessary.
> > 
> > Doing the missing install task required a 'state' to be written, but it had
to
> > be a new state otherwise it would opt all currently installed component
> > extensions into the more stringent manifest requirement checks, and at the
> time
> > of reading the manifest info.
> > 
> > 
> > I can't remember if there was an explicit reason for leaning on `state`
over,
> > say, refactoring GetInstalledExtensionInfo and relying instead on
`location`,
> > and doing the same thing there. If that works, I agree with Ben, and we
should
> > be able to ditch ENABLED_COMPONENT.
> > 
> > 
> > There might still be a risk.. at the time it was a royal mess because a
> > first_run would start up two Chrome processes both writing to
> > $profile/Preferences -- the other-browser-import process would partially
load
> > component extensions and then write out garbage prefs for them that the
"real"
> > browser process would read back and have to handle. That extra process is
now
> > gone (IPC is used instead of writing to disk directly), but the prefs
reading
> > still needs to be robust to handle any existing crap that may have been
> written
> > for component extensions :/. That is, it might be possible that mess was
> writing
> > prefs for component extensions without a valid `location`.
> 
> Removed the ENABLED_COMPONENT state, but not sure if I got it right. I guess
the
> trybots will tell us.

tapted - wanna take a look? trybots look green

[tapted, 2014-08-13 00:25:07]

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:1340: base::FilePath::StringType path;
nit: `path` isn't used until the very end of the function, so I'd move lines
1340-1349 down to the bottom

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:1348: if
(!base::FilePath(path).IsAbsolute()) {
nit: no curlies

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:1355: // TODO(amistry): Is this what we
want?
yep! But I think the sentiment we want is: Component extensions may have data
saved in preferences, but they are already loaded at this point (by
ComponentLoader) and shouldn't be populated into the result of
GetInstalledExtensionsInfo, otherwise InstalledLoader would also want to load
them. Exiting here also skips the path validation, which doesn't make sense for
component extensions since they are loaded from the resource bundle.

(.. maybe a shorter version of that)

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:1387: if (!ext->GetInteger(kPrefState,
&state_value)) {
I'd probably combine this with the statement below, and drop the comment about
component extensions. i.e.

if (ext->GetInteger(kPrefState, &state_value) && state_value ==
..EXTERNAL_EXTENSION_UNINSTALLED) {
  LOG(..);
  return;
}

i.e. rely just on the check in GetInstalledInfoHelper, which is more robust

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:2063: if
(extension->manifest()->location() != Manifest::COMPONENT)
This extra check can probably be removed (i.e. write the pref unconditionally).
_really_ old versions of Chrome will still crash if this is encountered, but for
~20 months they will have had that early exit case. And we don't usually cater
for old versions of chome this way - but when this first went in crash on
startup was guaranteed otherwise if you try to go e.g. Beta->Stable with the
same profile. Should be safer now.

https://codereview.chromium.org/462533002/diff/40001/extensions/common/extens...
File extensions/common/extension.h (left):

https://codereview.chromium.org/462533002/diff/40001/extensions/common/extens...
extensions/common/extension.h:67: ENABLED_COMPONENT,
It's annoying, but I think we need to keep a placeholder here, since preferences
will have this enum value stored to disk. If someone adds another (unlikely), it
needs to start at 4 not 3. Probably just ENABLED_COMPONENT_DEPRECATED -
hopefully we don't have to mention it anywhere else.

[chromium-reviews, 2014-08-13 00:36:50]

On 13 Aug 2014 10:25, <tapted@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
> extensions/browser/extension_prefs.cc:2063: if
> (extension->manifest()->location() != Manifest::COMPONENT)
> This extra check can probably be removed (i.e. write the pref
> unconditionally). _really_ old versions of Chrome will still crash if
> this is encountered, but for ~20 months they will have had that early
> exit case. And we don't usually cater for old versions of chome this way
> - but when this first went in crash on startup was guaranteed otherwise
> if you try to go e.g. Beta->Stable with the same profile. Should be
> safer now.

Whoops that's not true: p. You'd have to be writing INSTALLED_COMPONENT for
that early exit, but we're getting rid of that :p

>
>
https://codereview.chromium.org/462533002/diff/40001/extensions/common/extens...
> File extensions/common/extension.h (left):
>
>
https://codereview.chromium.org/462533002/diff/40001/extensions/common/extens...
> extensions/common/extension.h:67: ENABLED_COMPONENT,
> It's annoying, but I think we need to keep a placeholder here, since
> preferences will have this enum value stored to disk. If someone adds
> another (unlikely), it needs to start at 4 not 3. Probably just
> ENABLED_COMPONENT_DEPRECATED - hopefully we don't have to mention it
> anywhere else.
>
> https://codereview.chromium.org/462533002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Anand Mistry (off Chromium), 2014-08-13 01:53:15]

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:1340: base::FilePath::StringType path;
On 2014/08/13 00:25:07, tapted wrote:
> nit: `path` isn't used until the very end of the function, so I'd move lines
> 1340-1349 down to the bottom

Done.

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:1348: if
(!base::FilePath(path).IsAbsolute()) {
On 2014/08/13 00:25:07, tapted wrote:
> nit: no curlies

Yuk! I prefer braces.

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:1355: // TODO(amistry): Is this what we
want?
On 2014/08/13 00:25:07, tapted wrote:
> yep! But I think the sentiment we want is: Component extensions may have data
> saved in preferences, but they are already loaded at this point (by
> ComponentLoader) and shouldn't be populated into the result of
> GetInstalledExtensionsInfo, otherwise InstalledLoader would also want to load
> them. Exiting here also skips the path validation, which doesn't make sense
for
> component extensions since they are loaded from the resource bundle.
> 
> (.. maybe a shorter version of that)

Done.

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:1387: if (!ext->GetInteger(kPrefState,
&state_value)) {
On 2014/08/13 00:25:07, tapted wrote:
> I'd probably combine this with the statement below, and drop the comment about
> component extensions. i.e.
> 
> if (ext->GetInteger(kPrefState, &state_value) && state_value ==
> ..EXTERNAL_EXTENSION_UNINSTALLED) {
>   LOG(..);
>   return;
> }
> 
> i.e. rely just on the check in GetInstalledInfoHelper, which is more robust

Done.

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:2063: if
(extension->manifest()->location() != Manifest::COMPONENT)
On 2014/08/13 00:25:07, tapted wrote:
> This extra check can probably be removed (i.e. write the pref
unconditionally).
> _really_ old versions of Chrome will still crash if this is encountered, but
for
> ~20 months they will have had that early exit case. And we don't usually cater
> for old versions of chome this way - but when this first went in crash on
> startup was guaranteed otherwise if you try to go e.g. Beta->Stable with the
> same profile. Should be safer now.

Done.

https://codereview.chromium.org/462533002/diff/40001/extensions/common/extens...
File extensions/common/extension.h (left):

https://codereview.chromium.org/462533002/diff/40001/extensions/common/extens...
extensions/common/extension.h:67: ENABLED_COMPONENT,
On 2014/08/13 00:25:07, tapted wrote:
> It's annoying, but I think we need to keep a placeholder here, since
preferences
> will have this enum value stored to disk. If someone adds another (unlikely),
it
> needs to start at 4 not 3. Probably just ENABLED_COMPONENT_DEPRECATED -
> hopefully we don't have to mention it anywhere else.

Done.

[tapted, 2014-08-13 03:06:55]

lgtm

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/462533002/diff/40001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:2063: if
(extension->manifest()->location() != Manifest::COMPONENT)
On 2014/08/13 01:53:15, Anand Mistry wrote:
> On 2014/08/13 00:25:07, tapted wrote:
> > This extra check can probably be removed (i.e. write the pref
> unconditionally).
> > _really_ old versions of Chrome will still crash if this is encountered, but
> for
> > ~20 months they will have had that early exit case. And we don't usually
cater
> > for old versions of chome this way - but when this first went in crash on
> > startup was guaranteed otherwise if you try to go e.g. Beta->Stable with the
> > same profile. Should be safer now.
> 
> Done.

so yeah - looks like this can't tickle a crash out of stable, so should be fine.
cool.

https://codereview.chromium.org/462533002/diff/80001/extensions/browser/exten...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/462533002/diff/80001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:1369: // Make path absolute. Most (but not
all) extension types don't have absolute
optional nit: "don't have absolute paths" -> "have relative paths" is maybe
easier to read

[Anand Mistry (off Chromium), 2014-08-13 03:12:56]

https://codereview.chromium.org/462533002/diff/80001/extensions/browser/exten...
File extensions/browser/extension_prefs.cc (right):

https://codereview.chromium.org/462533002/diff/80001/extensions/browser/exten...
extensions/browser/extension_prefs.cc:1369: // Make path absolute. Most (but not
all) extension types don't have absolute
On 2014/08/13 03:06:55, tapted wrote:
> optional nit: "don't have absolute paths" -> "have relative paths" is maybe
> easier to read

Done.

[arv (Not doing code reviews), 2014-08-13 15:04:17]

LGTM

[benwells, 2014-08-13 23:29:51]

stamp lgtm

[Anand Mistry (off Chromium), 2014-08-13 23:30:14]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-08-13 23:33:41]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/amistry@chromium.org/462533002/100001

[commit-bot: I haz the power, 2014-08-14 16:53:46]

Committed patchset #6 (100001) as 289616