Observe OnExtensionLoaded to trigger update of chrome.commands keybindings for component extensions.

Observe OnExtensionLoaded to trigger update of chrome.commands keybindings for component extensions.

ExtensionRegistryObserver::OnExtensionWillBeInstalled only triggers when a component extension loads for the first time on a new profile. As a result, component extensions using the commands api never get their commands manifest block re-parsed on an update.

The fix is to observe load to trigger update of keybindings which differs from what happens in ExtensionKyebindingRegistry's OnExtensionLoaded; namely, the kExtensionBindings pref gets set in the former only.

BUG=440459
TEST=manually edit a component extension's manifest command keybinding. Load the component extension and observe the keybinding changes and triggers on the new keybinding.

Committed: https://crrev.com/7c3129ac745bd5598bde8ce572444f9a7224e4ce
Cr-Commit-Position: refs/heads/master@{#311320}

[David Tseng, 2014-12-09 21:41:09]

dtseng@chromium.org changed reviewers:
+ finnur@chromium.org

[David Tseng, 2014-12-10 00:45:32]

On 2014/12/09 21:41:09, David Tseng wrote:

FYI. This appears to be W.I.P. since the tests fail. Please have a quick look.
Thanks.

[Finnur, 2014-12-10 12:25:38]

https://codereview.chromium.org/788973002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/788973002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:306:
RemoveKeybindingPrefs(extension->id(), std::string());
If I temporarily disable an extension/it crashes, won't that 
unregister the keybinding? What if a new extension is then installed, which
overlaps in keybindings. Who wins if the initial extension gets re-enabled?

What happens if I install extension A and B, which request the same keybinding
and start toggling them enabled/disabled. Will keybindings change as a result?

Should we perhaps listen to both sets of series (Load-Unload vs.
Install-Uninstall) and act on Load/Unload only for Component Extensions, because
those won't be "toggle-able"?

[David Tseng, 2014-12-10 17:25:47]

Patchset #2 (id:20001) has been deleted

[David Tseng, 2014-12-10 17:27:51]

https://codereview.chromium.org/788973002/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/788973002/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/commands/command_service.cc:306:
RemoveKeybindingPrefs(extension->id(), std::string());
On 2014/12/10 12:25:38, Finnur wrote:
> If I temporarily disable an extension/it crashes, won't that 
> unregister the keybinding? What if a new extension is then installed, which
> overlaps in keybindings. Who wins if the initial extension gets re-enabled?
> 
> What happens if I install extension A and B, which request the same keybinding
> and start toggling them enabled/disabled. Will keybindings change as a result?
> 
> Should we perhaps listen to both sets of series (Load-Unload vs.
> Install-Uninstall) and act on Load/Unload only for Component Extensions,
because
> those won't be "toggle-able"?

Done.
Yup; that's the right way to do this; had only tested with component extensions.
Install does get called when the component extension gets loaded into a new
profile, so we need to qualify on Manifest::COMPONENT. in all OnExtension*.

[Finnur, 2014-12-11 13:51:49]

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:303: if
(extension->location() != Manifest::COMPONENT)
I would document this with:

// Component extensions don't generate normal install/uninstall 
// events, so those are handled in OnLoaded and Unloaded.

... or something.

But... can you refresh my memory as to why Component extensions don't generate
the install/uninstall events on update? Maybe an even better fix is to start
doing so? Hmmm... I can't remember how component extensions behave in this
regard. Maybe Antony should weigh in, or set me straight. :)

Also, if component extension update generates no event, then there's no need for
this guard. I'd be tempted to do:

DCHECK(extension->location() != Manifest::COMPONENT);

... instead, because if it starts sending these events, we'd want to revert this
change...

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:326:
RemoveKeybindingPrefs(extension->id(), std::string());
Do we even want to remove keybindings for component extensions on every unload?
That means if one gets disabled it might potentially lose its keybinding to
another extension, which is not necessarily good. Isn't just the update problem
an issue for us (which is solved by OnExtensionLoaded)?

[asargent_no_longer_on_chrome, 2014-12-11 22:03:11]

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

[asargent_no_longer_on_chrome, 2014-12-11 22:03:23]

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

[asargent_no_longer_on_chrome, 2014-12-11 22:03:49]

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

[asargent_no_longer_on_chrome, 2014-12-11 22:07:58]

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

[asargent_no_longer_on_chrome, 2014-12-11 22:11:51]

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

[asargent_no_longer_on_chrome, 2014-12-11 22:14:43]

Huh, I took a brief look and didn't see any code around triggering the
OnExtensionWillBeInstalled callbacks that actively tries to exclude
component
extensions.

Note that there are 2 kinds of component extensions:

a) The kind that are built into the chrome resource file. I think these are
handled more like development loaded ones, in that we don't really have the
concept of "updates" even though their source code can of course change
from one
chrome build to another.

b) external component ones, which are more like regularly installed
extensions
and can be autoupdated.

On Thu, Dec 11, 2014 at 5:51 AM, <finnur@chromium.org> wrote:
>
>
> https://codereview.chromium.org/788973002/diff/40001/
> chrome/browser/extensions/api/commands/command_service.cc
> File chrome/browser/extensions/api/commands/command_service.cc (right):
>
> https://codereview.chromium.org/788973002/diff/40001/
> chrome/browser/extensions/api/commands/command_service.cc#newcode303
> chrome/browser/extensions/api/commands/command_service.cc:303: if
> (extension->location() != Manifest::COMPONENT)
> I would document this with:
>
> // Component extensions don't generate normal install/uninstall
> // events, so those are handled in OnLoaded and Unloaded.
>
> ... or something.
>
> But... can you refresh my memory as to why Component extensions don't
> generate the install/uninstall events on update? Maybe an even better
> fix is to start doing so? Hmmm... I can't remember how component
> extensions behave in this regard. Maybe Antony should weigh in, or set
> me straight. :)
>
> Also, if component extension update generates no event, then there's no
> need for this guard. I'd be tempted to do:
>
> DCHECK(extension->location() != Manifest::COMPONENT);
>
> ... instead, because if it starts sending these events, we'd want to
> revert this change...
>
> https://codereview.chromium.org/788973002/diff/40001/
> chrome/browser/extensions/api/commands/command_service.cc#newcode326
> chrome/browser/extensions/api/commands/command_service.cc:326:
> RemoveKeybindingPrefs(extension->id(), std::string());
> Do we even want to remove keybindings for component extensions on every
> unload? That means if one gets disabled it might potentially lose its
> keybinding to another extension, which is not necessarily good. Isn't
> just the update problem an issue for us (which is solved by
> OnExtensionLoaded)?
>
> https://codereview.chromium.org/788973002/
>

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

[David Tseng, 2014-12-12 18:09:01]

On 2014/12/11 22:14:43, asargent wrote:
> Huh, I took a brief look and didn't see any code around triggering the
> OnExtensionWillBeInstalled callbacks that actively tries to exclude
> component
> extensions.
> 

What I am seeing is a built-in (type a) component extension _not_ trigger the
OnExtensionInstalled callbacks after the first time Chrome OS gets launched.
Unfortunately, this means changes in the manifest never get picked up until the
entire profile gets deleted which appears to once again cause the
OnExtensionInstalled callback to get triggered.

> Note that there are 2 kinds of component extensions:
> 
> a) The kind that are built into the chrome resource file. I think these are
> handled more like development loaded ones, in that we don't really have the
> concept of "updates" even though their source code can of course change
> from one
> chrome build to another.
> 
> b) external component ones, which are more like regularly installed
> extensions
> and can be autoupdated.
> 
> On Thu, Dec 11, 2014 at 5:51 AM, <mailto:finnur@chromium.org> wrote:
> >
> >
> > https://codereview.chromium.org/788973002/diff/40001/
> > chrome/browser/extensions/api/commands/command_service.cc
> > File chrome/browser/extensions/api/commands/command_service.cc (right):
> >
> > https://codereview.chromium.org/788973002/diff/40001/
> > chrome/browser/extensions/api/commands/command_service.cc#newcode303
> > chrome/browser/extensions/api/commands/command_service.cc:303: if
> > (extension->location() != Manifest::COMPONENT)
> > I would document this with:
> >
> > // Component extensions don't generate normal install/uninstall
> > // events, so those are handled in OnLoaded and Unloaded.
> >
> > ... or something.
> >
> > But... can you refresh my memory as to why Component extensions don't
> > generate the install/uninstall events on update? Maybe an even better
> > fix is to start doing so? Hmmm... I can't remember how component
> > extensions behave in this regard. Maybe Antony should weigh in, or set
> > me straight. :)
> >
> > Also, if component extension update generates no event, then there's no
> > need for this guard. I'd be tempted to do:
> >
> > DCHECK(extension->location() != Manifest::COMPONENT);
> >
> > ... instead, because if it starts sending these events, we'd want to
> > revert this change...
> >
> > https://codereview.chromium.org/788973002/diff/40001/
> > chrome/browser/extensions/api/commands/command_service.cc#newcode326
> > chrome/browser/extensions/api/commands/command_service.cc:326:
> > RemoveKeybindingPrefs(extension->id(), std::string());
> > Do we even want to remove keybindings for component extensions on every
> > unload? That means if one gets disabled it might potentially lose its
> > keybinding to another extension, which is not necessarily good. Isn't
> > just the update problem an issue for us (which is solved by
> > OnExtensionLoaded)?
> >
> > https://codereview.chromium.org/788973002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[David Tseng, 2014-12-12 18:38:31]

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:303: if
(extension->location() != Manifest::COMPONENT)
On 2014/12/11 13:51:49, Finnur wrote:
> I would document this with:
> 
> // Component extensions don't generate normal install/uninstall 
> // events, so those are handled in OnLoaded and Unloaded.
> 
> ... or something.
> 
> But... can you refresh my memory as to why Component extensions don't generate
> the install/uninstall events on update? Maybe an even better fix is to start
> doing so? Hmmm... I can't remember how component extensions behave in this
> regard. Maybe Antony should weigh in, or set me straight. :)
> 
> Also, if component extension update generates no event, then there's no need
for
> this guard. I'd be tempted to do:
> 
> DCHECK(extension->location() != Manifest::COMPONENT);
> 
> ... instead, because if it starts sending these events, we'd want to revert
this
> change...

As mentioned before, I was seeing component extensions generating this callback
once immediately after a profile deletion/Chrome launch. Thereafter, I wasn't
seeing this being called at all.

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:326:
RemoveKeybindingPrefs(extension->id(), std::string());
On 2014/12/11 13:51:49, Finnur wrote:
> Do we even want to remove keybindings for component extensions on every
unload?
> That means if one gets disabled it might potentially lose its keybinding to
> another extension, which is not necessarily good. Isn't just the update
problem
> an issue for us (which is solved by OnExtensionLoaded)?

Removed this method.

[Finnur, 2014-12-15 14:01:43]

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:303: if
(extension->location() != Manifest::COMPONENT)
OK, in that case I'd remove the if-check from OnExtensionUninstalled and always
call RemoveKeybindingPrefs. I think it isn't right to leave the keybinding
behind after the extension uninstalls. I don't think it will make a world of
difference if it only happens when the profile is getting deleted, but at least
the reader doesn't have to wonder why we don't do it for component extensions.

https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:304: // those are
handled in loaded and unloaded.
This comment is now outdated (specifically unloaded).

https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:305: if
(extension->location() != Manifest::COMPONENT)
nit: Weird indentation.

https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:319: LOG(ERROR) <<
"loaded extension=" << extension->id();
This should not be here. Again, WIP? Would be good to know beforehand if I'm not
supposed to review the CL.

https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.cc:140: DCHECK_EQ(1u,
event_targets_[accelerator].size()) << "Too many targets " <<
accelerator.GetShortcutText() << " id:" << extension_id << " name:" <<
command_name;
This breaks a couple of style-rules, including 80 cols and braces around
multi-line if bodies. But, it doesn't look like this fixes anything -- is this
still WIP?

[David Tseng, 2014-12-15 16:27:58]

On 2014/12/15 14:01:43, Finnur wrote:
>
https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
> File chrome/browser/extensions/api/commands/command_service.cc (right):
> 
>
https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
> chrome/browser/extensions/api/commands/command_service.cc:303: if
> (extension->location() != Manifest::COMPONENT)
> OK, in that case I'd remove the if-check from OnExtensionUninstalled and
always
> call RemoveKeybindingPrefs. I think it isn't right to leave the keybinding
> behind after the extension uninstalls. I don't think it will make a world of
> difference if it only happens when the profile is getting deleted, but at
least
> the reader doesn't have to wonder why we don't do it for component extensions.
> 
>
https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
> File chrome/browser/extensions/api/commands/command_service.cc (right):
> 
>
https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
> chrome/browser/extensions/api/commands/command_service.cc:304: // those are
> handled in loaded and unloaded.
> This comment is now outdated (specifically unloaded).
> 
>
https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
> chrome/browser/extensions/api/commands/command_service.cc:305: if
> (extension->location() != Manifest::COMPONENT)
> nit: Weird indentation.
> 
>
https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
> chrome/browser/extensions/api/commands/command_service.cc:319: LOG(ERROR) <<
> "loaded extension=" << extension->id();
> This should not be here. Again, WIP? Would be good to know beforehand if I'm
not
> supposed to review the CL.
> 
>
https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
> File chrome/browser/extensions/extension_keybinding_registry.cc (right):
> 
>
https://codereview.chromium.org/788973002/diff/140001/chrome/browser/extensio...
> chrome/browser/extensions/extension_keybinding_registry.cc:140: DCHECK_EQ(1u,
> event_targets_[accelerator].size()) << "Too many targets " <<
> accelerator.GetShortcutText() << " id:" << extension_id << " name:" <<
> command_name;
> This breaks a couple of style-rules, including 80 cols and braces around
> multi-line if bodies. But, it doesn't look like this fixes anything -- is this
> still WIP?

Latest patchsets fail on a bot (but not locally); trying to debug so ignore all
patchsets entitled *debug*.

[David Tseng, 2014-12-15 18:33:20]

Patchset #7 (id:140001) has been deleted

[David Tseng, 2014-12-15 18:33:30]

Patchset #6 (id:120001) has been deleted

[David Tseng, 2014-12-15 18:33:39]

Patchset #5 (id:100001) has been deleted

[David Tseng, 2014-12-17 01:08:56]

Patchset #11 (id:280001) has been deleted

[David Tseng, 2014-12-17 01:09:15]

Patchset #10 (id:260001) has been deleted

[David Tseng, 2014-12-17 01:09:28]

Patchset #9 (id:240001) has been deleted

[David Tseng, 2014-12-17 01:09:59]

Patchset #7 (id:200001) has been deleted

[David Tseng, 2014-12-17 01:10:29]

Patchset #7 (id:220001) has been deleted

[David Tseng, 2014-12-17 01:10:42]

Patchset #6 (id:180001) has been deleted

[David Tseng, 2014-12-17 01:11:01]

Patchset #5 (id:160001) has been deleted

[David Tseng, 2014-12-18 01:06:10]

PTAL; got all tests to pass.

[Finnur, 2014-12-19 15:26:52]

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:303: if
(extension->location() != Manifest::COMPONENT)
On 2014/12/15 14:01:42, Finnur wrote:
> OK, in that case I'd remove the if-check from OnExtensionUninstalled and
always
> call RemoveKeybindingPrefs. I think it isn't right to leave the keybinding
> behind after the extension uninstalls. I don't think it will make a world of
> difference if it only happens when the profile is getting deleted, but at
least
> the reader doesn't have to wonder why we don't do it for component extensions.

What about this?

https://codereview.chromium.org/788973002/diff/380001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/788973002/diff/380001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.cc:146: return;
Hmm... I'm not sure I like all this. I'm sure it gets the tests to pass but I'm
worried we are papering over a problem.

Originally, we had a simple DCHECK:
   DCHECK_EQ(1u, event_targets_[accelerator].size());
... which was designed to catch when we were adding another target for the same
accelerator, because that should never happen.

Then MediaKeys were introduced, and we realized that they should be
non-exclusive. Because such keys can have more than one target and the code
makes sure to let all consumers know when that key is struck (as we designed).
So we made an exception when IsMediaKey, because it suppports multiple targets.

Now, however, looking at the NOTREACHED above, I'm not even sure when it will
hit or what it is guarding now... 

I think we need to figure out why the same accelerator is being added twice and
go back to the DCHECK as it was. At least I'd like to know why it is occurring
before relaxing the restriction.

[David Tseng, 2014-12-19 19:41:01]

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/788973002/diff/40001/chrome/browser/extension...
chrome/browser/extensions/api/commands/command_service.cc:303: if
(extension->location() != Manifest::COMPONENT)
On 2014/12/15 14:01:42, Finnur wrote:
> OK, in that case I'd remove the if-check from OnExtensionUninstalled and
always
> call RemoveKeybindingPrefs. I think it isn't right to leave the keybinding
> behind after the extension uninstalls. I don't think it will make a world of
> difference if it only happens when the profile is getting deleted, but at
least
> the reader doesn't have to wonder why we don't do it for component extensions.

Done.

https://codereview.chromium.org/788973002/diff/380001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/788973002/diff/380001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.cc:146: return;
On 2014/12/19 15:26:52, Finnur wrote:
> Hmm... I'm not sure I like all this. I'm sure it gets the tests to pass but
I'm
> worried we are papering over a problem.
> 
> Originally, we had a simple DCHECK:
>    DCHECK_EQ(1u, event_targets_[accelerator].size());
> ... which was designed to catch when we were adding another target for the
same
> accelerator, because that should never happen.
> 
> Then MediaKeys were introduced, and we realized that they should be
> non-exclusive. Because such keys can have more than one target and the code
> makes sure to let all consumers know when that key is struck (as we designed).
> So we made an exception when IsMediaKey, because it suppports multiple
targets.
> 
> Now, however, looking at the NOTREACHED above, I'm not even sure when it will
> hit or what it is guarding now... 
> 
> I think we need to figure out why the same accelerator is being added twice
and
> go back to the DCHECK as it was. At least I'd like to know why it is occurring
> before relaxing the restriction.

I was seeing multiple event targets getting added, but only on a try bot; I
never did manage to reproduce locally. Any thoughts? 

According to my logging, the same command was sometimes being added twice and
sometimes, a command with an empty name was being added.

I can see if the change to OnExtensionUninstalled makes any difference.

[Finnur, 2014-12-22 15:20:08]

https://codereview.chromium.org/788973002/diff/380001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/788973002/diff/380001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.cc:146: return;
My recommendation would be to revert back to the simpler DCHECK. Then, if it
passes locally, run it through the try bot and see if the change to
OnExtensionUninstalled does the trick.

If it fails locally, then you should be able to see the two callstacks that lead
to the event being added (you can run the test on its own and make it crash when
the first request comes in -- the DCHECK should already have shown you the
second time the request comes in, I suppose, otherwise you can just make it
crash the second time the request comes in).

You might even be able to do the same trick on the bots, if it doesn't repro
locally.

[David Tseng, 2015-01-07 22:46:25]

Patchset #20 (id:600001) has been deleted

[David Tseng, 2015-01-07 22:46:35]

Patchset #19 (id:580001) has been deleted

[David Tseng, 2015-01-07 22:46:45]

Patchset #18 (id:560001) has been deleted

[David Tseng, 2015-01-07 22:46:54]

Patchset #17 (id:540001) has been deleted

[David Tseng, 2015-01-07 22:47:02]

Patchset #16 (id:520001) has been deleted

[David Tseng, 2015-01-07 22:47:11]

Patchset #15 (id:500001) has been deleted

[David Tseng, 2015-01-07 22:47:19]

Patchset #14 (id:480001) has been deleted

[David Tseng, 2015-01-07 22:47:29]

Patchset #13 (id:460001) has been deleted

[David Tseng, 2015-01-07 22:47:38]

Patchset #12 (id:440001) has been deleted

[David Tseng, 2015-01-07 22:47:47]

Patchset #11 (id:420001) has been deleted

[David Tseng, 2015-01-07 22:48:01]

Patchset #10 (id:400001) has been deleted

[David Tseng, 2015-01-07 22:49:42]

PTAL

On 2014/12/22 15:20:08, Finnur wrote:
>
https://codereview.chromium.org/788973002/diff/380001/chrome/browser/extensio...
> File chrome/browser/extensions/extension_keybinding_registry.cc (right):
> 
>
https://codereview.chromium.org/788973002/diff/380001/chrome/browser/extensio...
> chrome/browser/extensions/extension_keybinding_registry.cc:146: return;
> My recommendation would be to revert back to the simpler DCHECK. Then, if it
> passes locally, run it through the try bot and see if the change to
> OnExtensionUninstalled does the trick.
> 
> If it fails locally, then you should be able to see the two callstacks that
lead
> to the event being added (you can run the test on its own and make it crash
when
> the first request comes in -- the DCHECK should already have shown you the
> second time the request comes in, I suppose, otherwise you can just make it
> crash the second time the request comes in).
> 
> You might even be able to do the same trick on the bots, if it doesn't repro
> locally.

It turned out that ExtensionKeybindingRegistry also registers for
OnExtensionLoaded. It was likely a race condition which caused that particular
observer to get called on the bots, but somehow not locally.

[Finnur, 2015-01-08 14:13:50]

https://codereview.chromium.org/788973002/diff/650001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/788973002/diff/650001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:304: // those are
handled in loaded.
s/in loaded/in OnExtensionLoaded/

https://codereview.chromium.org/788973002/diff/650001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/788973002/diff/650001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.cc:202: // installs as
well as loads. This can cause adding of multiple key
It seems weird that OnExtensionLoaded is called twice for component extensions.
Is it clear why that happens? Is it perhaps a bug?

https://codereview.chromium.org/788973002/diff/650001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.cc:205: return;
We don't need to early-return when type == COMMAND_REMOVED, do we?

[David Tseng, 2015-01-08 20:34:41]

https://codereview.chromium.org/788973002/diff/650001/chrome/browser/extensio...
File chrome/browser/extensions/api/commands/command_service.cc (right):

https://codereview.chromium.org/788973002/diff/650001/chrome/browser/extensio...
chrome/browser/extensions/api/commands/command_service.cc:304: // those are
handled in loaded.
On 2015/01/08 14:13:50, Finnur wrote:
> s/in loaded/in OnExtensionLoaded/

Done.

https://codereview.chromium.org/788973002/diff/650001/chrome/browser/extensio...
File chrome/browser/extensions/extension_keybinding_registry.cc (right):

https://codereview.chromium.org/788973002/diff/650001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.cc:202: // installs as
well as loads. This can cause adding of multiple key
On 2015/01/08 14:13:50, Finnur wrote:
> It seems weird that OnExtensionLoaded is called twice for component
extensions.
> Is it clear why that happens? Is it perhaps a bug?

It is clear after I got the right logging in. OnExtensionLoaded gets called just
once, but since there's two observers for OnExtensionLoaded
(CommandService/ExtensionKeybindingRegistry) which results in a diamond-like
call graph, we get the DCHECK.

Here are the two stacks:
#1 0x0000039877aa extensions::ExtensionKeybindingRegistry::AddEventTarget()
#2 0x0000010d9f3f ExtensionKeybindingRegistryViews::AddExtensionKeybinding()
#3 0x000003987e94 extensions::ExtensionKeybindingRegistry::OnExtensionLoaded()
#4 0x000003bbf9dc extensions::ExtensionRegistry::TriggerOnLoaded()


#1 0x0000039877aa extensions::ExtensionKeybindingRegistry::AddEventTarget()
#2 0x0000010d9f3f ExtensionKeybindingRegistryViews::AddExtensionKeybinding()
#3 0x0000018f99d9 content::NotificationServiceImpl::Notify()
#4 0x000003921af7 extensions::CommandService::AddKeybindingPref()
#5 0x0000039235c3 extensions::CommandService::AssignKeybindings()
#6 0x000003922856 extensions::CommandService::UpdateKeybindings()
#7 0x000003bbf9dc extensions::ExtensionRegistry::TriggerOnLoaded()

The issue is that both ExtensionKeybindingRegistry and CommandService observe
OnExtensionLoaded. CommandService sends a command added notification, which then
results in ExtensionKeybindingRegistry calling AddKeybinding() again for the
same command.
Seems like a race condition causes this to not reproduce on my local machine.

https://codereview.chromium.org/788973002/diff/650001/chrome/browser/extensio...
chrome/browser/extensions/extension_keybinding_registry.cc:205: return;
On 2015/01/08 14:13:50, Finnur wrote:
> We don't need to early-return when type == COMMAND_REMOVED, do we?

Done. (since we don't process OnExtensionUnloaded in CommandService).

[Finnur, 2015-01-12 10:09:38]

I see.

LGTM

[Finnur, 2015-01-12 10:10:30]

Actually, shouldn't we have a test that exercises this use case?

[David Tseng, 2015-01-13 00:43:41]

I wasn't able to reproduce the issue using a pair of extensions, each
having the same key, both loaded as component extensions, with the same
command, but different shortcuts. I tried running one after another and I
appear to be getting the right accelerator (it updates properly).

However, in the case of ChromeVox, the manifest definitely does not update
properly. ChromeVox reads its manifest from the file thread, so that may be
the difference here. I don't quite see how that would cause us not to get
an install event but get one in the case of RunComponentExtensionTest.
Would you be ok with me checking this in but with a bug to investigate
further?

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

[David Tseng, 2015-01-13 00:57:17]

Actually, I see the discrepancy. ChromeVox uses ComponentLoader::Remove to
unload the extension; ExtensionBrowserTest uses
ExtensionService::UnloadExtension (even for RunComponentExtensionTest).

That may be the root cause of the issue.


On Mon, Jan 12, 2015 at 4:43 PM, David Tseng <dtseng@chromium.org> wrote:

> I wasn't able to reproduce the issue using a pair of extensions, each
> having the same key, both loaded as component extensions, with the same
> command, but different shortcuts. I tried running one after another and I
> appear to be getting the right accelerator (it updates properly).
>
> However, in the case of ChromeVox, the manifest definitely does not update
> properly. ChromeVox reads its manifest from the file thread, so that may be
> the difference here. I don't quite see how that would cause us not to get
> an install event but get one in the case of RunComponentExtensionTest.
> Would you be ok with me checking this in but with a bug to investigate
> further?
>

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

[Finnur, 2015-01-13 11:18:16]

Does that mean you are looking into it still or wanting to check in and
investigate afterwards? (I'm ok with that).

[David Tseng, 2015-01-13 18:38:27]

On Tue, Jan 13, 2015 at 3:18 AM, <finnur@chromium.org> wrote:

> Does that mean you are looking into it still or wanting to check in and
> investigate afterwards? (I'm ok with that).
>
> The latter; I filed:
The latter. I filed

> https://code.google.com/p/chromium/issues/detail?id=448446
>

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

[David Tseng, 2015-01-13 18:39:40]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2015-01-13 18:40:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/788973002/710001

[commit-bot: I haz the power, 2015-01-13 18:40:47]

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

[commit-bot: I haz the power, 2015-01-13 18:40:49]

CLs for remote refs other than refs/pending/heads/master must contain NOTRY=true
in order for the CQ to process them

[David Tseng, 2015-01-13 18:54:39]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2015-01-13 18:55:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/788973002/730001

[commit-bot: I haz the power, 2015-01-13 18:59:44]

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

[commit-bot: I haz the power, 2015-01-13 18:59:45]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[David Tseng, 2015-01-13 19:09:29]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2015-01-13 19:10:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/788973002/750001

[commit-bot: I haz the power, 2015-01-13 19:21:16]

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

[commit-bot: I haz the power, 2015-01-13 19:21:18]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[David Tseng, 2015-01-13 19:30:11]

The CQ bit was checked by dtseng@chromium.org

[commit-bot: I haz the power, 2015-01-13 19:30:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/788973002/770001

[commit-bot: I haz the power, 2015-01-13 20:49:57]

Committed patchset #18 (id:770001)

[commit-bot: I haz the power, 2015-01-13 20:50:53]

Patchset 18 (id:??) landed as
https://crrev.com/7c3129ac745bd5598bde8ce572444f9a7224e4ce
Cr-Commit-Position: refs/heads/master@{#311320}