[Extensions] Fix lifetime bug in ExtensionAction/IconImage

[Extensions] Fix lifetime bug in ExtensionAction/IconImage

ExtensionActions can lazily load the default icon image, which is
implemented as an IconImage. The IconImage, in turn, is constructed with
a BrowserContext because it needs access to the ImageLoader (a
BrowserContextKeyedService). ExtensionActions are owned by the
ExtensionActionManager (another BrowserContextKeyedService with one
instance shared across incognito and normal profiles), but took a
BrowserContext as an argument when loading the default icon image.

This all caused a problem when an incognito profile was passed in as
the browser context to load the default icon for the extension action.
The extension action would then create an IconImage with the incognito
profile as a context, but the ExtensionAction (and thus the IconImage)
are owned by the ExtensionActionManager, and are therefore not deleted
upon the incognito profile's destruction. This means that if you have a
flow where the ExtensionAction's default icon is loaded via an incognito
profile, the incognito profile is deleted, and then the icon is later
used in the normal profile, there's a crash.

Fix this by having the ExtensionActionManager assign the IconImage to
the ExtensionAction using its own profile. Since the ExtensionActions
are owned by the ExtensionActionManager, and the ExtensionActionManager
is owned by the profile by being a BrowserContextKeyedService, this is
guaranteed to be safe. This also abstracts out the context of a profile
or BrowserContext from the ExtensionAction class, keeping it more in
line with its data-structure-like concept.

Add a regression test.

BUG=663726
(and possibly others)

Committed: https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320
Cr-Commit-Position: refs/heads/master@{#433361}

[Devlin, 2016-11-17 01:32:48]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 01:35:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-17 02:28:25]

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

[commit-bot: I haz the power, 2016-11-17 02:28:26]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2016-11-17 18:15:47]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 18:16:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-11-17 18:17:40]

Description was changed from

==========
Icon fix
BUG=
==========

to

==========
[Extensions] Fix lifetime bug in ExtensionAction/IconImage

ExtensionActions can lazily load the default icon image, which is
implemented as an IconImage. The IconImage, in turn, is constructed with
a BrowserContext because it needs access to the ImageLoader (a
BrowserContextKeyedService). ExtensionActions are owned by the
ExtensionActionManager (another BrowserContextKeyedService with one
instance shared across incognito and normal profiles), but took a
BrowserContext as an argument when loading the default icon image.

This all caused a problem when an incognito profile was passed in as
the browser context to load the default icon for the extension action.
The extension action would then create an IconImage with the incognito
profile as a context, but the ExtensionAction (and thus the IconImage)
are owned by the ExtensionActionManager, and are therefore not deleted
upon the incognito profile's destruction. This means that if you have a
flow where the ExtensionAction's default icon is loaded via an incognito
profile, the incognito profile is deleted, and then the icon is later
used in the normal profile, there's a crash.

Fix this by having the ExtensionActionManager assign the IconImage to
the ExtensionAction using its own profile. Since the ExtensionActions
are owned by the ExtensionActionManager, and the ExtensionActionManager
is owned by the profile by being a BrowserContextKeyedService, this is
guaranteed to be safe. This also abstracts out the context of a profile
or BrowserContext from the ExtensionAction class, keeping it more in
line with its data-structure-like concept.

Add a regression test.

BUG=663726 (and possibly others)
==========

[commit-bot: I haz the power, 2016-11-17 19:42:29]

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

[commit-bot: I haz the power, 2016-11-17 19:42:30]

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

[Devlin, 2016-11-17 20:59:27]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-17 21:00:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-11-17 21:36:20]

Patchset #3 (id:40001) has been deleted

[Devlin, 2016-11-17 21:36:26]

Patchset #2 (id:20001) has been deleted

[Devlin, 2016-11-17 21:36:34]

Patchset #2 (id:60001) has been deleted

[commit-bot: I haz the power, 2016-11-17 23:17:48]

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

[commit-bot: I haz the power, 2016-11-17 23:17:48]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-11-17 23:33:43]

rdevlin.cronin@chromium.org changed reviewers:
+ asargent@chromium.org

[Devlin, 2016-11-17 23:33:44]

Fun! :)

[asargent_no_longer_on_chrome, 2016-11-18 00:55:27]

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

https://codereview.chromium.org/2506273002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_action_manager.cc:120:
ExtensionAction::DefaultIcon().AsImageSkia(), nullptr));
The naming here is pretty confusing and I really had to think things through to
understand it, as there are 3 different things named something like "default
icon":

ExtensionAction::DefaultIcon() [static method]
- returns gfx::Image of puzzle piece

ExtensionAction::default_icon() [instance method]
- returns ExtensionIconSet of icons from "icons" section of manifest, if no
specific browser/page action icons were specified


ExtensionAction::default_icon_image() [instance method]
- returns IconImage created with default_icon_ (or maybe DefaultIcon() in some
cases?)


Can we rename one or two of these? For instance, how about calling DefaultIcon
"FallbackIcon" or "LastResortIcon" or something like that?


Also, as an aside, if we only call SetDefaultIconImage when default_icon() is
non-null, will the IconImage ever use the DefaultIcon().AsImageSkia() we pass to
it? (I guess maybe if default_icon was non-null but also empty?)

[Devlin, 2016-11-18 01:53:59]

Patchset #3 (id:100001) has been deleted

[Devlin, 2016-11-18 01:56:47]

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

https://codereview.chromium.org/2506273002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_action_manager.cc:120:
ExtensionAction::DefaultIcon().AsImageSkia(), nullptr));
On 2016/11/18 00:55:27, Antony Sargent wrote:
> The naming here is pretty confusing and I really had to think things through
to
> understand it, as there are 3 different things named something like "default
> icon":
> 
> ExtensionAction::DefaultIcon() [static method]
> - returns gfx::Image of puzzle piece
> 
> ExtensionAction::default_icon() [instance method]
> - returns ExtensionIconSet of icons from "icons" section of manifest, if no
> specific browser/page action icons were specified
> 
> 
> ExtensionAction::default_icon_image() [instance method]
> - returns IconImage created with default_icon_ (or maybe DefaultIcon() in some
> cases?)
> 
> 
> Can we rename one or two of these? For instance, how about calling DefaultIcon
> "FallbackIcon" or "LastResortIcon" or something like that?
> 
> 
> Also, as an aside, if we only call SetDefaultIconImage when default_icon() is
> non-null, will the IconImage ever use the DefaultIcon().AsImageSkia() we pass
to
> it? (I guess maybe if default_icon was non-null but also empty?)

Rename DefaultIcon() to FallbackIcon() - I agree that makes things a bit more
clear.  Now default icon only refers to the default icon for the extension.

Re the default icon that's passed to the IconImage, it's used when the icon
loading fails somehow (either it's empty or the resource doesn't load properly,
etc).  We might be able to clean it up a bit, but it seems out of scope of this
CL.

[asargent_no_longer_on_chrome, 2016-11-18 18:29:33]

lgtm

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

https://codereview.chromium.org/2506273002/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/extension_action_manager.cc:120:
ExtensionAction::DefaultIcon().AsImageSkia(), nullptr));
> Re the default icon that's passed to the IconImage, it's used when the icon
> loading fails somehow (either it's empty or the resource doesn't load
properly,
> etc).  We might be able to clean it up a bit, but it seems out of scope of
this
> CL.

Yeah, agreed. I guess to complete the thought, I was wondering if we could just
pass an empty ImageSkia, but I guess it is possible that the actual icon loading
would fail somehow.

[Devlin, 2016-11-18 22:22:34]

The CQ bit was checked by rdevlin.cronin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-18 22:23:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Devlin, 2016-11-19 00:04:20]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[Devlin, 2016-11-19 00:04:25]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-11-19 00:05:23]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-19 01:03:55]

Description was changed from

==========
[Extensions] Fix lifetime bug in ExtensionAction/IconImage

ExtensionActions can lazily load the default icon image, which is
implemented as an IconImage. The IconImage, in turn, is constructed with
a BrowserContext because it needs access to the ImageLoader (a
BrowserContextKeyedService). ExtensionActions are owned by the
ExtensionActionManager (another BrowserContextKeyedService with one
instance shared across incognito and normal profiles), but took a
BrowserContext as an argument when loading the default icon image.

This all caused a problem when an incognito profile was passed in as
the browser context to load the default icon for the extension action.
The extension action would then create an IconImage with the incognito
profile as a context, but the ExtensionAction (and thus the IconImage)
are owned by the ExtensionActionManager, and are therefore not deleted
upon the incognito profile's destruction. This means that if you have a
flow where the ExtensionAction's default icon is loaded via an incognito
profile, the incognito profile is deleted, and then the icon is later
used in the normal profile, there's a crash.

Fix this by having the ExtensionActionManager assign the IconImage to
the ExtensionAction using its own profile. Since the ExtensionActions
are owned by the ExtensionActionManager, and the ExtensionActionManager
is owned by the profile by being a BrowserContextKeyedService, this is
guaranteed to be safe. This also abstracts out the context of a profile
or BrowserContext from the ExtensionAction class, keeping it more in
line with its data-structure-like concept.

Add a regression test.

BUG=663726 (and possibly others)
==========

to

==========
[Extensions] Fix lifetime bug in ExtensionAction/IconImage

ExtensionActions can lazily load the default icon image, which is
implemented as an IconImage. The IconImage, in turn, is constructed with
a BrowserContext because it needs access to the ImageLoader (a
BrowserContextKeyedService). ExtensionActions are owned by the
ExtensionActionManager (another BrowserContextKeyedService with one
instance shared across incognito and normal profiles), but took a
BrowserContext as an argument when loading the default icon image.

This all caused a problem when an incognito profile was passed in as
the browser context to load the default icon for the extension action.
The extension action would then create an IconImage with the incognito
profile as a context, but the ExtensionAction (and thus the IconImage)
are owned by the ExtensionActionManager, and are therefore not deleted
upon the incognito profile's destruction. This means that if you have a
flow where the ExtensionAction's default icon is loaded via an incognito
profile, the incognito profile is deleted, and then the icon is later
used in the normal profile, there's a crash.

Fix this by having the ExtensionActionManager assign the IconImage to
the ExtensionAction using its own profile. Since the ExtensionActions
are owned by the ExtensionActionManager, and the ExtensionActionManager
is owned by the profile by being a BrowserContextKeyedService, this is
guaranteed to be safe. This also abstracts out the context of a profile
or BrowserContext from the ExtensionAction class, keeping it more in
line with its data-structure-like concept.

Add a regression test.

BUG=663726 (and possibly others)
==========

[commit-bot: I haz the power, 2016-11-19 01:03:57]

Committed patchset #3 (id:120001)

[commit-bot: I haz the power, 2016-11-19 01:06:08]

Description was changed from

==========
[Extensions] Fix lifetime bug in ExtensionAction/IconImage

ExtensionActions can lazily load the default icon image, which is
implemented as an IconImage. The IconImage, in turn, is constructed with
a BrowserContext because it needs access to the ImageLoader (a
BrowserContextKeyedService). ExtensionActions are owned by the
ExtensionActionManager (another BrowserContextKeyedService with one
instance shared across incognito and normal profiles), but took a
BrowserContext as an argument when loading the default icon image.

This all caused a problem when an incognito profile was passed in as
the browser context to load the default icon for the extension action.
The extension action would then create an IconImage with the incognito
profile as a context, but the ExtensionAction (and thus the IconImage)
are owned by the ExtensionActionManager, and are therefore not deleted
upon the incognito profile's destruction. This means that if you have a
flow where the ExtensionAction's default icon is loaded via an incognito
profile, the incognito profile is deleted, and then the icon is later
used in the normal profile, there's a crash.

Fix this by having the ExtensionActionManager assign the IconImage to
the ExtensionAction using its own profile. Since the ExtensionActions
are owned by the ExtensionActionManager, and the ExtensionActionManager
is owned by the profile by being a BrowserContextKeyedService, this is
guaranteed to be safe. This also abstracts out the context of a profile
or BrowserContext from the ExtensionAction class, keeping it more in
line with its data-structure-like concept.

Add a regression test.

BUG=663726 (and possibly others)
==========

to

==========
[Extensions] Fix lifetime bug in ExtensionAction/IconImage

ExtensionActions can lazily load the default icon image, which is
implemented as an IconImage. The IconImage, in turn, is constructed with
a BrowserContext because it needs access to the ImageLoader (a
BrowserContextKeyedService). ExtensionActions are owned by the
ExtensionActionManager (another BrowserContextKeyedService with one
instance shared across incognito and normal profiles), but took a
BrowserContext as an argument when loading the default icon image.

This all caused a problem when an incognito profile was passed in as
the browser context to load the default icon for the extension action.
The extension action would then create an IconImage with the incognito
profile as a context, but the ExtensionAction (and thus the IconImage)
are owned by the ExtensionActionManager, and are therefore not deleted
upon the incognito profile's destruction. This means that if you have a
flow where the ExtensionAction's default icon is loaded via an incognito
profile, the incognito profile is deleted, and then the icon is later
used in the normal profile, there's a crash.

Fix this by having the ExtensionActionManager assign the IconImage to
the ExtensionAction using its own profile. Since the ExtensionActions
are owned by the ExtensionActionManager, and the ExtensionActionManager
is owned by the profile by being a BrowserContextKeyedService, this is
guaranteed to be safe. This also abstracts out the context of a profile
or BrowserContext from the ExtensionAction class, keeping it more in
line with its data-structure-like concept.

Add a regression test.

BUG=663726 (and possibly others)

Committed: https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320
Cr-Commit-Position: refs/heads/master@{#433361}
==========

[commit-bot: I haz the power, 2016-11-19 01:06:09]

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