Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(62)

Issue 2506273002: [Extensions] Fix lifetime bug in ExtensionAction/IconImage (Closed)

Created:
2 years, 4 months ago by Devlin
Modified:
2 years, 4 months ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[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}

Patch Set 1 #

Patch Set 2 : missingfile #

Total comments: 3

Patch Set 3 : asargent #

Unified diffs Side-by-side diffs Delta from patch set Stats (+115 lines, -48 lines) Patch
M chrome/browser/extensions/extension_action.h View 1 2 5 chunks +15 lines, -12 lines 0 comments Download
M chrome/browser/extensions/extension_action.cc View 1 2 4 chunks +11 lines, -16 lines 0 comments Download
M chrome/browser/extensions/extension_action_icon_factory.cc View 1 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/extensions/extension_action_icon_factory_unittest.cc View 1 1 chunk +7 lines, -6 lines 0 comments Download
M chrome/browser/extensions/extension_action_manager.cc View 1 2 2 chunks +13 lines, -2 lines 0 comments Download
M chrome/browser/extensions/extension_browsertest.cc View 1 3 chunks +15 lines, -8 lines 0 comments Download
M chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc View 1 2 chunks +42 lines, -0 lines 0 comments Download
A chrome/test/data/extensions/api_test/browser_action_with_icon/icon.png View 1 Binary file 0 comments Download
A chrome/test/data/extensions/api_test/browser_action_with_icon/manifest.json View 1 1 chunk +10 lines, -0 lines 0 comments Download

Messages

Total messages: 31 (24 generated)
Devlin
Fun! :)
2 years, 4 months ago (2016-11-17 23:33:44 UTC) #18
asargent_no_longer_on_chrome
https://codereview.chromium.org/2506273002/diff/80001/chrome/browser/extensions/extension_action_manager.cc File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/2506273002/diff/80001/chrome/browser/extensions/extension_action_manager.cc#newcode120 chrome/browser/extensions/extension_action_manager.cc:120: ExtensionAction::DefaultIcon().AsImageSkia(), nullptr)); The naming here is pretty confusing and ...
2 years, 4 months ago (2016-11-18 00:55:27 UTC) #19
Devlin
https://codereview.chromium.org/2506273002/diff/80001/chrome/browser/extensions/extension_action_manager.cc File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/2506273002/diff/80001/chrome/browser/extensions/extension_action_manager.cc#newcode120 chrome/browser/extensions/extension_action_manager.cc:120: ExtensionAction::DefaultIcon().AsImageSkia(), nullptr)); On 2016/11/18 00:55:27, Antony Sargent wrote: > ...
2 years, 4 months ago (2016-11-18 01:56:47 UTC) #21
asargent_no_longer_on_chrome
lgtm https://codereview.chromium.org/2506273002/diff/80001/chrome/browser/extensions/extension_action_manager.cc File chrome/browser/extensions/extension_action_manager.cc (right): https://codereview.chromium.org/2506273002/diff/80001/chrome/browser/extensions/extension_action_manager.cc#newcode120 chrome/browser/extensions/extension_action_manager.cc:120: ExtensionAction::DefaultIcon().AsImageSkia(), nullptr)); > Re the default icon that's ...
2 years, 4 months ago (2016-11-18 18:29:33 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2506273002/120001
2 years, 4 months ago (2016-11-19 00:05:23 UTC) #27
commit-bot: I haz the power
Committed patchset #3 (id:120001)
2 years, 4 months ago (2016-11-19 01:03:57 UTC) #29
commit-bot: I haz the power
2 years, 4 months ago (2016-11-19 01:06:09 UTC) #31
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/d09e1c4d5c955d73cc38f507ede1119f6c540320
Cr-Commit-Position: refs/heads/master@{#433361}

Powered by Google App Engine
This is Rietveld 408576698