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

chrome/browser/extensions/extension_keybinding_registry.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/extension_keybinding_registry.h"
 
 #include "base/values.h"
 #include "chrome/browser/extensions/active_tab_permission_granter.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/command.h"
@@ skipping 180 matching lines @@
 
       const Extension* extension = ExtensionRegistry::Get(browser_context_)
                                        ->enabled_extensions()
                                        .GetByID(payload->first);
       // During install and uninstall the extension won't be found. We'll catch
       // those events above, with the LOADED/UNLOADED, so we ignore this event.
       if (!extension)
         return;
 
       if (ExtensionMatchesFilter(extension)) {
+        // Component extensions triggers OnExtensionLoaded for extension
+        // installs as well as loads. This can cause adding of multiple key

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

It seems weird that OnExtensionLoaded is called twice for component extensions.
Is it clear why that happens? Is it perhaps a bug?

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

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.

+        // targets.
+        if (extension->location() == Manifest::COMPONENT)
+          return;

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

We don't need to early-return when type == COMMAND_REMOVED, do we?

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

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

+
         if (type == extensions::NOTIFICATION_EXTENSION_COMMAND_ADDED)
           AddExtensionKeybinding(extension, payload->second);
         else
           RemoveExtensionKeybinding(extension, payload->second);
       }
       break;
     }
     default:
       NOTREACHED();
       break;
@@ skipping 31 matching lines @@
     if (extension_id.empty() || it->first == extension_id) {
       CommandExecuted(it->first, it->second);
       executed = true;
     }
   }
 
   return executed;
 }
 
 }  // namespace extensions