Bug 87953 – [SOUP] WebProcess crashes when a download is started from an existing ResourceHandle

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Carlos Garcia Campos]

The output stream to write the downloaded data is created in the didReceiveResponse callback of the download client. When a download is created for an existing ResourceHandle (this happens for example when policy decision is download), the response has already been received. In this case we should make sure that the download client is notified about the response.

[Carlos Garcia Campos]

Created attachment 145045 [details]
Patch

[Martin Robinson]

Comment on attachment 145045 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145045&action=review

> Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:154
> +    // 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);

Is there a chance that data has already been received or the request already failed as well? If that was the case then we'd have to forward those events. Or is the resource handle paused during policy decisions?

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 145045 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145045&action=review
> 
> > Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:154
> > +    // 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);
> 
> Is there a chance that data has already been received or the request already failed as well? If that was the case then we'd have to forward those events. Or is the resource handle paused during policy decisions?

Policy decisions happen before data is sent, when the response has been received. If it fails, it's not converted to a download.

[Carlos Garcia Campos]

Committed r119107: <http://trac.webkit.org/changeset/119107>

[Martin Robinson]

Comment on attachment 145045 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145045&action=review

>>> Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:154
>>> +        m_downloadClient->didReceiveResponse(m_resourceHandle.get(), handleInternal->m_response);
>> 
>> Is there a chance that data has already been received or the request already failed as well? If that was the case then we'd have to forward those events. Or is the resource handle paused during policy decisions?
> 
> Policy decisions happen before data is sent, when the response has been received. If it fails, it's not converted to a download.

The download client still handles didReceiveData and didFail though...

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 145045 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145045&action=review
> 
> >>> Source/WebKit2/WebProcess/Downloads/soup/DownloadSoup.cpp:154
> >>> +        m_downloadClient->didReceiveResponse(m_resourceHandle.get(), handleInternal->m_response);
> >> 
> >> Is there a chance that data has already been received or the request already failed as well? If that was the case then we'd have to forward those events. Or is the resource handle paused during policy decisions?
> > 
> > Policy decisions happen before data is sent, when the response has been received. If it fails, it's not converted to a download.
> 
> The download client still handles didReceiveData and didFail though...

once the download client has been set as the handle client, if it fails, it will be handled by the download, and of course didReceiveData is handled to write the downloaded data.

[Mikhail Pozdnyakov]

Could you please clarify what kind of crashes there used to be? Layout test names, crash logs..

The landed patch caused crash of fast/loader/reload-zero-byte-plugin.html on WK2 EFL (and most probably on WK2 GTK as well). Please take a look at 
https://bugs.webkit.org/show_bug.cgi?id=97565

[Carlos Garcia Campos]

(In reply to comment #7)
> Could you please clarify what kind of crashes there used to be? Layout test names, crash logs..
> 
> The landed patch caused crash of fast/loader/reload-zero-byte-plugin.html on WK2 EFL (and most probably on WK2 GTK as well). Please take a look at 
> https://bugs.webkit.org/show_bug.cgi?id=97565

The problem was that the output stream is created when handling the response, so if we receive data and and the response hasn't been handled, there's no output stream to write to. When the ResourceHandle is converted into a download, typically by the policy client, the response has already been received, but the download client hasn't been notified because the download is created after the response is received. So it always crashed when a download was started by the policy client for example.