Fix bug causing blank pages on resume of Chrome for Android.

content/browser/android/content_view_statics.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 <jni.h>
 #include <vector>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/basictypes.h"
 #include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "content/browser/android/content_view_statics.h"
 #include "content/common/android/address_parser.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_process_host_observer.h"
 #include "jni/ContentViewStatics_jni.h"
 
 using base::android::ConvertJavaStringToUTF16;
 using base::android::ConvertUTF16ToJavaString;
 
 namespace {
 
 // TODO(pliard): http://crbug.com/235909. Move WebKit shared timer toggling
 // functionality out of ContentViewStatistics and not be build on top of
 // blink::Platform::SuspendSharedTimer.
 // TODO(pliard): http://crbug.com/235912. Add unit tests for WebKit shared timer
 // toggling.
 
 // This tracks the renderer processes that received a suspend request. It's
 // important on resume to only resume the renderer processes that were actually
 // suspended as opposed to all the current renderer processes because the
 // suspend calls are refcounted within WebKitPlatformSupport and it expects a
 // perfectly matched number of resume calls.
 // Note that this vector is only accessed from the UI thread.
 base::LazyInstance<std::vector<int /* process id */> > g_suspended_processes =

[Philippe, 2014/02/25 09:41:12]

Nit: s/process id/RPH ID (see my comment below).

[Yaron, 2014/02/25 18:14:48]

Done.

     LAZY_INSTANCE_INITIALIZER;
 
+// Listens for RenderProcessHosts who's renderers have crashed. This is

[Philippe, 2014/02/25 09:41:12]

You mean in your CL description that the same RenderProcessHost instance is kept
alive when a renderer process gets killed and re-spawned, right?

It looks to me that we may have had this in mind when landing this CL. We did
completely screw up though by using RenderProcessHost::GetID(). By looking at
RenderProcessHostImpl the ID is constant (and set to the initial renderer
process PID) as opposed to the current renderer process' PID backing the RPH (as
I/we were expecting). Having a non-constant ID would not be sane either though
:) I suspect that we should have used RPH::GetHandle() in the first place as
opposed to RPH::GetID() but there is no RPH::FromHandle() method...

So removing the RPH IDs from the vector as the renderer processes get killed
like you did looks like the best/only solution IMO (modulo doing the refactoring
mentioned line 26 of course :).

[Philippe, 2014/02/25 09:41:12]

Nit: s/who's/whose I believe although my knowledge in English is quite limited
:)

[Yaron, 2014/02/25 18:14:48]

Done.

[Yaron, 2014/02/25 18:14:48]

It's not actually the renderer process pid. It's just a monotonically increasing
id. Ya, I came to the same conclusion about GetHandle when looking for an
alternate way.

+// necessary to ensure that we don't over-resume a RenderProcess as described
+// above.
+class SuspendedProcessWatcher : public content::RenderProcessHostObserver {
+  virtual void RenderProcessExited(content::RenderProcessHost* host,
+                                    base::ProcessHandle handle,

[Philippe, 2014/02/25 09:41:12]

Nit: extra leading space here and on the two lines below.

[Yaron, 2014/02/25 18:14:48]

Done.

+                                    base::TerminationStatus status,
+                                    int exit_code) OVERRIDE {
+    std::vector<int>* suspended_processes = g_suspended_processes.Pointer();
+    std::vector<int>::iterator pos = std::find(suspended_processes->begin(),
+                                               suspended_processes->end(),
+                                               host->GetID());
+    if (pos != suspended_processes->end()) {
+      host->RemoveObserver(this);
+      suspended_processes->erase(pos);
+    }
+  }
+};
+
+base::LazyInstance<SuspendedProcessWatcher > g_suspended_processes_watcher =

[Philippe, 2014/02/25 09:41:12]

Nit: extra space before '>'.

[Yaron, 2014/02/25 18:14:48]

Done.

+    LAZY_INSTANCE_INITIALIZER;
+
 // Suspends timers in all current render processes.
 void SuspendWebKitSharedTimers(std::vector<int>* suspended_processes) {
   for (content::RenderProcessHost::iterator i(
           content::RenderProcessHost::AllHostsIterator());
        !i.IsAtEnd(); i.Advance()) {
     content::RenderProcessHost* host = i.GetCurrentValue();
     suspended_processes->push_back(host->GetID());
     host->Send(new ViewMsg_SetWebKitSharedTimersSuspended(true));
+    host->AddObserver(g_suspended_processes_watcher.Pointer());
   }
 }
 
 // Resumes timers in processes that were previously stopped.
 void ResumeWebkitSharedTimers(const std::vector<int>& suspended_processes) {
   for (std::vector<int>::const_iterator it = suspended_processes.begin();
        it != suspended_processes.end(); ++it) {
     content::RenderProcessHost* host = content::RenderProcessHost::FromID(*it);
     if (host)  // The process might have been killed since it was suspended.
       host->Send(new ViewMsg_SetWebKitSharedTimersSuspended(false));
@@ skipping 24 matching lines @@
   }
 }
 
 namespace content {
 
 bool RegisterWebViewStatics(JNIEnv* env) {
   return RegisterNativesImpl(env) >= 0;
 }
 
 }  // namespace content