Bug 164631 – REGRESSION: API test _WKDownload.ConvertResponseToDownload is a flaky timeout

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

API test _WKDownload.ConvertResponseToDownload is a flaky timeout

https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/builds/9355

https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20(Tests)/builds/16143

https://build.webkit.org/builders/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/builds/9671

[Ryan Haddad]

This is probably related to http://trac.webkit.org/changeset/208521

[Ryan Haddad]

Or maybe https://trac.webkit.org/changeset/208522

[Chris Dumez]

More likely to be Carlos' http://trac.webkit.org/changeset/208521.

[Chris Dumez]

(In reply to comment #0)
> API test _WKDownload.ConvertResponseToDownload is a flaky timeout
> 
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/builds/9355
> 
> https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK2%20(Tests)/
> builds/16143
> 
> https://build.webkit.org/builders/
> Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/builds/9671

Looks like those bots are not using Sierra and therefore not using the NETWORK_SESSION code path.

[Alexey Proskuryakov]

Adding REGRESSION to the title, as the discussion above is about recent changes.

[Carlos Garcia Campos]

But r208521 shouldn't change the behavior for non NetworkSession code path. Maybe the change in NetworkResourceLoader. It could be that the NetworkLoad is deleted earlier now for the non NetworkSession case. When the load is converted to download, we now transfer the NetworkLoad to the download manager that creates the Download. In the case of !NetworkSession the download manager doesn't adopt the network load, so it's deleted after convertNetworkLoadToDownload. After the load is converted to download the web process deletes the resource loader and sends RemoveLoadIdentifier that calls abort on the NetworkResourceLoader. Since the load was converted to download, the abort simply does the cleanup, that used to delete the network load in that case. I don't see how this change would affect the API, though. If the download is cancelled after the network load is deleted, the download manager already has a Download object to cancel it. Maybe the download finishes before the NetworkProcess::CancelDownload message is received.

[Alex Christensen]

Created attachment 295132 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 295132 [details]
Patch

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

Thanks!

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:303
> +#if USE(NETWORK_SESSION)
>      if (m_networkLoad) {
> +#else
> +    if (m_networkLoad && !m_didConvertToDownload) {
> +#endif

I don't understand it, TBH. Why do we need m_didConvertToDownload? We set it to true in NetworkResourceLoader::convertToDownload, right before we do the WTFMove(m_networkLoad) so I would expect m_networkLoad to be always nullptr here when m_didConvertToDownload is true. Maybe we could do a exchange before calling convertNetworkLoadToDownload() where the m_didConvertToDownload is in this patch, and then we don't need all the ifdefs.

[Alex Christensen]

Comment on attachment 295132 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:303
>> +#endif
> 
> I don't understand it, TBH. Why do we need m_didConvertToDownload? We set it to true in NetworkResourceLoader::convertToDownload, right before we do the WTFMove(m_networkLoad) so I would expect m_networkLoad to be always nullptr here when m_didConvertToDownload is true. Maybe we could do a exchange before calling convertNetworkLoadToDownload() where the m_didConvertToDownload is in this patch, and then we don't need all the ifdefs.

std::exchange fixes it too.

[Alex Christensen]

Created attachment 295137 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 295137 [details]
Patch

Cool, much better :-) Thanks!

[Alex Christensen]

http://trac.webkit.org/changeset/208880

[Chris Dumez]

(In reply to comment #12)
> http://trac.webkit.org/changeset/208880

I don't understand, doesn't the WTFMove() already set the member to null?

[Chris Dumez]

Comment on attachment 295137 [details]
Patch

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

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:286
> +    NetworkProcess::singleton().downloadManager().convertNetworkLoadToDownload(downloadID, std::exchange(m_networkLoad, nullptr), WTFMove(m_fileReferences), request, response);

For the record, it seems the issue was that the parameter to convertNetworkLoadToDownload() is a unique_ptr<NetworkResourceLoader>&&, not a unique_ptr<NetworkResourceLoader>. Also the implementation of convertNetworkLoadToDownload() was never constructing a unique_ptr from the parameter. Therefore, we were never calling unique_ptr(unique_ptr&&) which would have nulled out the member. We were only doing a WTFMove() which in itself does not null out the member.