Extensions: Keep track of loaded extensions in RendererStartupHelper.

extensions/browser/renderer_startup_helper.cc

 // Copyright 2014 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 "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"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "extensions/browser/extension_function_dispatcher.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/browser/guest_view/web_view/web_view_guest.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/common/extension_set.h"
 #include "extensions/common/extensions_client.h"
 #include "extensions/common/features/feature_channel.h"
 #include "extensions/common/features/feature_session_type.h"
 #include "ui/base/webui/web_ui_util.h"
 
 using content::BrowserContext;
 
 namespace extensions {
 
 RendererStartupHelper::RendererStartupHelper(BrowserContext* browser_context)
     : browser_context_(browser_context) {
   DCHECK(browser_context);
   registrar_.Add(this, content::NOTIFICATION_RENDERER_PROCESS_CREATED,
                  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() {}
 
 void RendererStartupHelper::Observe(
     int type,
     const content::NotificationSource& source,
     const content::NotificationDetails& details) {
   switch (type) {
     case content::NOTIFICATION_RENDERER_PROCESS_CREATED:
       InitializeProcess(
           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:
       NOTREACHED() << "Unexpected notification: " << type;
   }
 }
 
 void RendererStartupHelper::InitializeProcess(
     content::RenderProcessHost* process) {
   ExtensionsBrowserClient* client = ExtensionsBrowserClient::Get();
@@ skipping 31 matching lines @@
   if (!webview_partition_id.empty()) {
     process->Send(new ExtensionMsg_SetWebViewPartitionID(
         WebViewGuest::GetPartitionID(process)));
   }
 
   // Loaded extensions.
   std::vector<ExtensionMsg_Loaded_Params> loaded_extensions;
   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
       // processes, no other process needs it, so it's mildly wasteful.
       // 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(
           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(
     content::RenderProcessHost* process) {
   if (!ExtensionsBrowserClient::Get()->IsSameContext(
           browser_context_, process->GetBrowserContext())) {
     return;
   }
 
   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;
 
   // We don't need to include tab permisisons here, since the extension
   // was just loaded.
   // Uninitialized renderers will be informed of the extension load during the
   // first batch of messages.
   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());
 }
 
 //////////////////////////////////////////////////////////////////////////////
 
 // static
 RendererStartupHelper* RendererStartupHelperFactory::GetForBrowserContext(
     BrowserContext* context) {
   return static_cast<RendererStartupHelper*>(
       GetInstance()->GetServiceForBrowserContext(context, true));
 }
@@ skipping 21 matching lines @@
     BrowserContext* context) const {
   // Redirected in incognito.
   return ExtensionsBrowserClient::Get()->GetOriginalContext(context);
 }
 
 bool RendererStartupHelperFactory::ServiceIsCreatedWithBrowserContext() const {
   return true;
 }
 
 }  // namespace extensions