Chromecast: call ClosePage(), wait 50ms before deleting WebContents

chromecast/shell/browser/android/cast_window_android.cc

Index: chromecast/shell/browser/android/cast_window_android.cc
diff --git a/chromecast/shell/browser/android/cast_window_android.cc b/chromecast/shell/browser/android/cast_window_android.cc
index 1ecc1c9acba38175981d1afdf7c6ef7f390ad3e5..12768aca57ff90a8ddc4db52b5b724c4f0808e61 100644
--- a/chromecast/shell/browser/android/cast_window_android.cc
+++ b/chromecast/shell/browser/android/cast_window_android.cc
@@ -4,7 +4,7 @@
 
 #include "chromecast/shell/browser/android/cast_window_android.h"
 
-#include "base/message_loop/message_loop.h"
+#include "base/message_loop/message_loop_proxy.h"
 #include "base/path_service.h"
 #include "chromecast/shell/browser/android/cast_window_manager.h"
 #include "content/public/browser/devtools_agent_host.h"
@@ -17,13 +17,22 @@
 namespace chromecast {
 namespace shell {
 
+namespace {
+
+// The time (in milliseconds) we wait for after a page is closed (i.e.
+// after an app is stopped) before we delete the corresponding WebContents.
+const int kWebContentsDestructionDelayInMs = 50;
+
+}  // namespace
+
 // static
 bool CastWindowAndroid::RegisterJni(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
 CastWindowAndroid::CastWindowAndroid(content::WebContents* web_contents)
-    : content::WebContentsObserver(web_contents) {
+    : content::WebContentsObserver(web_contents),
+      weak_factory_(this) {
 }
 
 CastWindowAndroid::~CastWindowAndroid() {
@@ -71,6 +80,12 @@ CastWindowAndroid* CastWindowAndroid::CreateCastWindowAndroid(
 }
 
 void CastWindowAndroid::Close() {
+  // Close content first. This fires window.unload event and
+  // CastWindowAndroid::CloseContents().

[lcwu1, 2014/09/25 19:10:54]

This should probably be something like this:
"Close page first, which would lead the window.unload event to be fired. The
window itself would be then destroyed inside CloseContents when the browser
receives the ACK from the renderer that the page is closed."

[gunsch, 2014/09/25 19:18:14]

Done.

+  web_contents_->GetRenderViewHost()->ClosePage();
+}
+
+void CastWindowAndroid::Destroy() {
   // Note: if multiple windows becomes supported, this may close other devtools
   // sessions.
   content::DevToolsAgentHost::DetachAllClients();
@@ -101,7 +116,29 @@ void CastWindowAndroid::AddNewContents(content::WebContents* source,
 
 void CastWindowAndroid::CloseContents(content::WebContents* source) {
   DCHECK_EQ(source, web_contents_.get());
-  Close();
+
+  // We need to delay the deletion of web_contents_ (currently for 50ms) to
+  // give (and guarantee) the renderer enough time to finish 'onunload'
+  // handler (but we don't want to wait any longer than that to delay the
+  // starting of next app).
+
+  if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType)) {
+    // When shutting down in a test context, the last remaining WebContents
+    // is torn down at browser-thread shutdown time. Call Destroy directly to
+    // avoid losing the last posted task to delete this object.
+    // TODO(gunsch): This could probably be avoided by using a
+    // CompletionCallback in StopCurrentApp to wait until the app is completely
+    // stopped. This might require a separate message loop and might only be
+    // appropriate for test contexts or during shutdown, since it triggers a
+    // wait on the main thread.
+    Destroy();
+    return;
+  }
+
+  base::MessageLoopProxy::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&CastWindowAndroid::Destroy, weak_factory_.GetWeakPtr()),
+      base::TimeDelta::FromMilliseconds(kWebContentsDestructionDelayInMs));
 }
 
 bool CastWindowAndroid::CanOverscrollContent() const {
@@ -128,7 +165,7 @@ void CastWindowAndroid::DeactivateContents(content::WebContents* contents) {
 
 void CastWindowAndroid::RenderProcessGone(base::TerminationStatus status) {
   LOG(ERROR) << "Render process gone: status=" << status;
-  Close();
+  Destroy();
 }
 
 }  // namespace shell