Bug 97565 – [WK2][SOUP] Assertion hit in WebKit::DownloadManager::downloadFinished() when download fails

• Component: WebKit EFL
• Status: RESOLVED FIXED

[Raphael Kubo da Costa (:rakuco)]

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

[Mikhail Pozdnyakov]

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

[Jussi Kukkonen (jku)]

*** Bug 97698 has been marked as a duplicate of this bug. ***

[Carlos Garcia Campos]

Created attachment 165961 [details]
Patch

This patch should fix the crash by handling the response in an idle.

[Mikhail Pozdnyakov]

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?

[Carlos Garcia Campos]

(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.

[Mikhail Pozdnyakov]

(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.

[Jussi Kukkonen (jku)]

(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)

[Gustavo Noronha (kov)]

Comment on attachment 165961 [details]
Patch

LGTM. I'm wondering, though, why don't we need to use this also for Download::start(WebPage*)?

[Carlos Garcia Campos]

(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().

[Carlos Garcia Campos]

Committed r130506: <http://trac.webkit.org/changeset/130506>