Remove keep-alive impulse IPCs from NaCl modules.

Remove keep-alive impulse IPCs from NaCl modules.

NaCl modules used to dispatch regular IPCs to the browser, to boost the
app/extension background page's keep-alive count for a short while. The
IPCs were triggered by PPAPI calls being initiated by the module.

Issue 472532 introduced a separate mechanism which keeps the background
page alive so long as any NaCl module is loaded, regardless of whether
or not the module is active, rendering the keep-alive impulse IPCs
redundant.

BUG=412621, 331954, 332440, 490440, 346278
Review-Url: https://codereview.chromium.org/2762513002
Cr-Commit-Position: refs/heads/master@{#459689}
Committed: https://chromium.googlesource.com/chromium/src/+/d6be280f1167614a0dcee57708dde086e5ab0708

[Wez, 2017-03-19 03:00:59]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-19 03:01:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-19 03:55:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-19 03:55:06]

Dry run: This issue passed the CQ dry run.

[Wez, 2017-03-19 19:56:04]

wez@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Wez, 2017-03-19 19:56:04]

PTAL - removes the keepalive impulse from PPAPI calls.

Will add OWNERS for the no-extension bits separately.

[Devlin, 2017-03-21 01:08:15]

Nice!  Love seeing that much red. :)

A few nits, and I wanna take another look to make sure there's nothing else we
need to clean up, but I don't foresee any large changes necessary if you wanted
to add other folks.

Thanks for doing this!

https://codereview.chromium.org/2762513002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://codereview.chromium.org/2762513002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/app_background_page_apitest.cc:544: #if
!defined(DISABLE_NACL)
nit: not directly related to this patch, but can we instead do this like:
#if defined(DISABLE_NACL)
#define NaCl_BackgroundKeepaliveActive DISABLED_BackgroundKeepaliveNacl
#else
#define NaCl_BackgroundKeepaliveActive BackgroundKeepaliveNacl
#endif

or similar?  It doesn't look like there's anything that needs to be compiled out
in the test.

https://codereview.chromium.org/2762513002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/app_background_page_apitest.cc:551: // Keepalive count
set for chrome.test.sendMessage only.
Maybe expand on this:
// Since |ready_listener| hasn't replied, the extension API hasn't called the
callback and completed the API invocation.

https://codereview.chromium.org/2762513002/diff/1/extensions/browser/process_...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/2762513002/diff/1/extensions/browser/process_...
extensions/browser/process_manager.cc:568: CHECK_GT(idle_time_msec, 0u);  //
OnKeepaliveImpulseCheck requires non zero.
OnKeepaliveImpulseCheck() no longer exists

https://codereview.chromium.org/2762513002/diff/1/ppapi/tests/extensions/back...
File ppapi/tests/extensions/background_keepalive/background.js (right):

https://codereview.chromium.org/2762513002/diff/1/ppapi/tests/extensions/back...
ppapi/tests/extensions/background_keepalive/background.js:9:
chrome.test.sendMessage("created_module:" + moduleNumber.toString(),
nit: pref single quotes in js (looks like we were inconsistent before; now's a
good time to fix settle on one. :))

https://codereview.chromium.org/2762513002/diff/1/ppapi/tests/extensions/back...
ppapi/tests/extensions/background_keepalive/background.js:27: function
createNaClEmbed(touch_embed_hack) {
nit: prefer camelCaseVariableNames in JS

https://codereview.chromium.org/2762513002/diff/1/ppapi/tests/extensions/back...
ppapi/tests/extensions/background_keepalive/background.js:34: var listener =
document.createElement("div");
nit: do we need to wrap in a div?

https://codereview.chromium.org/2762513002/diff/1/ppapi/tests/extensions/back...
ppapi/tests/extensions/background_keepalive/background.js:36: true);
nit: do we need to use capture?

[Wez, 2017-03-21 23:50:53]

The CQ bit was checked by wez@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-21 23:51:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Wez, 2017-03-21 23:59:21]

wez@chromium.org changed reviewers:
+ bbudge@chromium.org, mseaborn@chromium.org, nasko@chromium.org

[Wez, 2017-03-21 23:59:23]

mseaborn: components/nacl & chrome/browser/nacl_host
bbudge: ppapi/ & content/browser/renderer_host
nasko: ppapi/proxy/ppapi_messages.h

zelidrag: FYI; this is removing the notion of active vs idle PPAPI/NaCl modules,
committing us to relying on NaCl modules incrementing the lazy page keep-alive
count for their entire lifetime. Un-doing the lifelong keepalive bump and fixing
this keepalive "impulse" to work for long-lived API calls doesn't seem worth the
complexity, given we've gone ~2 years with the current behaviour.

https://codereview.chromium.org/2762513002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/app_background_page_apitest.cc (right):

https://codereview.chromium.org/2762513002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/app_background_page_apitest.cc:544: #if
!defined(DISABLE_NACL)
On 2017/03/21 01:08:15, Devlin wrote:
> nit: not directly related to this patch, but can we instead do this like:
> #if defined(DISABLE_NACL)
> #define NaCl_BackgroundKeepaliveActive DISABLED_BackgroundKeepaliveNacl
> #else
> #define NaCl_BackgroundKeepaliveActive BackgroundKeepaliveNacl
> #endif
> 
> or similar?  It doesn't look like there's anything that needs to be compiled
out
> in the test.

Is there any reason to even compile the test in DISABLE_NACL builds? i.e. should
we just move the #if ... #endif to enclose the whole test fixture?

https://codereview.chromium.org/2762513002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/app_background_page_apitest.cc:551: // Keepalive count
set for chrome.test.sendMessage only.
On 2017/03/21 01:08:15, Devlin wrote:
> Maybe expand on this:
> // Since |ready_listener| hasn't replied, the extension API hasn't called the
> callback and completed the API invocation.

Done.

https://codereview.chromium.org/2762513002/diff/1/extensions/browser/process_...
File extensions/browser/process_manager.cc (right):

https://codereview.chromium.org/2762513002/diff/1/extensions/browser/process_...
extensions/browser/process_manager.cc:568: CHECK_GT(idle_time_msec, 0u);  //
OnKeepaliveImpulseCheck requires non zero.
On 2017/03/21 01:08:15, Devlin wrote:
> OnKeepaliveImpulseCheck() no longer exists

Done.

https://codereview.chromium.org/2762513002/diff/1/ppapi/tests/extensions/back...
File ppapi/tests/extensions/background_keepalive/background.js (right):

https://codereview.chromium.org/2762513002/diff/1/ppapi/tests/extensions/back...
ppapi/tests/extensions/background_keepalive/background.js:9:
chrome.test.sendMessage("created_module:" + moduleNumber.toString(),
On 2017/03/21 01:08:15, Devlin wrote:
> nit: pref single quotes in js (looks like we were inconsistent before; now's a
> good time to fix settle on one. :))

Done.

https://codereview.chromium.org/2762513002/diff/1/ppapi/tests/extensions/back...
ppapi/tests/extensions/background_keepalive/background.js:27: function
createNaClEmbed(touch_embed_hack) {
On 2017/03/21 01:08:15, Devlin wrote:
> nit: prefer camelCaseVariableNames in JS

Done.

https://codereview.chromium.org/2762513002/diff/1/ppapi/tests/extensions/back...
ppapi/tests/extensions/background_keepalive/background.js:34: var listener =
document.createElement("div");
On 2017/03/21 01:08:15, Devlin wrote:
> nit: do we need to wrap in a div?

This is how the code was originally, and removing the div breaks the test; I
have vague recollections of some subtlety around event dispatch for plugins, but
I'm not sure what it is that breaks w/out the div.

https://codereview.chromium.org/2762513002/diff/1/ppapi/tests/extensions/back...
ppapi/tests/extensions/background_keepalive/background.js:36: true);
On 2017/03/21 01:08:15, Devlin wrote:
> nit: do we need to use capture?

Yes, this is capturing modulesCreated as the first parameter to moduleDidLoad so
that we can create multiple modules and get discrete did-load notifications for
each.  I could just use the global modulesCreated number in the notifications
but this seems cleaner.

[bbudge, 2017-03-22 00:50:16]

content/browser/renderer_host/pepper and ppapi LGTM

[commit-bot: I haz the power, 2017-03-22 01:53:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-22 01:53:12]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT,
build hasn't started yet, builder probably lacks capacity)

[Devlin, 2017-03-23 01:30:06]

extensions lgtm

[Wez, 2017-03-23 22:23:16]

wez@chromium.org changed reviewers:
+ jochen@chromium.org, palmer@chromium.org
- mseaborn@chromium.org, nasko@chromium.org

[Wez, 2017-03-23 22:23:16]

s/nasko/palmer: *messages.h
s/mseaborn/jochen: NaCl stuffs.

[palmer, 2017-03-23 22:55:51]

lgtm

[Mark Seaborn, 2017-03-24 01:49:56]

On 2017/03/21 23:59:23, Wez wrote:
> mseaborn: components/nacl & chrome/browser/nacl_host

LGTM for components/nacl & chrome/browser/nacl_host

[jochen (gone - plz use gerrit), 2017-03-24 09:35:12]

jochen@chromium.org changed reviewers:
+ mseaborn@chromium.org

[jochen (gone - plz use gerrit), 2017-03-24 09:35:12]

lgtm

[Wez, 2017-03-24 23:15:04]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2017-03-24 23:15:32]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-25 00:16:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-25 00:16:22]

Failed to apply patch for extensions/browser/process_manager.cc:
While running git apply --index -p1;
  error: patch failed: extensions/browser/process_manager.cc:263
  error: extensions/browser/process_manager.cc: patch does not apply

Patch:       extensions/browser/process_manager.cc
Index: extensions/browser/process_manager.cc
diff --git a/extensions/browser/process_manager.cc
b/extensions/browser/process_manager.cc
index
dc3b5e71266cc0a0b0c4ed56d36d244f6ed572b3..a7e774f65534642a6b0ca9e7f56372d5f7ab5659
100644
--- a/extensions/browser/process_manager.cc
+++ b/extensions/browser/process_manager.cc
@@ -48,8 +48,6 @@ namespace {
 
 // The time to delay between an extension becoming idle and
 // sending a ShouldSuspend message.
-// Note: Must be sufficiently larger (e.g. 2x) than
-// kKeepaliveThrottleIntervalInSeconds in ppapi/proxy/plugin_globals.
 unsigned g_event_page_idle_time_msec = 10000;
 
 // The time to delay between sending a ShouldSuspend message and
@@ -121,46 +119,32 @@ void PropagateExtensionWakeResult(const
base::Callback<void(bool)>& callback,
 
 struct ProcessManager::BackgroundPageData {
   // The count of things keeping the lazy background page alive.
-  int lazy_keepalive_count;
-
-  // Tracks if an impulse event has occured since the last polling check.
-  bool keepalive_impulse;
-  bool previous_keepalive_impulse;
+  int lazy_keepalive_count = 0;
 
   // True if the page responded to the ShouldSuspend message and is currently
   // dispatching the suspend event. During this time any events that arrive
will
   // cancel the suspend process and an onSuspendCanceled event will be
   // dispatched to the page.
-  bool is_closing;
+  bool is_closing = false;
 
   // Stores the value of the incremented
   // ProcessManager::last_background_close_sequence_id_ whenever the extension
   // is active. A copy of the ID is also passed in the callbacks and IPC
   // messages leading up to CloseLazyBackgroundPageNow. The process is aborted
   // if the IDs ever differ due to new activity.
-  uint64_t close_sequence_id;
+  uint64_t close_sequence_id = 0ull;
 
   // Keeps track of when this page was last suspended. Used for perf metrics.
   std::unique_ptr<base::ElapsedTimer> since_suspended;
-
-  BackgroundPageData()
-      : lazy_keepalive_count(0),
-        keepalive_impulse(false),
-        previous_keepalive_impulse(false),
-        is_closing(false),
-        close_sequence_id(0) {}
 };
 
 // Data of a RenderFrameHost associated with an extension.
 struct ProcessManager::ExtensionRenderFrameData {
   // The type of the view.
-  extensions::ViewType view_type;
+  extensions::ViewType view_type = VIEW_TYPE_INVALID;
 
   // Whether the view is keeping the lazy background page alive or not.
-  bool has_keepalive;
-
-  ExtensionRenderFrameData()
-      : view_type(VIEW_TYPE_INVALID), has_keepalive(false) {}
+  bool has_keepalive = false;
 
   // Returns whether the view can keep the lazy background page alive or not.
   bool CanKeepalive() const {
@@ -263,8 +247,6 @@ ProcessManager::ProcessManager(BrowserContext* context,
                  extensions::NOTIFICATION_EXTENSION_HOST_VIEW_SHOULD_CLOSE,
                  content::Source<BrowserContext>(context));
   content::DevToolsAgentHost::AddObserver(this);
-
-  OnKeepaliveImpulseCheck();
 }
 
 ProcessManager::~ProcessManager() {
@@ -490,52 +472,6 @@ void ProcessManager::DecrementLazyKeepaliveCount(const
Extension* extension) {
     DecrementLazyKeepaliveCount(extension->id());
 }
 
-// This implementation layers on top of the keepalive count. An impulse sets
-// a per extension flag. On a regular interval that flag is checked. Changes
-// from the flag not being set to set cause an IncrementLazyKeepaliveCount.
-void ProcessManager::KeepaliveImpulse(const Extension* extension) {
-  if (!BackgroundInfo::HasLazyBackgroundPage(extension))
-    return;
-
-  BackgroundPageData& bd = background_page_data_[extension->id()];
-
-  if (!bd.keepalive_impulse) {
-    bd.keepalive_impulse = true;
-    if (!bd.previous_keepalive_impulse) {
-      IncrementLazyKeepaliveCount(extension);
-    }
-  }
-
-  if (!keepalive_impulse_callback_for_testing_.is_null()) {
-    ImpulseCallbackForTesting callback_may_clear_callbacks_reentrantly =
-      keepalive_impulse_callback_for_testing_;
-    callback_may_clear_callbacks_reentrantly.Run(extension->id());
-  }
-}
-
-// static
-void ProcessManager::OnKeepaliveFromPlugin(int render_process_id,
-                                           int render_frame_id,
-                                           const std::string& extension_id) {
-  content::RenderFrameHost* render_frame_host =
-      content::RenderFrameHost::FromID(render_process_id, render_frame_id);
-  if (!render_frame_host)
-    return;
-
-  content::SiteInstance* site_instance = render_frame_host->GetSiteInstance();
-  if (!site_instance)
-    return;
-
-  BrowserContext* browser_context = site_instance->GetBrowserContext();
-  const Extension* extension =
-      ExtensionRegistry::Get(browser_context)->enabled_extensions().GetByID(
-          extension_id);
-  if (!extension)
-    return;
-
-  ProcessManager::Get(browser_context)->KeepaliveImpulse(extension);
-}
-
 void ProcessManager::OnShouldSuspendAck(const std::string& extension_id,
                                         uint64_t sequence_id) {
   ExtensionHost* host = GetBackgroundHostForExtension(extension_id);
@@ -625,19 +561,9 @@ void ProcessManager::CloseBackgroundHosts() {
   DCHECK(background_hosts_.empty());
 }
 
-void ProcessManager::SetKeepaliveImpulseCallbackForTesting(
-    const ImpulseCallbackForTesting& callback) {
-  keepalive_impulse_callback_for_testing_ = callback;
-}
-
-void ProcessManager::SetKeepaliveImpulseDecrementCallbackForTesting(
-    const ImpulseCallbackForTesting& callback) {
-  keepalive_impulse_decrement_callback_for_testing_ = callback;
-}
-
 // static
 void ProcessManager::SetEventPageIdleTimeForTesting(unsigned idle_time_msec) {
-  CHECK_GT(idle_time_msec, 0u);  // OnKeepaliveImpulseCheck requires non zero.
+  CHECK_GT(idle_time_msec, 0u);
   g_event_page_idle_time_msec = idle_time_msec;
 }
 
@@ -795,41 +721,6 @@ void ProcessManager::DecrementLazyKeepaliveCount(
   }
 }
 
-// DecrementLazyKeepaliveCount is called when no calls to KeepaliveImpulse
-// have been made for at least g_event_page_idle_time_msec. In the best case an
-// impulse was made just before being cleared, and the decrement will occur
-// g_event_page_idle_time_msec later, causing a 2 * g_event_page_idle_time_msec
-// total time for extension to be shut down based on impulses. Worst case is
-// an impulse just after a clear, adding one check cycle and resulting in 3x
-// total time.
-void ProcessManager::OnKeepaliveImpulseCheck() {
-  for (BackgroundPageDataMap::iterator i = background_page_data_.begin();
-       i != background_page_data_.end();
-       ++i) {
-    if (i->second.previous_keepalive_impulse && !i->second.keepalive_impulse) {
-      DecrementLazyKeepaliveCount(i->first);
-      if (!keepalive_impulse_decrement_callback_for_testing_.is_null()) {
-        ImpulseCallbackForTesting callback_may_clear_callbacks_reentrantly =
-            keepalive_impulse_decrement_callback_for_testing_;
-        callback_may_clear_callbacks_reentrantly.Run(i->first);
-      }
-    }
-
-    i->second.previous_keepalive_impulse = i->second.keepalive_impulse;
-    i->second.keepalive_impulse = false;
-  }
-
-  // OnKeepaliveImpulseCheck() is always called in constructor, but in unit
-  // tests there will be no thread task runner handle. In that event don't
-  // schedule tasks.
-  if (base::ThreadTaskRunnerHandle::IsSet()) {
-    base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
-        FROM_HERE, base::Bind(&ProcessManager::OnKeepaliveImpulseCheck,
-                              weak_ptr_factory_.GetWeakPtr()),
-        base::TimeDelta::FromMilliseconds(g_event_page_idle_time_msec));
-  }
-}
-
 void ProcessManager::OnLazyBackgroundPageIdle(const std::string& extension_id,
                                               uint64_t sequence_id) {
   ExtensionHost* host = GetBackgroundHostForExtension(extension_id);

[Wez, 2017-03-27 00:25:22]

The CQ bit was checked by wez@chromium.org

[Wez, 2017-03-27 00:25:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org, palmer@chromium.org, bbudge@chromium.org,
jochen@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2762513002/#ps40001
(title: "Rebase")

[commit-bot: I haz the power, 2017-03-27 00:25:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-27 01:49:35]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490574322966790,
"parent_rev": "d364139fee76154a1d9fa8875ad0cbb5ccb523c3", "commit_rev":
"d6be280f1167614a0dcee57708dde086e5ab0708"}

[commit-bot: I haz the power, 2017-03-27 01:50:24]

Description was changed from

==========
Remove keep-alive impulse IPCs from NaCl modules.

NaCl modules used to dispatch regular IPCs to the browser, to boost the
app/extension background page's keep-alive count for a short while. The
IPCs were triggered by PPAPI calls being initiated by the module.

Issue 472532 introduced a separate mechanism which keeps the background
page alive so long as any NaCl module is loaded, regardless of whether
or not the module is active, rendering the keep-alive impulse IPCs
redundant.

BUG=412621,331954,332440,490440,346278
==========

to

==========
Remove keep-alive impulse IPCs from NaCl modules.

NaCl modules used to dispatch regular IPCs to the browser, to boost the
app/extension background page's keep-alive count for a short while. The
IPCs were triggered by PPAPI calls being initiated by the module.

Issue 472532 introduced a separate mechanism which keeps the background
page alive so long as any NaCl module is loaded, regardless of whether
or not the module is active, rendering the keep-alive impulse IPCs
redundant.

BUG=412621,331954,332440,490440,346278

Review-Url: https://codereview.chromium.org/2762513002
Cr-Commit-Position: refs/heads/master@{#459689}
Committed:
https://chromium.googlesource.com/chromium/src/+/d6be280f1167614a0dcee57708dd...
==========

[commit-bot: I haz the power, 2017-03-27 01:50:26]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d6be280f1167614a0dcee57708dd...