Bug 164631

Summary: REGRESSION: API test _WKDownload.ConvertResponseToDownload is a flaky timeout
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ap, cdumez, cgarcia
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=164599
Bug Depends on:    
Bug Blocks: 164220    
Attachments:
Description Flags
Patch
none
Patch cgarcia: review+

Comment 1 Ryan Haddad 2016-11-10 23:46:12 PST
This is probably related to http://trac.webkit.org/changeset/208521
Comment 2 Ryan Haddad 2016-11-11 16:30:11 PST
Or maybe https://trac.webkit.org/changeset/208522
Comment 3 Chris Dumez 2016-11-11 16:45:21 PST
More likely to be Carlos' http://trac.webkit.org/changeset/208521.
Comment 4 Chris Dumez 2016-11-11 16:48:38 PST
(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.
Comment 5 Alexey Proskuryakov 2016-11-11 23:25:03 PST
Adding REGRESSION to the title, as the discussion above is about recent changes.
Comment 6 Carlos Garcia Campos 2016-11-12 01:28:28 PST
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.
Comment 7 Alex Christensen 2016-11-17 21:00:37 PST
Created attachment 295132 [details]
Patch
Comment 8 Carlos Garcia Campos 2016-11-17 22:55:32 PST
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.
Comment 9 Alex Christensen 2016-11-17 23:14:56 PST
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.
Comment 10 Alex Christensen 2016-11-17 23:16:40 PST
Created attachment 295137 [details]
Patch
Comment 11 Carlos Garcia Campos 2016-11-17 23:20:44 PST
Comment on attachment 295137 [details]
Patch

Cool, much better :-) Thanks!
Comment 12 Alex Christensen 2016-11-17 23:22:33 PST
http://trac.webkit.org/changeset/208880
Comment 13 Chris Dumez 2016-11-18 07:38:44 PST
(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?
Comment 14 Chris Dumez 2016-11-18 16:30:18 PST
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.