Fixed crash when chrome.runtime.reload() is called with app windows open

chrome/browser/extensions/extension_process_manager.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_process_manager.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
@@ skipping 377 matching lines @@
   WebContents* web_contents = WebContents::FromRenderViewHost(render_view_host);
   all_extension_views_[render_view_host] =
       extensions::GetViewType(web_contents);
 
   // Keep the lazy background page alive as long as any non-background-page
   // extension views are visible. Keepalive count balanced in
   // UnregisterRenderViewHost.
   IncrementLazyKeepaliveCountForView(render_view_host);
 }
 
+void ExtensionProcessManager::ReregisterRenderViewHosts(
+    const std::string& extension_id) {
+  // Re-register all RenderViews for this extension. We do this to restore
+  // the lazy_keepalive_count (if any) to properly reflect the number of open
+  // views.
+  for (ExtensionRenderViews::const_iterator it = all_extension_views_.begin();
+       it != all_extension_views_.end(); ++it) {
+    if (GetExtensionID(it->first) == extension_id)
+      IncrementLazyKeepaliveCountForView(it->first);
+  }
+}
+
 SiteInstance* ExtensionProcessManager::GetSiteInstanceForURL(const GURL& url) {
   return site_instance_->GetRelatedSiteInstance(url);
 }
 
 bool ExtensionProcessManager::IsBackgroundHostClosing(
     const std::string& extension_id) {
   ExtensionHost* host = GetBackgroundHostForExtension(extension_id);
   return (host && background_page_data_[extension_id].is_closing);
 }
 
@@ skipping 176 matching lines @@
     }
 
     case chrome::NOTIFICATION_EXTENSION_LOADED: {
       Profile* profile = content::Source<Profile>(source).ptr();
       ExtensionService* service =
           extensions::ExtensionSystem::Get(profile)->extension_service();
       if (service->is_ready()) {
         const Extension* extension =
             content::Details<const Extension>(details).ptr();
         CreateBackgroundHostForExtensionLoad(this, extension);
+
+        if (extension->is_platform_app() &&
+            service->IsBeingReloaded(extension->id()))
+          ReregisterRenderViewHosts(extension->id());

[benwells, 2013/09/20 07:35:31]

This makes me uncomfortable. The render view hosts will be destroyed very soon
which could make the lazy count go to zero and get unloaded. But worse than
that, they are really for a different extension which is now dead.

Can we instead clear all_extension_views_ when the extension is unloaded, or
will that have other side effects?

Unloading / reloading has a few flaky problems which need to be fixed. E.g.
https://code.google.com/p/chromium/issues/detail?id=174250

[tmdiep, 2013/09/20 11:07:36]

Incrementing the render view hosts actually prevents the lazy count going to
zero.  When the extension loading begins, the count is incremented (and later
decremented when the loading is complete). Without incrementing to account for
the "old" render views, they will decrement the count to zero when they are
deregistered and trigger the timeout for suspending the extension.  It doesn't
unload because an increment comes in soon due to some other network-related
activity and therefore cancels the suspension.
This sounds ok, but I'm not confident enough to say what side effects this will
have. We do need to fix this count somehow though.
I noticed.  I got the impression that the ExtensionService::ReloadExtension()
function is heavy-handed - it unloads, destroys everything and initiates
reloading synchronously.  Meanwhile there is a lot of code still lurking around
either with a dangling extension pointer or null pointer (and see a lot of usage
of extension objects which don't check for null).  

Just feels like the whole process needs to be analysed and sequenced properly,
rather than applying sporadic bandaids whenever we encounter a crash. My 2c :)

[benwells, 2013/09/20 11:22:47]

That was the conclusion I came to today as well. Let's talk about it on Monday.

       }
       break;
     }
 
     case chrome::NOTIFICATION_EXTENSION_UNLOADED: {
       const Extension* extension =
           content::Details<extensions::UnloadedExtensionInfo>(
               details)->extension;
       for (ExtensionHostSet::iterator iter = background_hosts_.begin();
            iter != background_hosts_.end(); ++iter) {
@@ skipping 165 matching lines @@
   for (ExtensionHostSet::iterator iter = background_hosts_.begin();
        iter != background_hosts_.end(); ) {
     ExtensionHostSet::iterator current = iter++;
     delete *current;
   }
 }
 
 void ExtensionProcessManager::ClearBackgroundPageData(
     const std::string& extension_id) {
   background_page_data_.erase(extension_id);
-
-  // Re-register all RenderViews for this extension. We do this to restore
-  // the lazy_keepalive_count (if any) to properly reflect the number of open
-  // views.
-  for (ExtensionRenderViews::const_iterator it = all_extension_views_.begin();
-       it != all_extension_views_.end(); ++it) {
-    if (GetExtensionID(it->first) == extension_id)
-      IncrementLazyKeepaliveCountForView(it->first);
-  }
+  ReregisterRenderViewHosts(extension_id);
 }
 
 bool ExtensionProcessManager::DeferLoadingBackgroundHosts() const {
   // Don't load background hosts now if the loading should be deferred.
   if (defer_background_host_creation_)
     return true;
 
   // The profile may not be valid yet if it is still being initialized.
   // In that case, defer loading, since it depends on an initialized profile.
   // http://crbug.com/222473
@@ skipping 111 matching lines @@
         if (service && service->is_ready())
           CreateBackgroundHostsForProfileStartup();
       }
       break;
     }
     default:
       ExtensionProcessManager::Observe(type, source, details);
       break;
   }
 }