crash log for WebProcess (pid <unknown>): STDOUT: <empty> STDERR: ASSERTION FAILED: m_downloads.contains(download->downloadID()) STDERR: /home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebKit2/WebProcess/Downloads/DownloadManager.cpp(76) : void WebKit::DownloadManager::downloadFinished(WebKit::Download*) STDERR: 1 0x7fdd4367c619 WebKit::DownloadManager::downloadFinished(WebKit::Download*) STDERR: 2 0x7fdd4367acbb WebKit::Download::didFail(WebCore::ResourceError const&, CoreIPC::DataReference const&) STDERR: 3 0x7fdd4373cf7b WebKit::DownloadClient::downloadFailed(WebCore::ResourceError const&) STDERR: 4 0x7fdd4373d3d2 WebKit::DownloadClient::didReceiveResponse(WebCore::ResourceHandle*, WebCore::ResourceResponse const&) STDERR: 5 0x7fdd4373c81b WebKit::Download::startWithHandle(WebKit::WebPage*, WebCore::ResourceHandle*, WebCore::ResourceResponse const&) STDERR: 6 0x7fdd4367c4e6 WebKit::DownloadManager::convertHandleToDownload(unsigned long, WebKit::WebPage*, WebCore::ResourceHandle*, WebCore::ResourceRequest const&, WebCore::ResourceResponse const&) STDERR: 7 0x7fdd436e64d4 WebKit::WebFrame::convertHandleToDownload(WebCore::ResourceHandle*, WebCore::ResourceRequest const&, WebCore::ResourceResponse const&) STDERR: 8 0x7fdd436c4ecc WebKit::WebFrameLoaderClient::download(WebCore::ResourceHandle*, WebCore::ResourceRequest const&, WebCore::ResourceResponse const&) STDERR: 9 0x7fdd3fcde7d4 WebCore::MainResourceLoader::continueAfterContentPolicy(WebCore::PolicyAction, WebCore::ResourceResponse const&) STDERR: 10 0x7fdd3fcdecfa WebCore::MainResourceLoader::continueAfterContentPolicy(WebCore::PolicyAction) STDERR: 11 0x7fdd3fcdec34 WebCore::MainResourceLoader::callContinueAfterContentPolicy(void*, WebCore::PolicyAction) STDERR: 12 0x7fdd3fce3666 WebCore::PolicyCallback::call(WebCore::PolicyAction) STDERR: 13 0x7fdd3fce44a3 WebCore::PolicyChecker::continueAfterContentPolicy(WebCore::PolicyAction) STDERR: 14 0x7fdd436e6384 WebKit::WebFrame::didReceivePolicyDecision(unsigned long, WebCore::PolicyAction, unsigned long) STDERR: 15 0x7fdd436c23d8 WebKit::WebFrameLoaderClient::dispatchDecidePolicyForResponse(void (WebCore::PolicyChecker::*)(WebCore::PolicyAction), WebCore::ResourceResponse const&, WebCore::ResourceRequest const&) STDERR: 16 0x7fdd3fce3f5a WebCore::PolicyChecker::checkContentPolicy(WebCore::ResourceResponse const&, void (*)(void*, WebCore::PolicyAction), void*) STDERR: 17 0x7fdd3fcdf304 WebCore::MainResourceLoader::didReceiveResponse(WebCore::ResourceResponse const&) STDERR: 18 0x7fdd3fcf19db WebCore::ResourceLoader::didReceiveResponse(WebCore::ResourceHandle*, WebCore::ResourceResponse const&) STDERR: 19 0x7fdd407e9d5a STDERR: 20 0x7fdd3bd9c8dd g_simple_async_result_complete STDERR: 21 0x7fdd3bd9ca0c STDERR: 22 0x7fdd3c0cee53 g_main_context_dispatch STDERR: 23 0x7fdd3d01223e STDERR: 24 0x7fdd3d00c7b1 STDERR: 25 0x7fdd3d00d245 STDERR: 26 0x7fdd3d00d547 ecore_main_loop_begin STDERR: 27 0x7fdd407d117d WebCore::RunLoop::run() STDERR: 28 0x7fdd4373f1f0 WebProcessMainEfl STDERR: 29 0x4007f4 main STDERR: 30 0x7fdd42d1976d __libc_start_main STDERR: 31 0x400719 STDERR: LEAK: 2 WebPageProxy STDERR: LEAK: 1 WebContext
The actual problem is that with DownloadSoup.cpp implementation, functions WebKit::DownloadManager::convertHandleToDownload() and WebKit::DownloadManager::downloadFinished() are invoked in one call chain when download fails. And this is not supposed to happen by design of DownloadManager. To say in more details: 1) DownloadManager::convertHandleToDownload() is invoked, here is an extract from its body: download->startWithHandle(initiatingPage, handle, response); ASSERT(!m_downloads.contains(downloadID)); m_downloads.set(downloadID, download.leakPtr()); so download->startWithHandle() is invoked BEFORE the download is added to m_downloads. 2) In Download::startWithHandle (implementation for soup) have following workaround: // If the handle already got a response, make sure the download client is notified. ResourceHandleInternal* handleInternal = m_resourceHandle->getInternal(); if (!handleInternal->m_response.isNull()) m_downloadClient->didReceiveResponse(m_resourceHandle.get(), handleInternal->m_response); 3) In case of download failure DownloadClient::didReceiveResponse ends up with WebKit::DownloadManager::downloadFinished() 4) WebKit::DownloadManager::downloadFinished() has the following assertion that hits: ASSERT(m_downloads.contains(download->downloadID())); 5) CRASH
*** Bug 97698 has been marked as a duplicate of this bug. ***
Created attachment 165961 [details] Patch This patch should fix the crash by handling the response in an idle.
Comment on attachment 165961 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165961&action=review > Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:159 > + void handleResponseLater(const ResourceResponse& response) Are you sure this will be invoked? I had very same patch yesterday (however used WebCore::RunLoop::Timer to invoke another call chain) but saw that DownloadClient instance is deleted with the same call chain, so callback was never invoked. > Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:195 > + static_cast<DownloadClient*>(m_downloadClient.get())->handleResponseLater(response); shouldn't we check that response is not null?
(In reply to comment #4) > (From update of attachment 165961 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165961&action=review > > > Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:159 > > + void handleResponseLater(const ResourceResponse& response) > > Are you sure this will be invoked? I had very same patch yesterday (however used WebCore::RunLoop::Timer to invoke another call chain) but saw that > DownloadClient instance is deleted with the same call chain, so callback was never invoked. Yes, I'm sure, I've tried the patch updating the gtk minibrowser to reply with a download on response policy decision when the mime type can't be handled by the web view. > > Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:195 > > + static_cast<DownloadClient*>(m_downloadClient.get())->handleResponseLater(response); > > shouldn't we check that response is not null? We are now using the passed ResourceResponse that is the policy client one, so I don't think it can be Null.
(In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 165961 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=165961&action=review > > > > > Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:159 > > > + void handleResponseLater(const ResourceResponse& response) > > > > Are you sure this will be invoked? I had very same patch yesterday (however used WebCore::RunLoop::Timer to invoke another call chain) but saw that > > DownloadClient instance is deleted with the same call chain, so callback was never invoked. > > Yes, I'm sure, I've tried the patch updating the gtk minibrowser to reply with a download on response policy decision when the mime type can't be handled by the web view. > > > > Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:195 > > > + static_cast<DownloadClient*>(m_downloadClient.get())->handleResponseLater(response); > > > > shouldn't we check that response is not null? > > We are now using the passed ResourceResponse that is the policy client one, so I don't think it can be Null. Ok, thanks. Do you think it's worth also making (In reply to comment #5) > (In reply to comment #4) > > (From update of attachment 165961 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=165961&action=review > > > > > Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:159 > > > + void handleResponseLater(const ResourceResponse& response) > > > > Are you sure this will be invoked? I had very same patch yesterday (however used WebCore::RunLoop::Timer to invoke another call chain) but saw that > > DownloadClient instance is deleted with the same call chain, so callback was never invoked. > > Yes, I'm sure, I've tried the patch updating the gtk minibrowser to reply with a download on response policy decision when the mime type can't be handled by the web view. > > > > Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:195 > > > + static_cast<DownloadClient*>(m_downloadClient.get())->handleResponseLater(response); > > > > shouldn't we check that response is not null? > > We are now using the passed ResourceResponse that is the policy client one, so I don't think it can be Null. Ok, thank you. To my mind it might also worth making handleResponseLaterCallback() and class data members private.
(In reply to comment #3) > Created an attachment (id=165961) [details] > Patch > > This patch should fix the crash by handling the response in an idle. WFM, this fixes the three crashes I was seeing with patch from bug 94515: http/tests/multipart/policy-ignore-crash.php http/tests/multipart/multipart-replace-non-html-content.php http/tests/multipart/load-last-non-html-frame.php (just to be clear, both patches are needed to unskip these tests)
Comment on attachment 165961 [details] Patch LGTM. I'm wondering, though, why don't we need to use this also for Download::start(WebPage*)?
(In reply to comment #8) > (From update of attachment 165961 [details]) > LGTM. I'm wondering, though, why don't we need to use this also for Download::start(WebPage*)? Because in Download::start() we haven't received the response yet, so the client callback will be called normally after Download::start().
Committed r130506: <http://trac.webkit.org/changeset/130506>