Add an arbitrary delay before unloading lazy background pages.

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 "base/bind.h"
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
+#include "base/message_loop.h"
+#include "base/time.h"
 #include "chrome/browser/extensions/extension_event_router.h"
 #include "chrome/browser/extensions/extension_process_manager.h"
 #include "chrome/browser/extensions/extension_host.h"
 #include "chrome/browser/extensions/extension_info_map.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_window.h"
 #include "chrome/common/chrome_notification_types.h"
@@ skipping 100 matching lines @@
 //
 
 // static
 ExtensionProcessManager* ExtensionProcessManager::Create(Profile* profile) {
   return (profile->IsOffTheRecord()) ?
       new IncognitoExtensionProcessManager(profile) :
       new ExtensionProcessManager(profile);
 }
 
 ExtensionProcessManager::ExtensionProcessManager(Profile* profile)
-    : site_instance_(SiteInstance::Create(profile)) {
+  : site_instance_(SiteInstance::Create(profile)),
+    weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
   Profile* original_profile = profile->GetOriginalProfile();
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSIONS_READY,
                  content::Source<Profile>(original_profile));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_LOADED,
                  content::Source<Profile>(original_profile));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
                  content::Source<Profile>(original_profile));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_DESTROYED,
                  content::Source<Profile>(profile));
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE,
@@ skipping 268 matching lines @@
   return count;
 }
 
 int ExtensionProcessManager::DecrementLazyKeepaliveCount(
      const Extension* extension) {
   if (!extension->has_lazy_background_page())
     return 0;
 
   int& count = background_page_data_[extension->id()].lazy_keepalive_count;
   DCHECK_GT(count, 0);
-  if (--count == 0)
-    OnLazyBackgroundPageIdle(extension->id());
+  if (--count == 0) {
+    VLOG(1) << "delaying post for " << extension->id();
+    MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        base::Bind(&ExtensionProcessManager::OnLazyBackgroundPageIdle,
+                   weak_ptr_factory_.GetWeakPtr(), extension->id(),
+                   ++background_page_data_[extension->id()].close_sequence_id),
+        base::TimeDelta::FromSeconds(10));

[Yoyo Zhou, 2012/04/30 21:34:09]

I'm not sure if this should be a constant defined somewhere (or even a
command-line flag).

[Aaron Boodman, 2012/04/30 21:47:13]

Let's do a command-line flag so that we can experiment with different values.

[Matt Perry, 2012/04/30 21:58:45]

I think we should have 2 timeouts, like I described in the bug:
- first timeout is a soft timeout. if the extension has any activity in the time
period, the timeout is reset. the purpose of this one is to avoid thrashing if
there are many events in sequence.
- once the first timeout expires, we dispatch the unload event, and start a
second timeout. this timeout is a hard timeout, and never resets. when it
expires, we unload the page. the purpose of this one is to give some (but not
infinite) time for the extension to do cleanup-on-unload things.

[Yoyo Zhou, 2012/05/01 22:15:13]

This is the first one then. As for the second case, I thought we were doing
something with asynchronous responses to the unloading event.

Both timeouts are command-line-switchable, although the parsing is ad-hoc since
I couldn't find more examples of ints with default values.

+  }
 
   return count;
 }
 
 void ExtensionProcessManager::OnLazyBackgroundPageIdle(
-    const std::string& extension_id) {
+    const std::string& extension_id, int close_sequence_id) {
+  VLOG(1) << "onlazybgpidle " << extension_id;
   ExtensionHost* host = GetBackgroundHostForExtension(extension_id);
   if (host && !background_page_data_[extension_id].is_closing) {

[Matt Perry, 2012/04/30 21:58:45]

We should check whether close_sequence_id has changed. If it has, then we know
this idle notification is obsolete, and we shouldn't bother sending the message
(since we'd just ignore it the ack).

[Yoyo Zhou, 2012/05/01 22:15:13]

True.

     // Tell the renderer we are about to close. This is a simple ping that the
     // renderer will respond to. The purpose is to control sequencing: if the
     // extension remains idle until the renderer responds with an ACK, then we
     // know that the extension process is ready to shut down.
     host->render_view_host()->Send(new ExtensionMsg_ShouldUnload(
-        extension_id, ++background_page_data_[extension_id].close_sequence_id));
+        extension_id, close_sequence_id));
   }
 }
 
 void ExtensionProcessManager::OnLazyBackgroundPageActive(
     const std::string& extension_id) {
   ExtensionHost* host = GetBackgroundHostForExtension(extension_id);
   if (host && !background_page_data_[extension_id].is_closing) {
     // Cancel the current close sequence by changing the close_sequence_id,
     // which causes us to ignore the next ShouldUnloadAck.
     ++background_page_data_[extension_id].close_sequence_id;
@@ skipping 260 matching lines @@
         if (service && service->is_ready())
           CreateBackgroundHostsForProfileStartup(this, service->extensions());
       }
       break;
     }
     default:
       ExtensionProcessManager::Observe(type, source, details);
       break;
   }
 }