Don't create extension actions for disabled extensions.

Don't create extension actions for disabled extensions.

The GetOrCreateOrNull function creates extension actions if needed and
caches them for later. When an extension is unloaded, the cache is
cleared.
Unfortunately it is possible for other listeners of the
NOTIFICATION_EXTENSION_UNLOADED to call GetOrCreateOrNull and the action
be recreated and cached for the extension being unloaded. The cached
action then exists even if the reloaded extension doesn't have the
action.

This change prevents recreation of the action object for extensions that
are already unloaded.

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

[Jeffrey Yasskin, 2013-07-30 00:25:22]

Are you sure this requires the action to go away across the reload? If this
function gets called after the UNLOADED event, it looks like it would save a
dangling pointer even if the new version still has a browser action.

https://codereview.chromium.org/20304002/diff/1/chrome/browser/extensions/ext...
File chrome/browser/extensions/extension_action_manager.cc (right):

https://codereview.chromium.org/20304002/diff/1/chrome/browser/extensions/ext...
chrome/browser/extensions/extension_action_manager.cc:120: // This avoids bugs
where actions are recreated just after being removed
Argh, use after free. :( Do you know which code path hit this?

I'm worried that any calls to these functions during loading or unloading might
use an Extension object that's not the one that's going to live until the next
unload.

That said, I don't see a great way to ensure that an extension object is the
right one, given our notification system.

[msimonides, 2013-07-30 06:51:33]

On 2013/07/30 00:25:22, Jeffrey Yasskin wrote:
> Are you sure this requires the action to go away across the reload? If this
> function gets called after the UNLOADED event, it looks like it would save a
> dangling pointer even if the new version still has a browser action.

With my change (which is not ideal), if this function gets called after UNLOAD
it will return NULL and not cache anything.
The action doesn't have to go away across reload but at UNLOAD we don't know
whether it will be present in the newly loaded extension.

>
https://codereview.chromium.org/20304002/diff/1/chrome/browser/extensions/ext...
> File chrome/browser/extensions/extension_action_manager.cc (right):
> 
>
https://codereview.chromium.org/20304002/diff/1/chrome/browser/extensions/ext...
> chrome/browser/extensions/extension_action_manager.cc:120: // This avoids bugs
> where actions are recreated just after being removed
> Argh, use after free. :( Do you know which code path hit this?

Other listeners for the NOTIFICATION_EXTENSION_UNLOADED.
In my case it was probably WebUI but we can't assume the order of notifications
is known so it doesn't really matter.

> I'm worried that any calls to these functions during loading or unloading
might
> use an Extension object that's not the one that's going to live until the next
> unload.

True, haven't thought about that.

> That said, I don't see a great way to ensure that an extension object is the
> right one, given our notification system.

IMO there are two issues that lead to this problem:
1. Lazy initialization - there is no control on when the object is created so it
happens in surprising places (and might change in the future).
2. Caching - keeping the cache up-to-date is not as easy as might be expected.

What if we get rid of the first one and always create the objects on
NOTIFICATION_EXTENSION_LOAD and clear the cache on
NOTIFICATION_EXTENSION_UNLOAD? So that the action either always is there or it
isn't.
And change GetOrCreateOrNull to just Get, which may return NULL if there is no
action. It will never mess with the state, just read it.

[msimonides, 2013-07-30 11:01:32]

On 2013/07/30 06:51:33, msimonides wrote:
> On 2013/07/30 00:25:22, Jeffrey Yasskin wrote:
> > Are you sure this requires the action to go away across the reload? If this
> > function gets called after the UNLOADED event, it looks like it would save a
> > dangling pointer even if the new version still has a browser action.
> 
> With my change (which is not ideal), if this function gets called after UNLOAD
> it will return NULL and not cache anything.
> The action doesn't have to go away across reload but at UNLOAD we don't know
> whether it will be present in the newly loaded extension.
> 
> >
>
https://codereview.chromium.org/20304002/diff/1/chrome/browser/extensions/ext...
> > File chrome/browser/extensions/extension_action_manager.cc (right):
> > 
> >
>
https://codereview.chromium.org/20304002/diff/1/chrome/browser/extensions/ext...
> > chrome/browser/extensions/extension_action_manager.cc:120: // This avoids
bugs
> > where actions are recreated just after being removed
> > Argh, use after free. :( Do you know which code path hit this?
> 
> Other listeners for the NOTIFICATION_EXTENSION_UNLOADED.
> In my case it was probably WebUI but we can't assume the order of
notifications
> is known so it doesn't really matter.
> 
> > I'm worried that any calls to these functions during loading or unloading
> might
> > use an Extension object that's not the one that's going to live until the
next
> > unload.
> 
> True, haven't thought about that.
> 
> > That said, I don't see a great way to ensure that an extension object is the
> > right one, given our notification system.
> 
> IMO there are two issues that lead to this problem:
> 1. Lazy initialization - there is no control on when the object is created so
it
> happens in surprising places (and might change in the future).
> 2. Caching - keeping the cache up-to-date is not as easy as might be expected.
> 
> What if we get rid of the first one and always create the objects on
> NOTIFICATION_EXTENSION_LOAD and clear the cache on
> NOTIFICATION_EXTENSION_UNLOAD? So that the action either always is there or it
> isn't.
> And change GetOrCreateOrNull to just Get, which may return NULL if there is no
> action. It will never mess with the state, just read it.

I tried doing that and got hit by the opposite side of the problem: some LOAD
observers try to access the action before the ExtensionActionManager has a
chance to act on the LOAD notification.
At the moment I don't have any good ideas, the ones I have add dependencies
(e.g. ExtensionActionManager could be called directly by the ExtensionService
before notifications are send. It's ugly) or disable the cache.

The proposed workaround is the best I have for now.

[Jeffrey Yasskin, 2013-07-30 19:07:48]

lgtm. Lots of commentary below, but I don't have any changes to
suggest. Thanks for the fix!

On Tue, Jul 30, 2013 at 4:01 AM,  <msimonides@opera.com> wrote:
> On 2013/07/30 06:51:33, msimonides wrote:
>>
>> On 2013/07/30 00:25:22, Jeffrey Yasskin wrote:
>> > Are you sure this requires the action to go away across the reload? If
>> > this
>> > function gets called after the UNLOADED event, it looks like it would
>> > save a
>> > dangling pointer even if the new version still has a browser action.
>
>
>> With my change (which is not ideal), if this function gets called after
>> UNLOAD
>> it will return NULL and not cache anything.
>> The action doesn't have to go away across reload but at UNLOAD we don't
>> know
>> whether it will be present in the newly loaded extension.

Sorry, I meant to ask about the reproduction. Does the bug only happen
if the action goes away, or do we also get a use-after-free when the
action sticks around? One might need to run under ASan to see the
crash.

(Don't block the fix on figuring this out; I'm just wondering how far
the bug went; I agree your fix fixes this regardless.)

>
https://codereview.chromium.org/20304002/diff/1/chrome/browser/extensions/ext...
>>
>> > File chrome/browser/extensions/extension_action_manager.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/20304002/diff/1/chrome/browser/extensions/ext...
>>
>> > chrome/browser/extensions/extension_action_manager.cc:120: // This
>> > avoids
>
> bugs
>>
>> > where actions are recreated just after being removed
>> > Argh, use after free. :( Do you know which code path hit this?
>
>
>> Other listeners for the NOTIFICATION_EXTENSION_UNLOADED.
>> In my case it was probably WebUI but we can't assume the order of
>
> notifications
>>
>> is known so it doesn't really matter.

I asked because we might be able to solve this by removing uses of
ExtensionActions from UNLOADED notifications. If there's only one,
removing it and then CHECKing in this function could work better than
just returning NULL from here.

>> > I'm worried that any calls to these functions during loading or
>> > unloading
>> might
>> > use an Extension object that's not the one that's going to live until
>> > the
>
> next
>>
>> > unload.
>
>
>> True, haven't thought about that.
>
>
>> > That said, I don't see a great way to ensure that an extension object is
>> > the
>> > right one, given our notification system.
>
>
>> IMO there are two issues that lead to this problem:
>> 1. Lazy initialization - there is no control on when the object is created
>> so
>
> it
>>
>> happens in surprising places (and might change in the future).
>> 2. Caching - keeping the cache up-to-date is not as easy as might be
>> expected.

Yep. But note that this isn't simply a cache. The ExtensionAction
maintains runtime state that needs to survive through the Extension's
full lifetime. Re-computing it isn't an option.

>> What if we get rid of the first one and always create the objects on
>> NOTIFICATION_EXTENSION_LOAD and clear the cache on
>> NOTIFICATION_EXTENSION_UNLOAD? So that the action either always is there
>> or it
>> isn't.
>> And change GetOrCreateOrNull to just Get, which may return NULL if there
>> is no
>> action. It will never mess with the state, just read it.
>
>
> I tried doing that and got hit by the opposite side of the problem: some
> LOAD
> observers try to access the action before the ExtensionActionManager has a
> chance to act on the LOAD notification.
> At the moment I don't have any good ideas, the ones I have add dependencies
> (e.g. ExtensionActionManager could be called directly by the
> ExtensionService
> before notifications are send. It's ugly) or disable the cache.

Exactly. :( I'd like to find a way to make these dependencies explicit
without making, say, ExtensionService explicitly list everything
that's part of constructing an Extension, but I don't have a good
design yet. If you come up with one, patches are very welcome. :)

> The proposed workaround is the best I have for now.
>
> https://codereview.chromium.org/20304002/

[commit-bot: I haz the power, 2013-07-31 07:56:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msimonides@opera.com/20304002/1

[msimonides, 2013-07-31 08:05:13]

On 2013/07/30 19:07:48, Jeffrey Yasskin wrote:
> lgtm. Lots of commentary below, but I don't have any changes to
> suggest. Thanks for the fix!
> 
> On Tue, Jul 30, 2013 at 4:01 AM,  <mailto:msimonides@opera.com> wrote:
> > On 2013/07/30 06:51:33, msimonides wrote:
> >>
> >> On 2013/07/30 00:25:22, Jeffrey Yasskin wrote:
> >> > Are you sure this requires the action to go away across the reload? If
> >> > this
> >> > function gets called after the UNLOADED event, it looks like it would
> >> > save a
> >> > dangling pointer even if the new version still has a browser action.
> >
> >
> >> With my change (which is not ideal), if this function gets called after
> >> UNLOAD
> >> it will return NULL and not cache anything.
> >> The action doesn't have to go away across reload but at UNLOAD we don't
> >> know
> >> whether it will be present in the newly loaded extension.
> 
> Sorry, I meant to ask about the reproduction. Does the bug only happen
> if the action goes away, or do we also get a use-after-free when the
> action sticks around? One might need to run under ASan to see the
> crash.

The bug happens when the action goes away. That's the only scenario I know.
Initially there is no crash but there is a browser action button on the toolbar.
Another reload causes a crash.
To be honest I have fixed this issue for Opera and haven't done much debugging
on Chrome. Mainly I have confirmed that the change actually fixes a crash.

[...]
> >> Other listeners for the NOTIFICATION_EXTENSION_UNLOADED.
> >> In my case it was probably WebUI but we can't assume the order of
> >
> > notifications
> >>
> >> is known so it doesn't really matter.
> 
> I asked because we might be able to solve this by removing uses of
> ExtensionActions from UNLOADED notifications. If there's only one,
> removing it and then CHECKing in this function could work better than
> just returning NULL from here.

The suspects are WebUI and the toolbar but that needs to be confirmed.

[commit-bot: I haz the power, 2013-07-31 10:54:43]

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

[commit-bot: I haz the power, 2013-07-31 11:32:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msimonides@opera.com/20304002/1

[commit-bot: I haz the power, 2013-07-31 11:48:36]

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

[commit-bot: I haz the power, 2013-07-31 13:49:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msimonides@opera.com/20304002/1

[commit-bot: I haz the power, 2013-07-31 17:33:54]

Change committed as 214767