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..999fce885c70b31cfb7ae14a4d32d404ae104965 100644
--- a/extensions/browser/renderer_startup_helper.cc
+++ b/extensions/browser/renderer_startup_helper.cc
@@ -4,6 +4,7 @@
 
 #include "extensions/browser/renderer_startup_helper.h"
 
+#include "base/stl_util.h"
 #include "base/values.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.h"
 #include "content/public/browser/notification_service.h"
@@ -91,8 +92,9 @@ void RendererStartupHelper::InitializeProcess(
         WebViewGuest::GetPartitionID(process)));
   }
 
-  // Loaded extensions.
-  std::vector<ExtensionMsg_Loaded_Params> loaded_extensions;
+  std::set<ExtensionId>* loaded_extensions = &loaded_extensions_[process];

[Devlin, 2017/03/27 14:47:57]

optional nit: maybe just prefer a reference here?
std::set<ExtensionId>& loaded_extensions = loaded_extensions_[process];

[Devlin, 2017/03/27 14:47:58]

Can we DCHECK(loaded_extensions.empty())?

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

Not relevant anymore.

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

Done.

+  // Load extensions.
+  std::vector<ExtensionMsg_Loaded_Params> loaded_extensions_params;
   const ExtensionSet& extensions =
       ExtensionRegistry::Get(browser_context_)->enabled_extensions();
   for (const auto& ext : extensions) {
@@ -103,11 +105,14 @@ void RendererStartupHelper::InitializeProcess(
       // I am not sure this is possible to know this here, at such a low
       // level of the stack. Perhaps site isolation can help.
       bool include_tab_permissions = true;
-      loaded_extensions.push_back(
+      loaded_extensions_params.push_back(
           ExtensionMsg_Loaded_Params(ext.get(), include_tab_permissions));
+      loaded_extensions->insert(ext->id());
     }
   }
-  process->Send(new ExtensionMsg_Loaded(loaded_extensions));
+
+  // Activate pending extensions.
+  process->Send(new ExtensionMsg_Loaded(loaded_extensions_params));
   auto iter = pending_active_extensions_.find(process);
   if (iter != pending_active_extensions_.end()) {
     for (const ExtensionId& id : iter->second) {
@@ -116,7 +121,7 @@ void RendererStartupHelper::InitializeProcess(
     }
   }
 
-  initialized_processes_.insert(process);
+  pending_active_extensions_.erase(process);
 }
 
 void RendererStartupHelper::UntrackProcess(
@@ -126,7 +131,7 @@ void RendererStartupHelper::UntrackProcess(
     return;
   }
 
-  initialized_processes_.erase(process);
+  loaded_extensions_.erase(process);
   pending_active_extensions_.erase(process);
 }
 
@@ -139,10 +144,18 @@ void RendererStartupHelper::ActivateExtensionInProcess(
   if (extension.is_theme())
     return;
 
-  if (initialized_processes_.count(process))
+  const auto& process_extensions_pair = loaded_extensions_.find(process);

[Devlin, 2017/03/27 14:47:57]

This is an iterator, so having const auto& isn't really right, since the return
result is a value, not a reference (though luckily the C++ ensures this is
safe).  Prefer just
auto iter = loaded_extensions_.find(process);

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

Oh yeah, this takes reference to a temporary.

+  if (process_extensions_pair != loaded_extensions_.end()) {
+    // The extension should have already been loaded in the process.
+    if (!base::ContainsKey(process_extensions_pair->second, extension.id())) {
+      LOG(ERROR) << "Extension " << extension.id()
+                 << " was not loaded for activation";
+      return;
+    }
     process->Send(new ExtensionMsg_ActivateExtension(extension.id()));

[Devlin, 2017/03/27 14:47:57]

Don't we need to add the extension to the set?

if (!iter->second.insert(extension.id()).second) {
  // Handle error
}
process->Send(new ExtensionMsg...);

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

Not sure I understand. It is being activated. The set tracks loaded extensions.

-  else
+  } else {
     pending_active_extensions_[process].insert(extension.id());
+  }
 }
 
 void RendererStartupHelper::OnExtensionLoaded(const Extension& extension) {
@@ -157,17 +170,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 (auto& process_extensions_pair : loaded_extensions_) {
+    // If extension is already loaded in process, do nothing.

[Devlin, 2017/03/27 14:47:57]

Hmm... can this happen, assuming OnExtensionLoaded/Unloaded are called properly?

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

For the incognito case I mentioned, yes. 

+    if (base::ContainsKey(process_extensions_pair.second, extension.id()))
+      continue;
+
+    content::RenderProcessHost* process = process_extensions_pair.first;
     process->Send(new ExtensionMsg_Loaded(params));
+    process_extensions_pair.second.insert(extension.id());
+  }
 }
 
 void RendererStartupHelper::OnExtensionUnloaded(const Extension& extension) {
-  // Renderers don't need to know about themes.
-  if (extension.is_theme())
-    return;
+  for (auto& process_extensions_pair : loaded_extensions_) {
+    // If extension is already unloaded, do nothing.
+    if (!base::ContainsKey(process_extensions_pair.second, extension.id()))
+      continue;
 
-  for (content::RenderProcessHost* process : initialized_processes_)
+    content::RenderProcessHost* process = process_extensions_pair.first;
     process->Send(new ExtensionMsg_Unloaded(extension.id()));
+    process_extensions_pair.second.erase(extension.id());
+  }
+
   for (auto& process_extensions_pair : pending_active_extensions_)
     process_extensions_pair.second.erase(extension.id());
 }