Don't silently eat Inspector.TargetCrashed

headless/app/headless_shell.cc

Index: headless/app/headless_shell.cc
diff --git a/headless/app/headless_shell.cc b/headless/app/headless_shell.cc
index 6f291a6cd6b9f8a71cc852e80644bcd69aec76a3..86c2ba60cbd91b61f3083979fa829400858d067f 100644
--- a/headless/app/headless_shell.cc
+++ b/headless/app/headless_shell.cc
@@ -19,6 +19,7 @@
 #include "content/public/common/content_switches.h"
 #include "headless/app/headless_shell_switches.h"
 #include "headless/public/devtools/domains/emulation.h"
+#include "headless/public/devtools/domains/inspector.h"
 #include "headless/public/devtools/domains/page.h"
 #include "headless/public/devtools/domains/runtime.h"
 #include "headless/public/headless_browser.h"
@@ -55,6 +56,7 @@ bool ParseWindowSize(std::string window_size, gfx::Size* parsed_window_size) {
 // An application which implements a simple headless browser.
 class HeadlessShell : public HeadlessWebContents::Observer,
                       emulation::ExperimentalObserver,
+                      inspector::ExperimentalObserver,
                       page::Observer {
  public:
   HeadlessShell()
@@ -118,6 +120,7 @@ class HeadlessShell : public HeadlessWebContents::Observer,
       return;
     if (!RemoteDebuggingEnabled()) {
       devtools_client_->GetEmulation()->GetExperimental()->RemoveObserver(this);
+      devtools_client_->GetInspector()->GetExperimental()->RemoveObserver(this);
       devtools_client_->GetPage()->RemoveObserver(this);
       if (web_contents_->GetDevToolsTarget()) {
         web_contents_->GetDevToolsTarget()->DetachClient(
@@ -135,6 +138,7 @@ class HeadlessShell : public HeadlessWebContents::Observer,
     if (RemoteDebuggingEnabled())
       return;
     web_contents_->GetDevToolsTarget()->AttachClient(devtools_client_.get());
+    devtools_client_->GetInspector()->GetExperimental()->AddObserver(this);
     devtools_client_->GetPage()->AddObserver(this);
     devtools_client_->GetPage()->Enable();
     // Check if the document had already finished loading by the time we
@@ -162,11 +166,7 @@ class HeadlessShell : public HeadlessWebContents::Observer,
     // TODO(skyostil): Implement more features to demonstrate the devtools API.
   }
 
-  void RenderProcessExited(base::TerminationStatus status,
-                           int exit_code) override {
-    if (status == base::TERMINATION_STATUS_NORMAL_TERMINATION)
-      return;
-
+  void OnTargetCrashed(const inspector::TargetCrashedParams& params) override {
     LOG(ERROR) << "Abnormal renderer termination.";
     Shutdown();

[Sami, 2016/11/21 14:59:28]

Should we remove this call to Shutdown() too (could be a TODO too)? If someone
wants to use Headless Shell as a long running rendering backend, tearing it down
if one renderer crashes isn't ideal.

[alex clarke (OOO till 29th), 2016/11/22 10:48:55]

Do people do that?

tearing it down
Well people where complaining that it never dumped the dom or generated a screen
shot.

[Sami, 2016/11/22 11:23:11]

Yeah, I would expect the NodeJS module to be used as a long running server.
That's how I think the existing Xvfb-version is used. There's a lot of overhead
in spinning up a new browser for every request.

I think we should come up with some guideline about what role Headless Shell
should fill. In my mind it should be less of a headless swiss army knife (we can
write a Node front-end which is that instead) and more of a dumb programmable
browser. Right now it's sort of an awkward mix of the two -- and for sort of a
good reason since we intended it to be a demo app. Now that --headless is going
to productize it, I think we should rethink a bit.

Maybe for the near term we could decide on a split where convenience stuff like
this automatic shutdown only happens if you're not using remote debugging. WDYT?

[alex clarke (OOO till 29th), 2016/11/22 15:30:58]

Note due to the way DevToolsTargetReady is written the observer never gets
registered if remote debugging is enabled.  I added a comment.

   }