Made it possible to tell whether an extension is being installed or updated.

Made it possible to tell whether an extension is being installed or updated.

The NOTIFICATION_EXTENSION_INSTALLED message details now include a Boolean flag
already_installed, which, if true, implies that the extension is being updated
rather than installed.

BUG=234041
TBR=kinuko@chromium.org

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

[Matt Giuca, 2013-04-30 06:40:07]

Hi Ben,

This is not quite ready yet (tests), but I just want your opinion on whether
it's the correct approach. To follow the changes I've made, read in this order:

chrome/common/chrome_notification_types.h
chrome/browser/extensions/extension_service.cc
apps/shortcut_manager.cc

Basically just adding a new message UPDATED (to go along with INSTALLED), and
then using that instead of INSTALLED when an extension is updated.

The rest of the changes are just updating all of the rest of the codebase that
listens for INSTALLED to also listen for UPDATED. Therefore, all of that code
should continue to work as-is, except for ShortcutManager which explicitly
distinguishes between the two cases.

[Matt Giuca, 2013-05-01 09:32:58]

Completely rewritten and tests are in. PTAL.

Note that this CL no longer has any user-visible change to Chrome. It is in
preparation for automatically creating app shortcuts upon install (but not
update) in Windows and Linux. For that, we first need the ability to tell
whether an extension is being installed or updated.

The main change is in chrome/browser/extensions/extension_service.cc (documented
in chrome/common/chrome_notification_types.h), and then I had to update all of
the other observers to use the new struct type. Note that this is extremely
dangerous, since all of the observers are unsafely downcasting, so if I missed
any, it will be memory corruption time. I have carefully checked all references
to NOTIFICATION_EXTENSION_INSTALLED; I will check again tomorrow.

Ben: For OWNERS on all of these:
  apps/shortcut_manager.cc
  chrome/browser/extensions/*
  chrome/common/extensions/*

Scott: Just for OWNERS on these files:
  chrome/browser/chromeos/enterprise_extension_observer.cc
  chrome/browser/chromeos/screensaver/screensaver_controller.cc
  chrome/browser/managed_mode/managed_user_service.cc
  chrome/browser/performance_monitor/performance_monitor.cc
  chrome/browser/policy/policy_browsertest.cc
  chrome/browser/sync_file_system/sync_file_system_service.cc
  chrome/common/chrome_notification_types.h

Which are all (except the last) just minor updates to random observers to deal
with the change.

Sam: If interested.

[sky, 2013-05-01 16:10:57]

On Wed, May 1, 2013 at 2:32 AM,  <mgiuca@chromium.org> wrote:
> Completely rewritten and tests are in. PTAL.
>
> Note that this CL no longer has any user-visible change to Chrome. It is in
> preparation for automatically creating app shortcuts upon install (but not
> update) in Windows and Linux. For that, we first need the ability to tell
> whether an extension is being installed or updated.
>
> The main change is in chrome/browser/extensions/extension_service.cc
> (documented
> in chrome/common/chrome_notification_types.h), and then I had to update all
> of
> the other observers to use the new struct type. Note that this is extremely
> dangerous, since all of the observers are unsafely downcasting, so if I
> missed
> any, it will be memory corruption time. I have carefully checked all
> references
> to NOTIFICATION_EXTENSION_INSTALLED; I will check again tomorrow.
>
> Ben: For OWNERS on all of these:
>   apps/shortcut_manager.cc
>   chrome/browser/extensions/*
>   chrome/common/extensions/*
>
> Scott: Just for OWNERS on these files:
>   chrome/browser/chromeos/enterprise_extension_observer.cc
>   chrome/browser/chromeos/screensaver/screensaver_controller.cc
>   chrome/browser/managed_mode/managed_user_service.cc
>   chrome/browser/performance_monitor/performance_monitor.cc
>   chrome/browser/policy/policy_browsertest.cc
>   chrome/browser/sync_file_system/sync_file_system_service.cc

I'm not a good OWNER for any of these files. Is there a better local
OWNER you can find?

  -Scott

>   chrome/common/chrome_notification_types.h
>
> Which are all (except the last) just minor updates to random observers to
> deal
> with the change.
>
> Sam: If interested.
>
> https://codereview.chromium.org/14238037/

[sky, 2013-05-01 16:11:18]

chrome/common/chrome_notification_types.h LGTM

[Matt Giuca, 2013-05-01 23:47:04]

On 2013/05/01 16:10:57, sky wrote:

> I'm not a good OWNER for any of these files. Is there a better local
> OWNER you can find?

I would have to track down 5 separate people since they are all in disparate
directories with different owners. Really for those six files, I am just asking
for a rubber stamp since all I am doing is updating them to use the correct type
as a result of my change to chrome/browser/extensions/extension_service.cc which
Ben will review properly.

[sky, 2013-05-02 00:44:34]

I recommend TBR in that situation.

On Wed, May 1, 2013 at 4:47 PM,  <mgiuca@chromium.org> wrote:
> On 2013/05/01 16:10:57, sky wrote:
>
>> I'm not a good OWNER for any of these files. Is there a better local
>> OWNER you can find?
>
>
> I would have to track down 5 separate people since they are all in disparate
> directories with different owners. Really for those six files, I am just
> asking
> for a rubber stamp since all I am doing is updating them to use the correct
> type
> as a result of my change to chrome/browser/extensions/extension_service.cc
> which
> Ben will review properly.
>
> https://codereview.chromium.org/14238037/

[Matt Giuca, 2013-05-02 01:07:02]

On 2013/05/02 00:44:34, sky wrote:
> I recommend TBR in that situation.

OK, added five TBRs:

satorux: chrome/browser/chromeos/*
bauerb: chrome/browser/managed_mode/managed_user_service.cc
rdevlin.cronin: chrome/browser/performance_monitor/performance_monitor.cc
mnissler: chrome/browser/policy/policy_browsertest.cc
kinuko: chrome/browser/sync_file_system/sync_file_system_service.cc

If you are on this list, it's because you were the OWNER that made the closest
changes to the section of your file I had to modify. This is a fairly mechanical
change, but you might want to check the new semantics.

[satorux1, 2013-05-02 01:15:37]

c/b/chromeos lgtm

[benwells, 2013-05-02 01:40:08]

all lgtm, with the name change in the new type.

https://codereview.chromium.org/14238037/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/14238037/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2426: bool already_installed =
GetInstalledExtension(extension->id()) != NULL;
nit: change this to is_update, i think that is clearer.

https://codereview.chromium.org/14238037/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/14238037/diff/44001/chrome/common/extensions/...
chrome/common/extensions/extension.h:706: bool already_installed;
I think this is more clear if it is just called is_update.

[Matt Giuca, 2013-05-02 05:40:33]

https://codereview.chromium.org/14238037/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/14238037/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/extension_service.cc:2426: bool already_installed =
GetInstalledExtension(extension->id()) != NULL;
On 2013/05/02 01:40:08, benwells wrote:
> nit: change this to is_update, i think that is clearer.

Done.

https://codereview.chromium.org/14238037/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/14238037/diff/44001/chrome/common/extensions/...
chrome/common/extensions/extension.h:706: bool already_installed;
On 2013/05/02 01:40:08, benwells wrote:
> I think this is more clear if it is just called is_update.

Done.

[Devlin, 2013-05-02 06:20:11]

c/b/performance_monitor lgtm, plus one small nit.

Cheers :)

https://codereview.chromium.org/14238037/diff/76001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/14238037/diff/76001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:1747: : extension(extension),
small nit: this and the following lines should have indent of four, i.e.

...
    bool is_update)
    : extension(extension),
...

Please also change the indentation on lines 1753 - 1755.

[Mattias Nissler (ping if slow), 2013-05-02 07:05:20]

chrome/browser/policy LGTM

[Bernhard Bauer, 2013-05-02 07:06:49]

chrome/browser/managed_mode LGTM.

[Matt Giuca, 2013-05-02 07:17:38]

Thanks for the speedy reviews, everybody.

https://codereview.chromium.org/14238037/diff/76001/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/14238037/diff/76001/chrome/common/extensions/...
chrome/common/extensions/extension.cc:1747: : extension(extension),
On 2013/05/02 06:20:11, D Cronin wrote:
> small nit: this and the following lines should have indent of four, i.e.
> 
> ...
>     bool is_update)
>     : extension(extension),
> ...
> 
> Please also change the indentation on lines 1753 - 1755.

Done.

[Matt Giuca, 2013-05-02 09:51:43]

mnissler: Sorry to pull you back but I had to make a major change to
policy_browsertest to stop it from crashing under this new scheme. Can you
please take another look? Explanation is in the comments.

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:232: // invalid after the call to
Observe().
This class is pretty complicated and duplicates a lot of code from
content::WindowedNotificationObserver, but it seems as though it will be
impossible to make this not crash using WindowedNotificationObserver.

The problem is that:
a) ExtensionService::FinishInstallation allocates memory for an
InstalledExtensionInfo, passes it to Notify, then frees the memory immediately.
The observer is not expected to hold on to the pointer. (In preparing this
approach, I looked at other Notify calls and they follow this same pattern.)
b) WindowedNotificationObserver::Observe stores the NotificationDetails object
(in this case, a pointer to an InstalledExtensionInfo) in a field, and then
returns. Therefore, it is storing a dangling pointer.

There is no way to make WindowedNotificationObserver work correctly for this
case and still be fully general. And the NotificationDetails has to be a plain
pointer (not a shared pointer or something similar). So the only way I could
think of of fixing this was to copy the WindowedNotificationObserver code, and
then change it so that its Observe method reads out the fields of the
InstalledExtensionInfo into its fields rather than storing the pointer.

If you have any other suggestions that would be simpler, I'm all ears.

[Mattias Nissler (ping if slow), 2013-05-02 11:00:33]

Joao, what do you think?

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:232: // invalid after the call to
Observe().
On 2013/05/02 09:51:44, Matt Giuca wrote:
> This class is pretty complicated and duplicates a lot of code from
> content::WindowedNotificationObserver, but it seems as though it will be
> impossible to make this not crash using WindowedNotificationObserver.
> 
> The problem is that:
> a) ExtensionService::FinishInstallation allocates memory for an
> InstalledExtensionInfo, passes it to Notify, then frees the memory
immediately.
> The observer is not expected to hold on to the pointer. (In preparing this
> approach, I looked at other Notify calls and they follow this same pattern.)
> b) WindowedNotificationObserver::Observe stores the NotificationDetails object
> (in this case, a pointer to an InstalledExtensionInfo) in a field, and then
> returns. Therefore, it is storing a dangling pointer.
> 
> There is no way to make WindowedNotificationObserver work correctly for this
> case and still be fully general. And the NotificationDetails has to be a plain
> pointer (not a shared pointer or something similar). So the only way I could
> think of of fixing this was to copy the WindowedNotificationObserver code, and
> then change it so that its Observe method reads out the fields of the
> InstalledExtensionInfo into its fields rather than storing the pointer.
> 
> If you have any other suggestions that would be simpler, I'm all ears.

Given that we're mostly only using the notification to achieve synchronization,
I guess it'd be fine to just continue using WindowedNotificationObserver and
just check via the ExtensionService that our extension has been installed. So
we'd no longer check that the notification contains the correct extension ID,
but that's not really of concern in these tests anyhow.

Adding Joao as well to see what he thinks.

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:234: public:
indentation

[Joao da Silva, 2013-05-02 11:36:29]

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:232: // invalid after the call to
Observe().
On 2013/05/02 09:51:44, Matt Giuca wrote:
> This class is pretty complicated and duplicates a lot of code from
> content::WindowedNotificationObserver, but it seems as though it will be
> impossible to make this not crash using WindowedNotificationObserver.
> 
> The problem is that:
> a) ExtensionService::FinishInstallation allocates memory for an
> InstalledExtensionInfo, passes it to Notify, then frees the memory
immediately.
> The observer is not expected to hold on to the pointer. (In preparing this
> approach, I looked at other Notify calls and they follow this same pattern.)
> b) WindowedNotificationObserver::Observe stores the NotificationDetails object
> (in this case, a pointer to an InstalledExtensionInfo) in a field, and then
> returns. Therefore, it is storing a dangling pointer.
> 
> There is no way to make WindowedNotificationObserver work correctly for this
> case and still be fully general. And the NotificationDetails has to be a plain
> pointer (not a shared pointer or something similar). So the only way I could
> think of of fixing this was to copy the WindowedNotificationObserver code, and
> then change it so that its Observe method reads out the fields of the
> InstalledExtensionInfo into its fields rather than storing the pointer.
> 
> If you have any other suggestions that would be simpler, I'm all ears.

I'd guess that other notifications have the same issue so this behavior is
surprising.

As Mattias mentioned, these notifications are used mostly for synchronization in
these tests and checking that the extension is installed at the ExtensionService
achieves the same.

I don't have a strong preference here. I suggest subclassing
WindowedNotificationObserver and just overriding Observe() to have less code
here, and then just extract the Extension* from the details:

  virtual void Observer(args...) OVERRIDE {
    WindowedNotificationObserver::Observe(args...);
    extension_ = details()->extension;
  }

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:287: const extensions::Extension*
extension_;
Doesn't this pointer have the same lifetime issue? Shouldn't this be a
scoped_refptr?

[Joao da Silva, 2013-05-02 11:42:03]

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:232: // invalid after the call to
Observe().
On 2013/05/02 11:00:33, Mattias Nissler wrote:
> On 2013/05/02 09:51:44, Matt Giuca wrote:
> > This class is pretty complicated and duplicates a lot of code from
> > content::WindowedNotificationObserver, but it seems as though it will be
> > impossible to make this not crash using WindowedNotificationObserver.
> > 
> > The problem is that:
> > a) ExtensionService::FinishInstallation allocates memory for an
> > InstalledExtensionInfo, passes it to Notify, then frees the memory
> immediately.
> > The observer is not expected to hold on to the pointer. (In preparing this
> > approach, I looked at other Notify calls and they follow this same pattern.)
> > b) WindowedNotificationObserver::Observe stores the NotificationDetails
object
> > (in this case, a pointer to an InstalledExtensionInfo) in a field, and then
> > returns. Therefore, it is storing a dangling pointer.
> > 
> > There is no way to make WindowedNotificationObserver work correctly for this
> > case and still be fully general. And the NotificationDetails has to be a
plain
> > pointer (not a shared pointer or something similar). So the only way I could
> > think of of fixing this was to copy the WindowedNotificationObserver code,
and
> > then change it so that its Observe method reads out the fields of the
> > InstalledExtensionInfo into its fields rather than storing the pointer.
> > 
> > If you have any other suggestions that would be simpler, I'm all ears.
> 
> Given that we're mostly only using the notification to achieve
synchronization,
> I guess it'd be fine to just continue using WindowedNotificationObserver and
> just check via the ExtensionService that our extension has been installed. So
> we'd no longer check that the notification contains the correct extension ID,
> but that's not really of concern in these tests anyhow.
> 
> Adding Joao as well to see what he thinks.

Discussed a bit more offline with Mattias. We agreed that just waiting for the
notification and ignoring the details is good enough for these tests, and this
class can be avoided altogether.

[Matt Giuca, 2013-05-03 02:02:09]

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:232: // invalid after the call to
Observe().
OK thanks for discussing and coming to a consensus, guys. I have removed the
class and also removed the checks that were crashing (with a comment to explain
why we don't check).

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:234: public:
On 2013/05/02 11:00:33, Mattias Nissler wrote:
> indentation

Done. (Not relevant any more.)

https://codereview.chromium.org/14238037/diff/86001/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:287: const extensions::Extension*
extension_;
On 2013/05/02 11:36:29, Joao da Silva wrote:
> Doesn't this pointer have the same lifetime issue? Shouldn't this be a
> scoped_refptr?

No, because the extension itself is longer lived (it gets destroyed by the test
case, after the call to observer.Wait()). The lifetime issue is just that the
caller of Notify / Observe is going to immediately free the
InstalledExtensionInfo object.

It can't be a scoped_refptr without completely refactoring all uses of
InstalledExtensionInfo to contain the extension as a scoped_refptr.

Anyway, we seem to have consensus on deleting this class.

[Joao da Silva, 2013-05-03 06:38:45]

lgtm from my side after adding line in the comment. Thanks!

https://codereview.chromium.org/14238037/diff/91001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/14238037/diff/91001/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:1311: 
Check that the extension was installed here:
EXPECT_TRUE(service->GetExtensionById(kGoodCrxId, true));

[Matt Giuca, 2013-05-03 07:08:09]

Thanks, Joao.

[Matt Giuca, 2013-05-03 07:09:32]

https://codereview.chromium.org/14238037/diff/91001/chrome/browser/policy/pol...
File chrome/browser/policy/policy_browsertest.cc (right):

https://codereview.chromium.org/14238037/diff/91001/chrome/browser/policy/pol...
chrome/browser/policy/policy_browsertest.cc:1311: 
On 2013/05/03 06:38:45, Joao da Silva wrote:
> Check that the extension was installed here:
> EXPECT_TRUE(service->GetExtensionById(kGoodCrxId, true));

Done.

[commit-bot: I haz the power, 2013-05-03 07:09:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/14238037/107001

[commit-bot: I haz the power, 2013-05-03 07:34:36]

Retried try job too often on ios_dbg_simulator for step(s) googleurl_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[commit-bot: I haz the power, 2013-05-03 07:41:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/14238037/107001

[commit-bot: I haz the power, 2013-05-03 10:56:04]

Change committed as 198079