Extensions: Keep track of loaded extensions in RendererStartupHelper.

extensions/browser/renderer_startup_helper.cc

Index: extensions/browser/renderer_startup_helper.cc
diff --git a/extensions/browser/renderer_startup_helper.cc b/extensions/browser/renderer_startup_helper.cc
index c200c27a2dd843ec4e6fa83d24ef74f875f9c21a..bd2d0fb2360e634fd85a41c8dfb3c67ef715ce82 100644
--- a/extensions/browser/renderer_startup_helper.cc
+++ b/extensions/browser/renderer_startup_helper.cc
@@ -4,6 +4,9 @@
 
 #include "extensions/browser/renderer_startup_helper.h"
 
+#include "base/debug/alias.h"
+#include "base/debug/dump_without_crashing.h"
+#include "base/strings/string_util.h"
 #include "base/values.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "content/public/browser/notification_service.h"
@@ -31,6 +34,8 @@ RendererStartupHelper::RendererStartupHelper(BrowserContext* browser_context)
                  content::NotificationService::AllBrowserContextsAndSources());
   registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_TERMINATED,
                  content::NotificationService::AllBrowserContextsAndSources());
+  registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CLOSED,
+                 content::NotificationService::AllBrowserContextsAndSources());
 }
 
 RendererStartupHelper::~RendererStartupHelper() {}
@@ -45,6 +50,8 @@ void RendererStartupHelper::Observe(
           content::Source<content::RenderProcessHost>(source).ptr());
       break;
     case content::NOTIFICATION_RENDERER_PROCESS_TERMINATED:
+    // Fall through.
+    case content::NOTIFICATION_RENDERER_PROCESS_CLOSED:

[karandeepb, 2017/04/04 00:49:42]

So there were cases where InitializeProcess was being called twice for a
RenderProcessHost. I found that this occurred when the RenderProcessHost was
reused for a different render process. (See test failures from PS4->PS5).
Observing NOTIFICATION_RENDERER_PROCESS_CLOSED takes care of this case.

[Devlin, 2017/04/04 15:25:00]

Wild!  Good find.  We should add a comment for this.

[karandeepb, 2017/04/04 18:22:25]

Done.

       UntrackProcess(content::Source<content::RenderProcessHost>(source).ptr());
       break;
     default:
@@ -96,6 +103,10 @@ void RendererStartupHelper::InitializeProcess(
   const ExtensionSet& extensions =
       ExtensionRegistry::Get(browser_context_)->enabled_extensions();
   for (const auto& ext : extensions) {
+    // OnLoadedExtension should have already been called for the extension.

[karandeepb, 2017/04/04 00:49:42]

This was not true for extensions loaded through ShellExtensionSystem, hence the
change for it.

+    DCHECK(base::ContainsKey(extension_process_map_, ext->id()));
+    DCHECK(!base::ContainsKey(extension_process_map_[ext->id()], process));
+
     // Renderers don't need to know about themes.
     if (!ext->is_theme()) {
       // TODO(kalman): Only include tab specific permissions for extension
@@ -105,18 +116,23 @@ void RendererStartupHelper::InitializeProcess(
       bool include_tab_permissions = true;
       loaded_extensions.push_back(
           ExtensionMsg_Loaded_Params(ext.get(), include_tab_permissions));
+      extension_process_map_[ext->id()].insert(process);
     }
   }
   process->Send(new ExtensionMsg_Loaded(loaded_extensions));
   auto iter = pending_active_extensions_.find(process);
   if (iter != pending_active_extensions_.end()) {
     for (const ExtensionId& id : iter->second) {
+      // The extension should be loaded in the process.
       DCHECK(extensions.Contains(id));
+      DCHECK(base::ContainsKey(extension_process_map_, id));
+      DCHECK(base::ContainsKey(extension_process_map_[id], process));
       process->Send(new ExtensionMsg_ActivateExtension(id));
     }
   }
 
   initialized_processes_.insert(process);
+  pending_active_extensions_.erase(process);
 }
 
 void RendererStartupHelper::UntrackProcess(
@@ -128,24 +144,52 @@ void RendererStartupHelper::UntrackProcess(
 
   initialized_processes_.erase(process);
   pending_active_extensions_.erase(process);
+  for (auto& extension_process_pair : extension_process_map_)
+    extension_process_pair.second.erase(process);
 }
 
 void RendererStartupHelper::ActivateExtensionInProcess(
     const Extension& extension,
     content::RenderProcessHost* process) {
+  // The extension should have been loaded already. Dump without crashing to
+  // debug crbug.com/528026.
+  if (!base::ContainsKey(extension_process_map_, extension.id())) {
+#if DCHECK_IS_ON()
+    NOTREACHED() << "Extension " << extension.id()
+                 << "activated before loading";
+#else
+    char extension_id_copy[33];

[Devlin, 2017/04/04 15:25:00]

Do you think the extension id help us debug the crash?  If so, then this is
fine, but otherwise we should avoid collecting it.

[karandeepb, 2017/04/04 18:22:25]

Removed.

+    base::strlcpy(extension_id_copy, extension.id().c_str(),
+                  arraysize(extension_id_copy));
+    base::debug::Alias(extension_id_copy);
+    base::debug::DumpWithoutCrashing();
+    return;
+#endif
+  }
+
   // Renderers don't need to know about themes. We also don't normally
   // "activate" themes, but this could happen if someone tries to open a tab
   // to the e.g. theme's manifest.
   if (extension.is_theme())
     return;
 
-  if (initialized_processes_.count(process))
+  if (base::ContainsKey(initialized_processes_, process)) {
+    DCHECK(base::ContainsKey(extension_process_map_[extension.id()], process));
     process->Send(new ExtensionMsg_ActivateExtension(extension.id()));
-  else
+  } else {
     pending_active_extensions_[process].insert(extension.id());
+  }
 }
 
 void RendererStartupHelper::OnExtensionLoaded(const Extension& extension) {
+  // Extension was already loaded.
+  if (base::ContainsKey(extension_process_map_, extension.id()))
+    return;
+
+  // Mark the extension as loaded.
+  std::set<content::RenderProcessHost*>& loaded_process_set =
+      extension_process_map_[extension.id()];
+
   // Renderers don't need to know about themes.
   if (extension.is_theme())
     return;
@@ -157,19 +201,29 @@ void RendererStartupHelper::OnExtensionLoaded(const Extension& extension) {
   std::vector<ExtensionMsg_Loaded_Params> params(
       1,
       ExtensionMsg_Loaded_Params(&extension, false /* no tab permissions */));
-  for (content::RenderProcessHost* process : initialized_processes_)
+  for (content::RenderProcessHost* process : initialized_processes_) {
     process->Send(new ExtensionMsg_Loaded(params));
+    loaded_process_set.insert(process);
+  }
 }
 
 void RendererStartupHelper::OnExtensionUnloaded(const Extension& extension) {
-  // Renderers don't need to know about themes.
-  if (extension.is_theme())
+  // Extension is not loaded.
+  if (!base::ContainsKey(extension_process_map_, extension.id()))

[karandeepb, 2017/04/04 00:49:42]

I was DCHECKing this (and the corresponding check in OnExtensionLoaded) earlier,
but this caused some tests to fail. The code in ExtensionService doesn't always
ensure that NotifyExtensionUnloaded is not called for an already unloaded
extension. E.g., See ExtensionService::BlockAllExtensions
(https://cs.chromium.org/chromium/src/chrome/browser/extensions/extension_serv...)
which'll call UnloadExtension for a disabled extension(which is already
unloaded). There are also some other cases I think. If you think this is a bug,
I can handle this in a subsequent CL.

[Devlin, 2017/04/04 15:25:00]

I think this should be fixed.  Mind filing a bug or a todo?

[karandeepb, 2017/04/04 18:22:25]

Done.

     return;
 
-  for (content::RenderProcessHost* process : initialized_processes_)
+  const std::set<content::RenderProcessHost*>& loaded_process_set =
+      extension_process_map_[extension.id()];
+  for (content::RenderProcessHost* process : loaded_process_set) {
+    DCHECK(base::ContainsKey(initialized_processes_, process));
     process->Send(new ExtensionMsg_Unloaded(extension.id()));
+  }
+
   for (auto& process_extensions_pair : pending_active_extensions_)
     process_extensions_pair.second.erase(extension.id());
+
+  // Mark the extension as unloaded.
+  extension_process_map_.erase(extension.id());
 }
 
 //////////////////////////////////////////////////////////////////////////////