Bug 109216 - [Qt][WK2] Crash on window resize if WebProcess is closed/crashed
Summary: [Qt][WK2] Crash on window resize if WebProcess is closed/crashed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Nobody
URL:
Keywords:
Depends on: 109842 109843
Blocks:
  Show dependency treegraph
 
Reported: 2013-02-07 12:11 PST by Adenilson Cavalcanti Silva
Modified: 2013-02-15 17:43 PST (History)
7 users (show)

See Also:


Attachments
The fix (a 2 liner) (2.03 KB, patch)
2013-02-07 12:22 PST, Adenilson Cavalcanti Silva
benjamin: review-
benjamin: commit-queue-
Details | Formatted Diff | Diff
Following reviewer's suggestion to test the pointed object (3.86 KB, patch)
2013-02-13 13:08 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Using a local pointer variable to save function calls. (3.92 KB, patch)
2013-02-13 13:37 PST, Adenilson Cavalcanti Silva
benjamin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Rebased patch (3.85 KB, patch)
2013-02-15 16:56 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adenilson Cavalcanti Silva 2013-02-07 12:11:52 PST
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.
Comment 1 Adenilson Cavalcanti Silva 2013-02-07 12:22:37 PST
Created attachment 187146 [details]
The fix (a 2 liner)
Comment 2 Benjamin Poulain 2013-02-07 16:29:11 PST
Comment on attachment 187146 [details]
The fix (a 2 liner)

You are gonna need a better excuse to avoid writing a test :)
Comment 3 Rafael Brandao 2013-02-07 17:42:03 PST
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.
Comment 4 Benjamin Poulain 2013-02-07 18:19:49 PST
I vote for the kitten!
Comment 5 Adenilson Cavalcanti Silva 2013-02-13 07:35:08 PST
Everybody loves kittens!
Comment 6 Simon Hausmann 2013-02-13 08:14:39 PST
(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.
Comment 7 Adenilson Cavalcanti Silva 2013-02-13 13:08:22 PST
Created attachment 188159 [details]
Following reviewer's suggestion to test the pointed object
Comment 8 Simon Hausmann 2013-02-13 13:22:32 PST
Note how the example declares a variable in the if to avoid repeated calls to webPageProxy->drawingArea()
Comment 9 Adenilson Cavalcanti Silva 2013-02-13 13:37:12 PST
Created attachment 188170 [details]
Using a local pointer variable to save function calls.
Comment 10 Simon Hausmann 2013-02-14 00:36:16 PST
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? :)
Comment 11 Benjamin Poulain 2013-02-14 00:40:51 PST
(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.
Comment 12 Benjamin Poulain 2013-02-14 00:42:45 PST
> Yep, this looks like a good idea. I sign off the patch.

Hum, "I sign off on the patch" is probably more grammatically correct. :)
Comment 13 Benjamin Poulain 2013-02-14 00:55:48 PST
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?
Comment 14 Adenilson Cavalcanti Silva 2013-02-14 04:23:18 PST
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?
Comment 15 Benjamin Poulain 2013-02-14 23:24:33 PST
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.
Comment 16 WebKit Review Bot 2013-02-15 16:16:14 PST
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
Comment 17 Adenilson Cavalcanti Silva 2013-02-15 16:56:28 PST
Created attachment 188668 [details]
Rebased patch
Comment 18 WebKit Review Bot 2013-02-15 17:43:53 PST
Comment on attachment 188668 [details]
Rebased patch

Clearing flags on attachment: 188668

Committed r143077: <http://trac.webkit.org/changeset/143077>
Comment 19 WebKit Review Bot 2013-02-15 17:43:58 PST
All reviewed patches have been landed.  Closing bug.