Merge lp:~osomon/oxide/renderProcessGone into lp:~oxide-developers/oxide/oxide.trunk

[Olivier Tilloy]

Add a WebView::webProcessStatus property to notify embedders when the renderer process crashed or was killed.

[Alexandre Abreu (abreu-alexandre)]

Added quick comment...

[lp:~osomon/oxide/renderProcessGone]

1058. By Olivier Tilloy on 2015-04-29

Safety checks before doing a static cast.

1059. By Olivier Tilloy on 2015-04-29

Add build-time assertions to ensure that casting a base::TerminationStatus to an int is safe and will always result in a correct value on the other side.

1060. By Olivier Tilloy on 2015-04-29

One more sanity check.

[Chris Coulson (chrisccoulson)]

Thanks for looking at this.

I wonder whether this API would be better as a property (eg, WebView.crashedStatus) rather than just having a signal. As this is indicating a status, a property probably makes more sense. Also it would have the advantage that it provides a mechanism to tell the application when a new process has been started, so that the browser can dismiss any UI it's showing.

Note that WebContents already tracks this and makes it available via WebContents::GetCrashedStatus. You can implement both WebContentsObserver::RenderProcessGone and WebContentsObserver::RenderViewReady for signalling changes to the application.

I've left a comment inline too

review: Needs Fixing

[lp:~osomon/oxide/renderProcessGone]

1061. By Olivier Tilloy on 2015-05-06

Replace the renderProcessGone signal with a webProcessStatus property on the webview.

1062. By Olivier Tilloy on 2015-05-06

Add unit tests for WebView.webProcessStatus.

[Olivier Tilloy (osomon)]

Updated to implement the suggested API (based on a property instead of a signal), and added unit tests. Note however that when running the tests I’m seeing the following error:

[0506/195725:FATAL:oxide_browser_context.cc(869)] Check failed: g_all_contexts.Get().size() == static_cast<size_t>(0) (1 vs. 0)

This happens no matter the signal sent to the renderer process (SIGKILL/SIGSEGV).

[Chris Coulson (chrisccoulson)]

I've just reported bug 1452407 for that CHECK failure, although it's not really clear why this test would make hitting it more likely

[Chris Coulson (chrisccoulson)]

I've got a couple of small comments inline. With those, this is fine to commit with the test disabled for now (it's not this which is causing the shutdown crash).

I'll fix the other issue you're having in bug 1452407 and then enable the test

review: Approve

[lp:~osomon/oxide/renderProcessGone]

1063. By Olivier Tilloy on 2015-05-11

Return WEB_PROCESS_RUNNING for the web process status when it hasn’t started yet, for consistency with how WebContents initializes crashed_status_.

1064. By Olivier Tilloy on 2015-05-11

Unify OnRenderViewReady() and OnRenderProcessGone() into one single method, as they serve the exact same purpose.

1065. By Olivier Tilloy on 2015-05-11

Skip the tests for now until bug #1452407 is fixed.

=== modified file 'qt/core/browser/oxide_qt_web_view.cc'
--- qt/core/browser/oxide_qt_web_view.cc	2015-04-16 23:11:37 +0000
+++ qt/core/browser/oxide_qt_web_view.cc	2015-05-11 10:20:57 +0000
@@ -545,6 +545,10 @@
       message_handlers_.at(index))->handler();
 }
 
+void WebView::OnCrashedStatusChanged() {
+  client_->WebProcessStatusChanged();
+}
+
 void WebView::OnURLChanged() {
   client_->URLChanged();
 }
@@ -1468,6 +1472,23 @@
   HideLocationBar(animate);
 }
 
+WebProcessStatus WebView::webProcessStatus() const {
+  if (!GetWebContents()) {
+    return WEB_PROCESS_RUNNING;
+  }
+
+  base::TerminationStatus status = GetWebContents()->GetCrashedStatus();
+  if (status == base::TERMINATION_STATUS_STILL_RUNNING) {
+    return WEB_PROCESS_RUNNING;
+  } else if (status == base::TERMINATION_STATUS_PROCESS_WAS_KILLED) {
+    return WEB_PROCESS_KILLED;
+  } else {
+    // Map all other termination statuses to crashed. This is
+    // consistent with how the sad tab helper works in Chrome.
+    return WEB_PROCESS_CRASHED;
+  }
+}
+
 WebView::WebView(WebViewProxyClient* client) :
     client_(client),
     has_input_method_state_(false),

=== modified file 'qt/core/browser/oxide_qt_web_view.h'
--- qt/core/browser/oxide_qt_web_view.h	2015-04-16 23:11:37 +0000
+++ qt/core/browser/oxide_qt_web_view.h	2015-05-11 10:20:57 +0000
@@ -101,6 +101,8 @@
   const oxide::ScriptMessageHandler* GetScriptMessageHandlerAt(
       size_t index) const override;
 
+  void OnCrashedStatusChanged() override;
+
   void OnURLChanged() override;
   void OnTitleChanged() override;
   void OnIconChanged(const GURL& icon) override;
@@ -271,6 +273,8 @@
   void locationBarShow(bool animate) override;
   void locationBarHide(bool animate) override;
 
+  WebProcessStatus webProcessStatus() const override;
+
   WebViewProxyClient* client_;
 
   bool has_input_method_state_;

=== modified file 'qt/core/glue/oxide_qt_web_view_proxy.h'
--- qt/core/glue/oxide_qt_web_view_proxy.h	2015-04-16 23:11:37 +0000
+++ qt/core/glue/oxide_qt_web_view_proxy.h	2015-05-11 10:20:57 +0000
@@ -76,6 +76,12 @@
   LOCATION_BAR_MODE_HIDDEN
 };
 
+enum WebProcessStatus {
+  WEB_PROCESS_RUNNING,
+  WEB_PROCESS_KILLED,
+  WEB_PROCESS_CRASHED
+};
+
 class CompositorFrameHandle {
  public:
   virtual ~CompositorFrameHandle() {}
@@ -196,6 +202,8 @@
   virtual void setLocationBarAnimated(bool animated) = 0;
   virtual void locationBarShow(bool animate) = 0;
   virtual void locationBarHide(bool animate) = 0;
+
+  virtual WebProcessStatus webProcessStatus() const = 0;
 };
 
 OXIDE_Q_DECL_PROXY_HANDLE(WebViewProxy);

=== modified file 'qt/core/glue/oxide_qt_web_view_proxy_client.h'
--- qt/core/glue/oxide_qt_web_view_proxy_client.h	2015-04-09 16:41:51 +0000
+++ qt/core/glue/oxide_qt_web_view_proxy_client.h	2015-05-11 10:20:57 +0000
@@ -79,6 +79,8 @@
       JavaScriptDialogProxyClient* client) = 0;
   virtual FilePickerProxy* CreateFilePicker(FilePickerProxyClient* client) = 0;
 
+  virtual void WebProcessStatusChanged() = 0;
+
   virtual void URLChanged() = 0;
   virtual void TitleChanged() = 0;
   virtual void IconChanged(QUrl icon) = 0; // XXX(chrisccoulson): Move paramter to a member on WebView

=== modified file 'qt/qmlplugin/oxide_qml_plugin.cc'
--- qt/qmlplugin/oxide_qml_plugin.cc	2015-04-16 23:11:37 +0000
+++ qt/qmlplugin/oxide_qml_plugin.cc	2015-05-11 10:20:57 +0000
@@ -127,6 +127,8 @@
 
     qmlRegisterUncreatableType<OxideQQuickLocationBarController, 1>(uri, 1, 7, "LocationBarController",
         "LocationBarController is accessed via WebView.locationBarController");
+
+    qmlRegisterType<OxideQQuickWebView, 4>(uri, 1, 8, "WebView");
   }
 };
 

=== modified file 'qt/quick/api/oxideqquickwebview.cc'
--- qt/quick/api/oxideqquickwebview.cc	2015-04-16 23:11:37 +0000
+++ qt/quick/api/oxideqquickwebview.cc	2015-05-11 10:20:57 +0000
@@ -309,6 +309,12 @@
   return new oxide::qquick::FilePicker(q, client);
 }
 
+void OxideQQuickWebViewPrivate::WebProcessStatusChanged() {
+  Q_Q(OxideQQuickWebView);
+
+  emit q->webProcessStatusChanged();
+}
+
 void OxideQQuickWebViewPrivate::URLChanged() {
   Q_Q(OxideQQuickWebView);
 
@@ -1642,6 +1648,22 @@
   return d->location_bar_controller_.data();
 }
 
+OxideQQuickWebView::WebProcessStatus OxideQQuickWebView::webProcessStatus() const {
+  Q_D(const OxideQQuickWebView);
+
+  Q_STATIC_ASSERT(
+      WebProcessRunning ==
+        static_cast<WebProcessStatus>(oxide::qt::WEB_PROCESS_RUNNING));
+  Q_STATIC_ASSERT(
+      WebProcessKilled ==
+        static_cast<WebProcessStatus>(oxide::qt::WEB_PROCESS_KILLED));
+  Q_STATIC_ASSERT(
+      WebProcessCrashed ==
+        static_cast<WebProcessStatus>(oxide::qt::WEB_PROCESS_CRASHED));
+
+  return static_cast<WebProcessStatus>(d->proxy()->webProcessStatus());
+}
+
 // static
 OxideQQuickWebViewAttached* OxideQQuickWebView::qmlAttachedProperties(
     QObject* object) {

=== modified file 'qt/quick/api/oxideqquickwebview_p.h'
--- qt/quick/api/oxideqquickwebview_p.h	2015-01-24 00:50:27 +0000
+++ qt/quick/api/oxideqquickwebview_p.h	2015-05-11 10:20:57 +0000
@@ -69,6 +69,7 @@
   Q_FLAGS(ContentType)
   Q_ENUMS(LogMessageSeverityLevel);
   Q_ENUMS(RestoreType);
+  Q_ENUMS(WebProcessStatus);
 
   Q_PROPERTY(QUrl url READ url WRITE setUrl NOTIFY urlChanged)
   Q_PROPERTY(QString title READ title NOTIFY titleChanged)
@@ -118,6 +119,8 @@
 
   Q_PROPERTY(OxideQQuickLocationBarController* locationBarController READ locationBarController CONSTANT REVISION 3)
 
+  Q_PROPERTY(WebProcessStatus webProcessStatus READ webProcessStatus NOTIFY webProcessStatusChanged REVISION 4)
+
   Q_DECLARE_PRIVATE(OxideQQuickWebView)
 
  public:
@@ -148,6 +151,12 @@
     RestoreLastSessionCrashed
   };
 
+  enum WebProcessStatus {
+    WebProcessRunning,
+    WebProcessKilled,
+    WebProcessCrashed
+  };
+
   void componentComplete();
 
   QUrl url() const;
@@ -223,6 +232,8 @@
 
   OxideQQuickLocationBarController* locationBarController();
 
+  WebProcessStatus webProcessStatus() const;
+
   static OxideQQuickWebViewAttached* qmlAttachedProperties(QObject* object);
 
  public Q_SLOTS:
@@ -278,6 +289,7 @@
   void blockedContentChanged();
   Q_REVISION(2) void prepareToCloseResponse(bool proceed);
   Q_REVISION(2) void closeRequested();
+  Q_REVISION(4) void webProcessStatusChanged();
 
   // Deprecated since 1.3
   void loadingChanged(OxideQLoadEvent* loadEvent);

=== modified file 'qt/quick/api/oxideqquickwebview_p_p.h'
--- qt/quick/api/oxideqquickwebview_p_p.h	2015-04-16 23:11:37 +0000
+++ qt/quick/api/oxideqquickwebview_p_p.h	2015-05-11 10:20:57 +0000
@@ -93,6 +93,7 @@
       oxide::qt::JavaScriptDialogProxyClient* client) override;
   oxide::qt::FilePickerProxy* CreateFilePicker(
       oxide::qt::FilePickerProxyClient* client) override;
+  void WebProcessStatusChanged() override;
   void URLChanged() override;
   void TitleChanged() override;
   void IconChanged(QUrl icon) override;

=== added file 'qt/tests/qmltests/api/tst_WebView_webProcessStatus.qml'
--- qt/tests/qmltests/api/tst_WebView_webProcessStatus.qml	1970-01-01 00:00:00 +0000
+++ qt/tests/qmltests/api/tst_WebView_webProcessStatus.qml	2015-05-11 10:20:57 +0000
@@ -0,0 +1,40 @@
+import QtQuick 2.0
+import QtTest 1.0
+import com.canonical.Oxide 1.8
+import com.canonical.Oxide.Testing 1.0
+
+TestWebView {
+  id: webView
+  focus: true
+  width: 200
+  height: 200
+
+  TestCase {
+    name: "WebView_webProcessStatus"
+    when: windowShown
+
+    function test_WebView_webProcessStatus_data() {
+      return [
+        { signal: 9, status: WebView.WebProcessKilled },
+        { signal: 11, status: WebView.WebProcessCrashed }
+      ];
+    }
+
+    function test_WebView_webProcessStatus(data) {
+      skip("TODO: re-enable once https://launchpad.net/bugs/1452407 is fixed");
+
+      webView.url = "http://testsuite/empty.html";
+      verify(webView.waitForLoadSucceeded(),
+             "Timed out waiting for successful load");
+      compare(webView.webProcessStatus, WebView.WebProcessRunning);
+
+      OxideTestingUtils.killWebProcesses(data.signal);
+      tryCompare(webView, "webProcessStatus", data.status);
+
+      webView.reload();
+      verify(webView.waitForLoadSucceeded(),
+             "Timed out waiting for successful load");
+      compare(webView.webProcessStatus, WebView.WebProcessRunning);
+    }
+  }
+}

=== modified file 'qt/tests/qmltests/oxide_qml_testing_plugin.cc'
--- qt/tests/qmltests/oxide_qml_testing_plugin.cc	2015-01-16 22:46:17 +0000
+++ qt/tests/qmltests/oxide_qml_testing_plugin.cc	2015-05-11 10:20:57 +0000
@@ -1,5 +1,5 @@
 // vim:expandtab:shiftwidth=2:tabstop=2:
-// Copyright (C) 2013 Canonical Ltd.
+// Copyright (C) 2013-2015 Canonical Ltd.
 
 // This library is free software; you can redistribute it and/or
 // modify it under the terms of the GNU Lesser General Public
@@ -15,17 +15,49 @@
 // License along with this library; if not, write to the Free Software
 // Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
 
+#include <signal.h>
+#include <sys/types.h>
+#include <unistd.h>
+
 #include <QCoreApplication>
 #include <QDesktopServices>
 #include <QLatin1String>
+#include <QList>
 #include <QQmlContext>
 #include <QQmlExtensionPlugin>
 #include <QQmlParserStatus>
+#include <QProcess>
 #include <QString>
 #include <QtGlobal>
 #include <QtQml>
 #include <QVariant>
 
+namespace {
+
+QList<pid_t> getChildProcesses(pid_t pid) {
+  QProcess pgrep;
+  pgrep.start(QString("pgrep --parent %1").arg(pid));
+  pgrep.waitForFinished();
+  QList<QByteArray> output = pgrep.readAllStandardOutput().split('\n');
+  QList<pid_t> children;
+  Q_FOREACH (const QByteArray& child, output) {
+    if (!child.isEmpty()) {
+      children.append(child.toInt());
+    }
+  }
+  return children;
+}
+
+QList<pid_t> getDescendantProcesses(pid_t pid) {
+  QList<pid_t> descendants = getChildProcesses(pid);
+  Q_FOREACH (pid_t descendant, descendants) {
+    descendants << getDescendantProcesses(descendant);
+  }
+  return descendants;
+}
+
+} // namespace
+
 class ExternalProtocolHandler : public QObject,
                                 public QQmlParserStatus {
   Q_OBJECT
@@ -141,6 +173,22 @@
   Q_INVOKABLE void removeAppProperty(const QString& property) {
     QCoreApplication::instance()->setProperty(property.toStdString().c_str(), QVariant());
   }
+
+  Q_INVOKABLE void killWebProcesses(uint signal=SIGKILL) {
+    Q_FOREACH (pid_t descendant, getDescendantProcesses(getpid())) {
+      QProcess ps;
+      ps.start(QString("ps fhp %1").arg(descendant));
+      ps.waitForFinished();
+      QString output = ps.readAllStandardOutput();
+      if (output.contains("oxide-renderer") &&
+          output.contains("--type=renderer")) {
+        QProcess kill;
+        kill.start(QString("kill -%1 %2")
+            .arg(QString::number(signal), QString::number(descendant)));
+        kill.waitForFinished();
+      }
+    }
+  }
 };
 
 QObject* UtilsFactory(QQmlEngine* engine, QJSEngine* script_engine) {

=== modified file 'shared/browser/oxide_web_view.cc'
--- shared/browser/oxide_web_view.cc	2015-05-04 10:48:14 +0000
+++ shared/browser/oxide_web_view.cc	2015-05-11 10:20:57 +0000
@@ -868,7 +868,13 @@
   frame->InitParent(parent);
 }
 
-void WebView::RenderProcessGone(base::TerminationStatus status) {}
+void WebView::RenderViewReady() {
+  OnCrashedStatusChanged();
+}
+
+void WebView::RenderProcessGone(base::TerminationStatus status) {
+  OnCrashedStatusChanged();
+}
 
 void WebView::RenderViewHostChanged(content::RenderViewHost* old_host,
                                     content::RenderViewHost* new_host) {
@@ -1111,6 +1117,8 @@
   return handled;
 }
 
+void WebView::OnCrashedStatusChanged() {}
+
 void WebView::OnURLChanged() {}
 void WebView::OnTitleChanged() {}
 void WebView::OnIconChanged(const GURL& icon) {}

=== modified file 'shared/browser/oxide_web_view.h'
--- shared/browser/oxide_web_view.h	2015-04-16 23:11:37 +0000
+++ shared/browser/oxide_web_view.h	2015-05-11 10:20:57 +0000
@@ -445,6 +445,7 @@
 
   // content::WebContentsObserver implementation
   void RenderFrameCreated(content::RenderFrameHost* render_frame_host) final;
+  void RenderViewReady() final;
   void RenderProcessGone(base::TerminationStatus status) final;
   void RenderViewHostChanged(content::RenderViewHost* old_host,
                              content::RenderViewHost* new_host) final;
@@ -492,6 +493,8 @@
                          content::RenderFrameHost* render_frame_host) final;
 
   // Override in sub-classes
+  virtual void OnCrashedStatusChanged();
+
   virtual void OnURLChanged();
   virtual void OnTitleChanged();
   virtual void OnIconChanged(const GURL& icon);