Oxide

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

renderProcessGone
Merge into oxide.trunk

Proposed by Olivier Tilloy on 2015-04-28

Status:

Merged

Merged at revision:

1064

Proposed branch:

lp://qastaging/~osomon/oxide/renderProcessGone

Merge into:

lp://qastaging/~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://qastaging/~osomon/oxide/renderProcessGone

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Chris Coulson		2015-04-28	Approve on 2015-05-11
Review via email: mp+257693@code.qastaging.launchpad.net

Commit message

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

Revision history for this message

Alexandre Abreu (abreu-alexandre) wrote on 2015-04-28:

Added quick comment...

lp://qastaging/~osomon/oxide/renderProcessGone updated on 2015-04-29

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.

Revision history for this message

Chris Coulson (chrisccoulson) wrote on 2015-05-04:

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://qastaging/~osomon/oxide/renderProcessGone updated on 2015-05-06

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.

Revision history for this message

Olivier Tilloy (osomon) wrote on 2015-05-06:

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 on 2015-05-06:

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 on 2015-05-11:

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://qastaging/~osomon/oxide/renderProcessGone updated on 2015-05-11

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.