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..8d0a9b31e3910bc02f66c9b246d4a0dc14cc9693 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];
+  // 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);
+  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())) {
+      NOTREACHED() << "Extension " << extension.id()

[karandeepb, 2017/03/24 23:54:12]

We crash in the renderer in this scenario. Is that to prevent against a
compromised renderer? 

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

We crash because it's a logical state that we should never be in, and we're
trying to track down and eliminate the scenarios in which it can happen.  I'd
encourage us to at least DumpWithoutCrashing here to help trace it.

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

Done.

+                   << " was not loaded for activation";
+      return;
+    }
     process->Send(new ExtensionMsg_ActivateExtension(extension.id()));
-  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.
+    if (base::ContainsKey(process_extensions_pair.second, extension.id()))

[karandeepb, 2017/03/24 23:54:12]

This would have been a NOTREACHED in the renderer, but I think this is fine
since these are public functions. We shouldn't assume that clients won't call
these functions in an incorrect manner.

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

Hmm... I'm not sure I agree.  I think it's perfectly reasonable to assume that
callers will only call functions in certain ways - that's why we can, e.g.,
DCHECK() that a pointer argument is non-null.  I think that classes should be
able to document and enforce expectations, including that function A is called
before function B - which is basically what this is doing here.  Especially
when, if it's not the case, it could hide a potential bug.

I agree that there are times when we want to be lenient in the API we expose,
but we should balance that with the ability to create a deterministic state.

+      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());
 }