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.
Created attachment 187146 [details] The fix (a 2 liner)
Comment on attachment 187146 [details] The fix (a 2 liner) You are gonna need a better excuse to avoid writing a test :)
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.
I vote for the kitten!
Everybody loves kittens!
(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.
Created attachment 188159 [details] Following reviewer's suggestion to test the pointed object
Note how the example declares a variable in the if to avoid repeated calls to webPageProxy->drawingArea()
Created attachment 188170 [details] Using a local pointer variable to save function calls.
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? :)
(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.
> 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 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?
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 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 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
Created attachment 188668 [details] Rebased patch
Comment on attachment 188668 [details] Rebased patch Clearing flags on attachment: 188668 Committed r143077: <http://trac.webkit.org/changeset/143077>
All reviewed patches have been landed. Closing bug.