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

Proposed by Olivier Tilloy
Status: Merged
Merged at revision: 1064
Proposed branch: lp:~osomon/oxide/renderProcessGone
Merge into: lp:~oxide-developers/oxide/oxide.trunk
Diff against target: 393 lines (+173/-2)
12 files modified
qt/core/browser/oxide_qt_web_view.cc (+21/-0)
qt/core/browser/oxide_qt_web_view.h (+4/-0)
qt/core/glue/oxide_qt_web_view_proxy.h (+8/-0)
qt/core/glue/oxide_qt_web_view_proxy_client.h (+2/-0)
qt/qmlplugin/oxide_qml_plugin.cc (+2/-0)
qt/quick/api/oxideqquickwebview.cc (+22/-0)
qt/quick/api/oxideqquickwebview_p.h (+12/-0)
qt/quick/api/oxideqquickwebview_p_p.h (+1/-0)
qt/tests/qmltests/api/tst_WebView_webProcessStatus.qml (+40/-0)
qt/tests/qmltests/oxide_qml_testing_plugin.cc (+49/-1)
shared/browser/oxide_web_view.cc (+9/-1)
shared/browser/oxide_web_view.h (+3/-0)
To merge this branch: bzr merge lp:~osomon/oxide/renderProcessGone
Reviewer Review Type Date Requested Status
Chris Coulson Approve
Review via email: mp+257693@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Alexandre Abreu (abreu-alexandre) wrote :

Added quick comment...

lp:~osomon/oxide/renderProcessGone updated
1058. By Olivier Tilloy

Safety checks before doing a static cast.

1059. By Olivier Tilloy

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

One more sanity check.

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

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 updated
1061. By Olivier Tilloy

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

1062. By Olivier Tilloy

Add unit tests for WebView.webProcessStatus.

Revision history for this message
Olivier Tilloy (osomon) wrote :

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).

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

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

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

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 updated
1063. By Olivier Tilloy

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

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

1065. By Olivier Tilloy

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'qt/core/browser/oxide_qt_web_view.cc'
2--- qt/core/browser/oxide_qt_web_view.cc 2015-04-16 23:11:37 +0000
3+++ qt/core/browser/oxide_qt_web_view.cc 2015-05-11 10:20:57 +0000
4@@ -545,6 +545,10 @@
5 message_handlers_.at(index))->handler();
6 }
7
8+void WebView::OnCrashedStatusChanged() {
9+ client_->WebProcessStatusChanged();
10+}
11+
12 void WebView::OnURLChanged() {
13 client_->URLChanged();
14 }
15@@ -1468,6 +1472,23 @@
16 HideLocationBar(animate);
17 }
18
19+WebProcessStatus WebView::webProcessStatus() const {
20+ if (!GetWebContents()) {
21+ return WEB_PROCESS_RUNNING;
22+ }
23+
24+ base::TerminationStatus status = GetWebContents()->GetCrashedStatus();
25+ if (status == base::TERMINATION_STATUS_STILL_RUNNING) {
26+ return WEB_PROCESS_RUNNING;
27+ } else if (status == base::TERMINATION_STATUS_PROCESS_WAS_KILLED) {
28+ return WEB_PROCESS_KILLED;
29+ } else {
30+ // Map all other termination statuses to crashed. This is
31+ // consistent with how the sad tab helper works in Chrome.
32+ return WEB_PROCESS_CRASHED;
33+ }
34+}
35+
36 WebView::WebView(WebViewProxyClient* client) :
37 client_(client),
38 has_input_method_state_(false),
39
40=== modified file 'qt/core/browser/oxide_qt_web_view.h'
41--- qt/core/browser/oxide_qt_web_view.h 2015-04-16 23:11:37 +0000
42+++ qt/core/browser/oxide_qt_web_view.h 2015-05-11 10:20:57 +0000
43@@ -101,6 +101,8 @@
44 const oxide::ScriptMessageHandler* GetScriptMessageHandlerAt(
45 size_t index) const override;
46
47+ void OnCrashedStatusChanged() override;
48+
49 void OnURLChanged() override;
50 void OnTitleChanged() override;
51 void OnIconChanged(const GURL& icon) override;
52@@ -271,6 +273,8 @@
53 void locationBarShow(bool animate) override;
54 void locationBarHide(bool animate) override;
55
56+ WebProcessStatus webProcessStatus() const override;
57+
58 WebViewProxyClient* client_;
59
60 bool has_input_method_state_;
61
62=== modified file 'qt/core/glue/oxide_qt_web_view_proxy.h'
63--- qt/core/glue/oxide_qt_web_view_proxy.h 2015-04-16 23:11:37 +0000
64+++ qt/core/glue/oxide_qt_web_view_proxy.h 2015-05-11 10:20:57 +0000
65@@ -76,6 +76,12 @@
66 LOCATION_BAR_MODE_HIDDEN
67 };
68
69+enum WebProcessStatus {
70+ WEB_PROCESS_RUNNING,
71+ WEB_PROCESS_KILLED,
72+ WEB_PROCESS_CRASHED
73+};
74+
75 class CompositorFrameHandle {
76 public:
77 virtual ~CompositorFrameHandle() {}
78@@ -196,6 +202,8 @@
79 virtual void setLocationBarAnimated(bool animated) = 0;
80 virtual void locationBarShow(bool animate) = 0;
81 virtual void locationBarHide(bool animate) = 0;
82+
83+ virtual WebProcessStatus webProcessStatus() const = 0;
84 };
85
86 OXIDE_Q_DECL_PROXY_HANDLE(WebViewProxy);
87
88=== modified file 'qt/core/glue/oxide_qt_web_view_proxy_client.h'
89--- qt/core/glue/oxide_qt_web_view_proxy_client.h 2015-04-09 16:41:51 +0000
90+++ qt/core/glue/oxide_qt_web_view_proxy_client.h 2015-05-11 10:20:57 +0000
91@@ -79,6 +79,8 @@
92 JavaScriptDialogProxyClient* client) = 0;
93 virtual FilePickerProxy* CreateFilePicker(FilePickerProxyClient* client) = 0;
94
95+ virtual void WebProcessStatusChanged() = 0;
96+
97 virtual void URLChanged() = 0;
98 virtual void TitleChanged() = 0;
99 virtual void IconChanged(QUrl icon) = 0; // XXX(chrisccoulson): Move paramter to a member on WebView
100
101=== modified file 'qt/qmlplugin/oxide_qml_plugin.cc'
102--- qt/qmlplugin/oxide_qml_plugin.cc 2015-04-16 23:11:37 +0000
103+++ qt/qmlplugin/oxide_qml_plugin.cc 2015-05-11 10:20:57 +0000
104@@ -127,6 +127,8 @@
105
106 qmlRegisterUncreatableType<OxideQQuickLocationBarController, 1>(uri, 1, 7, "LocationBarController",
107 "LocationBarController is accessed via WebView.locationBarController");
108+
109+ qmlRegisterType<OxideQQuickWebView, 4>(uri, 1, 8, "WebView");
110 }
111 };
112
113
114=== modified file 'qt/quick/api/oxideqquickwebview.cc'
115--- qt/quick/api/oxideqquickwebview.cc 2015-04-16 23:11:37 +0000
116+++ qt/quick/api/oxideqquickwebview.cc 2015-05-11 10:20:57 +0000
117@@ -309,6 +309,12 @@
118 return new oxide::qquick::FilePicker(q, client);
119 }
120
121+void OxideQQuickWebViewPrivate::WebProcessStatusChanged() {
122+ Q_Q(OxideQQuickWebView);
123+
124+ emit q->webProcessStatusChanged();
125+}
126+
127 void OxideQQuickWebViewPrivate::URLChanged() {
128 Q_Q(OxideQQuickWebView);
129
130@@ -1642,6 +1648,22 @@
131 return d->location_bar_controller_.data();
132 }
133
134+OxideQQuickWebView::WebProcessStatus OxideQQuickWebView::webProcessStatus() const {
135+ Q_D(const OxideQQuickWebView);
136+
137+ Q_STATIC_ASSERT(
138+ WebProcessRunning ==
139+ static_cast<WebProcessStatus>(oxide::qt::WEB_PROCESS_RUNNING));
140+ Q_STATIC_ASSERT(
141+ WebProcessKilled ==
142+ static_cast<WebProcessStatus>(oxide::qt::WEB_PROCESS_KILLED));
143+ Q_STATIC_ASSERT(
144+ WebProcessCrashed ==
145+ static_cast<WebProcessStatus>(oxide::qt::WEB_PROCESS_CRASHED));
146+
147+ return static_cast<WebProcessStatus>(d->proxy()->webProcessStatus());
148+}
149+
150 // static
151 OxideQQuickWebViewAttached* OxideQQuickWebView::qmlAttachedProperties(
152 QObject* object) {
153
154=== modified file 'qt/quick/api/oxideqquickwebview_p.h'
155--- qt/quick/api/oxideqquickwebview_p.h 2015-01-24 00:50:27 +0000
156+++ qt/quick/api/oxideqquickwebview_p.h 2015-05-11 10:20:57 +0000
157@@ -69,6 +69,7 @@
158 Q_FLAGS(ContentType)
159 Q_ENUMS(LogMessageSeverityLevel);
160 Q_ENUMS(RestoreType);
161+ Q_ENUMS(WebProcessStatus);
162
163 Q_PROPERTY(QUrl url READ url WRITE setUrl NOTIFY urlChanged)
164 Q_PROPERTY(QString title READ title NOTIFY titleChanged)
165@@ -118,6 +119,8 @@
166
167 Q_PROPERTY(OxideQQuickLocationBarController* locationBarController READ locationBarController CONSTANT REVISION 3)
168
169+ Q_PROPERTY(WebProcessStatus webProcessStatus READ webProcessStatus NOTIFY webProcessStatusChanged REVISION 4)
170+
171 Q_DECLARE_PRIVATE(OxideQQuickWebView)
172
173 public:
174@@ -148,6 +151,12 @@
175 RestoreLastSessionCrashed
176 };
177
178+ enum WebProcessStatus {
179+ WebProcessRunning,
180+ WebProcessKilled,
181+ WebProcessCrashed
182+ };
183+
184 void componentComplete();
185
186 QUrl url() const;
187@@ -223,6 +232,8 @@
188
189 OxideQQuickLocationBarController* locationBarController();
190
191+ WebProcessStatus webProcessStatus() const;
192+
193 static OxideQQuickWebViewAttached* qmlAttachedProperties(QObject* object);
194
195 public Q_SLOTS:
196@@ -278,6 +289,7 @@
197 void blockedContentChanged();
198 Q_REVISION(2) void prepareToCloseResponse(bool proceed);
199 Q_REVISION(2) void closeRequested();
200+ Q_REVISION(4) void webProcessStatusChanged();
201
202 // Deprecated since 1.3
203 void loadingChanged(OxideQLoadEvent* loadEvent);
204
205=== modified file 'qt/quick/api/oxideqquickwebview_p_p.h'
206--- qt/quick/api/oxideqquickwebview_p_p.h 2015-04-16 23:11:37 +0000
207+++ qt/quick/api/oxideqquickwebview_p_p.h 2015-05-11 10:20:57 +0000
208@@ -93,6 +93,7 @@
209 oxide::qt::JavaScriptDialogProxyClient* client) override;
210 oxide::qt::FilePickerProxy* CreateFilePicker(
211 oxide::qt::FilePickerProxyClient* client) override;
212+ void WebProcessStatusChanged() override;
213 void URLChanged() override;
214 void TitleChanged() override;
215 void IconChanged(QUrl icon) override;
216
217=== added file 'qt/tests/qmltests/api/tst_WebView_webProcessStatus.qml'
218--- qt/tests/qmltests/api/tst_WebView_webProcessStatus.qml 1970-01-01 00:00:00 +0000
219+++ qt/tests/qmltests/api/tst_WebView_webProcessStatus.qml 2015-05-11 10:20:57 +0000
220@@ -0,0 +1,40 @@
221+import QtQuick 2.0
222+import QtTest 1.0
223+import com.canonical.Oxide 1.8
224+import com.canonical.Oxide.Testing 1.0
225+
226+TestWebView {
227+ id: webView
228+ focus: true
229+ width: 200
230+ height: 200
231+
232+ TestCase {
233+ name: "WebView_webProcessStatus"
234+ when: windowShown
235+
236+ function test_WebView_webProcessStatus_data() {
237+ return [
238+ { signal: 9, status: WebView.WebProcessKilled },
239+ { signal: 11, status: WebView.WebProcessCrashed }
240+ ];
241+ }
242+
243+ function test_WebView_webProcessStatus(data) {
244+ skip("TODO: re-enable once https://launchpad.net/bugs/1452407 is fixed");
245+
246+ webView.url = "http://testsuite/empty.html";
247+ verify(webView.waitForLoadSucceeded(),
248+ "Timed out waiting for successful load");
249+ compare(webView.webProcessStatus, WebView.WebProcessRunning);
250+
251+ OxideTestingUtils.killWebProcesses(data.signal);
252+ tryCompare(webView, "webProcessStatus", data.status);
253+
254+ webView.reload();
255+ verify(webView.waitForLoadSucceeded(),
256+ "Timed out waiting for successful load");
257+ compare(webView.webProcessStatus, WebView.WebProcessRunning);
258+ }
259+ }
260+}
261
262=== modified file 'qt/tests/qmltests/oxide_qml_testing_plugin.cc'
263--- qt/tests/qmltests/oxide_qml_testing_plugin.cc 2015-01-16 22:46:17 +0000
264+++ qt/tests/qmltests/oxide_qml_testing_plugin.cc 2015-05-11 10:20:57 +0000
265@@ -1,5 +1,5 @@
266 // vim:expandtab:shiftwidth=2:tabstop=2:
267-// Copyright (C) 2013 Canonical Ltd.
268+// Copyright (C) 2013-2015 Canonical Ltd.
269
270 // This library is free software; you can redistribute it and/or
271 // modify it under the terms of the GNU Lesser General Public
272@@ -15,17 +15,49 @@
273 // License along with this library; if not, write to the Free Software
274 // Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
275
276+#include <signal.h>
277+#include <sys/types.h>
278+#include <unistd.h>
279+
280 #include <QCoreApplication>
281 #include <QDesktopServices>
282 #include <QLatin1String>
283+#include <QList>
284 #include <QQmlContext>
285 #include <QQmlExtensionPlugin>
286 #include <QQmlParserStatus>
287+#include <QProcess>
288 #include <QString>
289 #include <QtGlobal>
290 #include <QtQml>
291 #include <QVariant>
292
293+namespace {
294+
295+QList<pid_t> getChildProcesses(pid_t pid) {
296+ QProcess pgrep;
297+ pgrep.start(QString("pgrep --parent %1").arg(pid));
298+ pgrep.waitForFinished();
299+ QList<QByteArray> output = pgrep.readAllStandardOutput().split('\n');
300+ QList<pid_t> children;
301+ Q_FOREACH (const QByteArray& child, output) {
302+ if (!child.isEmpty()) {
303+ children.append(child.toInt());
304+ }
305+ }
306+ return children;
307+}
308+
309+QList<pid_t> getDescendantProcesses(pid_t pid) {
310+ QList<pid_t> descendants = getChildProcesses(pid);
311+ Q_FOREACH (pid_t descendant, descendants) {
312+ descendants << getDescendantProcesses(descendant);
313+ }
314+ return descendants;
315+}
316+
317+} // namespace
318+
319 class ExternalProtocolHandler : public QObject,
320 public QQmlParserStatus {
321 Q_OBJECT
322@@ -141,6 +173,22 @@
323 Q_INVOKABLE void removeAppProperty(const QString& property) {
324 QCoreApplication::instance()->setProperty(property.toStdString().c_str(), QVariant());
325 }
326+
327+ Q_INVOKABLE void killWebProcesses(uint signal=SIGKILL) {
328+ Q_FOREACH (pid_t descendant, getDescendantProcesses(getpid())) {
329+ QProcess ps;
330+ ps.start(QString("ps fhp %1").arg(descendant));
331+ ps.waitForFinished();
332+ QString output = ps.readAllStandardOutput();
333+ if (output.contains("oxide-renderer") &&
334+ output.contains("--type=renderer")) {
335+ QProcess kill;
336+ kill.start(QString("kill -%1 %2")
337+ .arg(QString::number(signal), QString::number(descendant)));
338+ kill.waitForFinished();
339+ }
340+ }
341+ }
342 };
343
344 QObject* UtilsFactory(QQmlEngine* engine, QJSEngine* script_engine) {
345
346=== modified file 'shared/browser/oxide_web_view.cc'
347--- shared/browser/oxide_web_view.cc 2015-05-04 10:48:14 +0000
348+++ shared/browser/oxide_web_view.cc 2015-05-11 10:20:57 +0000
349@@ -868,7 +868,13 @@
350 frame->InitParent(parent);
351 }
352
353-void WebView::RenderProcessGone(base::TerminationStatus status) {}
354+void WebView::RenderViewReady() {
355+ OnCrashedStatusChanged();
356+}
357+
358+void WebView::RenderProcessGone(base::TerminationStatus status) {
359+ OnCrashedStatusChanged();
360+}
361
362 void WebView::RenderViewHostChanged(content::RenderViewHost* old_host,
363 content::RenderViewHost* new_host) {
364@@ -1111,6 +1117,8 @@
365 return handled;
366 }
367
368+void WebView::OnCrashedStatusChanged() {}
369+
370 void WebView::OnURLChanged() {}
371 void WebView::OnTitleChanged() {}
372 void WebView::OnIconChanged(const GURL& icon) {}
373
374=== modified file 'shared/browser/oxide_web_view.h'
375--- shared/browser/oxide_web_view.h 2015-04-16 23:11:37 +0000
376+++ shared/browser/oxide_web_view.h 2015-05-11 10:20:57 +0000
377@@ -445,6 +445,7 @@
378
379 // content::WebContentsObserver implementation
380 void RenderFrameCreated(content::RenderFrameHost* render_frame_host) final;
381+ void RenderViewReady() final;
382 void RenderProcessGone(base::TerminationStatus status) final;
383 void RenderViewHostChanged(content::RenderViewHost* old_host,
384 content::RenderViewHost* new_host) final;
385@@ -492,6 +493,8 @@
386 content::RenderFrameHost* render_frame_host) final;
387
388 // Override in sub-classes
389+ virtual void OnCrashedStatusChanged();
390+
391 virtual void OnURLChanged();
392 virtual void OnTitleChanged();
393 virtual void OnIconChanged(const GURL& icon);

Subscribers

People subscribed via source and target branches