Bug 109216 – [Qt][WK2] Crash on window resize if WebProcess is closed/crashed

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Adenilson Cavalcanti Silva]

Steps to reproduce:
a) Start snowshoe
b) Kill QtWebProcess
c) Window resize

Cause of crash: calls to WebKit::DrawingAreaProxy without testing for pointer validity.

When the WebProcess was closed (or crashed), WebKit::WebPageProxy will set its DrawingAreaProxy data member pointer to null. At a resize event, QQuickWebView will simply call into its pimple updateViewportSize() which access the pointer to the WebPageProxy that is now set to null.

Some ports (e.g. EFL), will load an error page at WebProcess crash with the side effect forcing its re-spawn, which is not the case of Qt. Makes me wonder if we should have similar behavior?

This patch adds a test for the validity of WebPageProxy pointer which fixes the crash.

I considered the idea of creating a test for it, but it would either result in:
a) Having a test using a native API (e.g. POSIX) to inspect for QtWebProcess (i.e. inspecting /proc in linux) and kill it, followed by resizing a QQuickWebView client (QML WebView). This is not ideal since it would result in a test that only runs in a specific environment while Qt runs everywhere(Tm)!

b) Force QQuickWebView (or friends) to somehow export the process ID of QtWebProcess. Honestly, creating a YAPA (Yet Another Public API) just for a test's sake sounded like overkill.

Finally, it is a good idea to test for pointer state (if it can be null) before calling into it.

[Adenilson Cavalcanti Silva]

Created attachment 187146 [details]
The fix (a 2 liner)

[Benjamin Poulain]

Comment on attachment 187146 [details]
The fix (a 2 liner)

You are gonna need a better excuse to avoid writing a test :)

[Rafael Brandao]

Also I think this is not enough to solve the whole issue. Look at WebPageProxy::processDidCrash, page proxy is already invalidated. Why would you try to access m_drawingArea from it if you know it's invalid? This crash would also happen if you try to access something from m_mainFrame, so fixing this null check here would probably not be enough.

What you could use: m_pageClient->processDidCrash() (already called from there).
So your page client know when this happened, and maybe you could show a error page with a kitten image? Or just try to reload the last url? Or maybe export this behavior as API for the Qt's WebView user decide what to do.

[Benjamin Poulain]

I vote for the kitten!

[Adenilson Cavalcanti Silva]

Everybody loves kittens!

[Simon Hausmann]

(In reply to comment #2)
> (From update of attachment 187146 [details])
> You are gonna need a better excuse to avoid writing a test :)

From looking at mac/WKView.mm it appears that the common pattern is this:

if (DrawingAreaProxy* drawingArea = webPageProxy->drawingArea()) {
    drawingArea->foo();
    drawingArea->bar();
}

Code in qquickwebview.cpp should probably be changed to follow this pattern.

However having a unit test that calls every possible method in QQuickWebView after crashing the process manually seems overkill to me.

[Adenilson Cavalcanti Silva]

Created attachment 188159 [details]
Following reviewer's suggestion to test the pointed object

[Simon Hausmann]

Note how the example declares a variable in the if to avoid repeated calls to webPageProxy->drawingArea()

[Adenilson Cavalcanti Silva]

Created attachment 188170 [details]
Using a local pointer variable to save function calls.

[Simon Hausmann]

Comment on attachment 188170 [details]
Using a local pointer variable to save function calls.

Looks good to me. Benjamin, do you sign off on this? :)

[Benjamin Poulain]

(In reply to comment #10)
> (From update of attachment 188170 [details])
> Looks good to me. Benjamin, do you sign off on this? :)

Yep, this looks like a good idea. I sign off the patch.

[Benjamin Poulain]

> Yep, this looks like a good idea. I sign off the patch.

Hum, "I sign off on the patch" is probably more grammatically correct. :)

[Benjamin Poulain]

Comment on attachment 188170 [details]
Using a local pointer variable to save function calls.

Actually, no.

PlatformWebView has a method to resize: PlatformWebView::resizeTo().
It should be completely trivial to make an API tests (take  MouseMoveAfterCrash as an example). Why not add the test?

[Adenilson Cavalcanti Silva]

Benjamin

Answering the question: why not add a test? If I understood correctly, QtWebKit doesn't support WebKit2 API tests, we would need to provide a PlatformWebView implementation.

I can write a test just fine, fixing that other crash in WK2 (issue #109305) helped to understand better how tests are written. Plus, it would help to test this scenario (crash followed by window resize) in other ports.

Only issue in this test is that it wouldn't prove the case of this bug fix for Qt. I see the whole picture really as 3 different tasks:
a) Fix this Qt bug
b) Write a WK2 API test to simulate the crash scenario
c) Implement a PlatformWebView and change Qt buildsystem to support running WK2 API tests.

What you think about this?

[Benjamin Poulain]

Comment on attachment 188170 [details]
Using a local pointer variable to save function calls.

Good to go, given https://bugs.webkit.org/show_bug.cgi?id=109842.

[WebKit Review Bot]

Comment on attachment 188170 [details]
Using a local pointer variable to save function calls.

Rejecting attachment 188170 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=gce-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 188170, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
fs from patch file(s).
patching file Source/WebKit2/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp
Hunk #1 FAILED at 585.
Hunk #2 succeeded at 938 (offset 12 lines).
1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit2/UIProcess/API/qt/qquickwebview.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Benjamin Poulain']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/16589351

[Adenilson Cavalcanti Silva]

Created attachment 188668 [details]
Rebased patch

[WebKit Review Bot]

Comment on attachment 188668 [details]
Rebased patch

Clearing flags on attachment: 188668

Committed r143077: <http://trac.webkit.org/changeset/143077>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.