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
This is probably related to http://trac.webkit.org/changeset/208521
Or maybe https://trac.webkit.org/changeset/208522
More likely to be Carlos' http://trac.webkit.org/changeset/208521.
(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.
Adding REGRESSION to the title, as the discussion above is about recent changes.
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.
Created attachment 295132 [details] Patch
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 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.
Created attachment 295137 [details] Patch
Comment on attachment 295137 [details] Patch Cool, much better :-) Thanks!
http://trac.webkit.org/changeset/208880
(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 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.