164631 2016-11-10 21:43:00 -0800 REGRESSION: API test _WKDownload.ConvertResponseToDownload is a flaky timeout 2016-11-18 16:30:18 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=164599 P2 Normal --- 164220 1 ryanhaddad achristensen achristensen ap cdumez cgarcia oldest_to_newest 1250159 0 ryanhaddad 2016-11-10 21:43:00 -0800 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 1250188 1 ryanhaddad 2016-11-10 23:46:12 -0800 This is probably related to http://trac.webkit.org/changeset/208521 1250493 2 ryanhaddad 2016-11-11 16:30:11 -0800 Or maybe https://trac.webkit.org/changeset/208522 1250511 3 cdumez 2016-11-11 16:45:21 -0800 More likely to be Carlos' http://trac.webkit.org/changeset/208521. 1250515 4 cdumez 2016-11-11 16:48:38 -0800 (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. 1250620 5 ap 2016-11-11 23:25:03 -0800 Adding REGRESSION to the title, as the discussion above is about recent changes. 1250652 6 cgarcia 2016-11-12 01:28:28 -0800 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. 1252596 7 295132 achristensen 2016-11-17 21:00:37 -0800 Created attachment 295132 Patch 1252609 8 295132 cgarcia 2016-11-17 22:55:32 -0800 Comment on attachment 295132 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. 1252611 9 295132 achristensen 2016-11-17 23:14:56 -0800 Comment on attachment 295132 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. 1252612 10 295137 achristensen 2016-11-17 23:16:40 -0800 Created attachment 295137 Patch 1252614 11 295137 cgarcia 2016-11-17 23:20:44 -0800 Comment on attachment 295137 Patch Cool, much better :-) Thanks! 1252615 12 achristensen 2016-11-17 23:22:33 -0800 http://trac.webkit.org/changeset/208880 1252672 13 cdumez 2016-11-18 07:38:44 -0800 (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? 1252939 14 295137 cdumez 2016-11-18 16:30:18 -0800 Comment on attachment 295137 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. 295132 2016-11-17 21:00:37 -0800 2016-11-17 23:16:37 -0800 Patch bug-164631-20161117210031.patch text/plain 3896 achristensen SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwODg3NCkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDI1IEBACisyMDE2LTExLTE3ICBBbGV4IENo cmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUkVHUkVTU0lP TjogQVBJIHRlc3QgX1dLRG93bmxvYWQuQ29udmVydFJlc3BvbnNlVG9Eb3dubG9hZCBpcyBhIGZs YWt5IHRpbWVvdXQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE2NDYzMQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgICogTmV0d29ya1Byb2Nlc3MvTmV0d29ya1Jlc291cmNlTG9hZGVyLmNwcDoKKyAgICAgICAg KFdlYktpdDo6TmV0d29ya1Jlc291cmNlTG9hZGVyOjpjb252ZXJ0VG9Eb3dubG9hZCk6CisgICAg ICAgIChXZWJLaXQ6Ok5ldHdvcmtSZXNvdXJjZUxvYWRlcjo6YWJvcnQpOgorICAgICAgICAqIE5l dHdvcmtQcm9jZXNzL05ldHdvcmtSZXNvdXJjZUxvYWRlci5oOgorICAgICAgICBSZXZlcnQgYSBm dW5jdGlvbmFsIGNoYW5nZSBpbiByMjA4NTIxIHdoaWNoIGNhdXNlZCBkb3dubG9hZHMgdG8gYmUg Y2FuY2VsZWQgaW4gYnVpbGRzIG5vdCB1c2luZyBORVRXT1JLX1NFU1NJT04KKyAgICAgICAgd2hl biB0aGV5IGFyZSBiZWNvbWluZyBhIGRvd25sb2FkIGZyb20gYSBOZXR3b3JrTG9hZC4gIFRoaXMg Y291bGQgYmUgcmVwZWF0ZWQgMTAwJSBvZiB0aGUgdGltZSBieSBhZGRpbmcgYSAKKyAgICAgICAg dXNsZWVwKDEwMDAwMDApIGluIE5ldHdvcmtSZXNvdXJjZUxvYWRlcjo6Y29udmVydFRvRG93bmxv YWQuICBUaGlzIGlzIG5vdCBhIHByb2JsZW0gd2hlbiB1c2luZyBORVRXT1JLX1NFU1NJT04KKyAg ICAgICAgYmVjYXVzZSB0aGUgbGlmZXRpbWUgb2YgYSBkb3dubG9hZCBpcyBzaWduaWZpY2FudGx5 IGRpZmZlcmVudCBhcyB2ZXJpZmllZCBieSB0aGlzIG5vdCBiZWluZyBmbGFreSB3aGVuIHVzaW5n IE5FVFdPUktfU0VTU0lPTiwKKyAgICAgICAgZXZlbiB3aXRoIGFuIGFkZGVkIHVzbGVlcCBjYWxs LgorICAgICAgICBXaGVuIG5vdCB1c2luZyBORVRXT1JLX1NFU1NJT04gb24gQ29jb2EgcGxhdGZv cm1zLCB3ZSBzeW5jaHJvbm91c2x5IGNhbGwgW05TVVJMRG93bmxvYWQgX2Rvd25sb2FkV2l0aExv YWRpbmdDb25uZWN0aW9uLi4uXQorICAgICAgICB0byBjcmVhdGUgYSBkb3dubG9hZC4gIFdoZW4g dXNpbmcgTkVUV09SS19TRVNTSU9OIG9uIENvY29hIHBsYXRmb3JtcyB3ZSBhc3luY2hyb25vdXNs eSBnZXQgdGhlIGNhbGxiYWNrIAorICAgICAgICBVUkxTZXNzaW9uOmRhdGFUYXNrOmRpZEJlY29t ZURvd25sb2FkVGFzazogYWZ0ZXIgYXN5bmNocm9ub3VzbHkgY2FsbGluZyB0aGUgY29tcGxldGlv bkhhbmRsZXIgb2YKKyAgICAgICAgVVJMU2Vzc2lvbjpkYXRhVGFzazpkaWRSZWNlaXZlUmVzcG9u c2U6Y29tcGxldGlvbkhhbmRsZXI6LiAgVGhpcyBOZXR3b3JrTG9hZCB0byBEb3dubG9hZCB0cmFu c2l0aW9uIGJlaW5nIGFzeW5jaHJvbm91cyBpcyB3aHkKKyAgICAgICAgdGhlIGxpZmV0aW1lIG9m IHRoZSBEb3dubG9hZCBpcyBzbyBkaWZmZXJlbnQgd2hlbiB1c2luZyBORVRXT1JLX1NFU1NJT04u CisKIDIwMTYtMTEtMTcgIEpvaG4gV2lsYW5kZXIgIDx3aWxhbmRlckBhcHBsZS5jb20+CiAKICAg ICAgICAgUmVzb3VyY2UgbG9hZCBzdGF0aXN0aWNzOiBDb3ZlciBmdXJ0aGVyIGRhdGEgcmVjb3Jk cywgY291bnQgcmVtb3ZlZCBkYXRhIHJlY29yZHMsIGFuZCBvbmx5IGZpcmUgaGFuZGxlciB3aGVu IG5lZWRlZApJbmRleDogU291cmNlL1dlYktpdDIvTmV0d29ya1Byb2Nlc3MvTmV0d29ya1Jlc291 cmNlTG9hZGVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0Mi9OZXR3b3JrUHJvY2Vz cy9OZXR3b3JrUmVzb3VyY2VMb2FkZXIuY3BwCShyZXZpc2lvbiAyMDg4NjkpCisrKyBTb3VyY2Uv V2ViS2l0Mi9OZXR3b3JrUHJvY2Vzcy9OZXR3b3JrUmVzb3VyY2VMb2FkZXIuY3BwCSh3b3JraW5n IGNvcHkpCkBAIC0yODIsNiArMjgyLDkgQEAgdm9pZCBOZXR3b3JrUmVzb3VyY2VMb2FkZXI6OmNs ZWFudXAoKQogCiB2b2lkIE5ldHdvcmtSZXNvdXJjZUxvYWRlcjo6Y29udmVydFRvRG93bmxvYWQo RG93bmxvYWRJRCBkb3dubG9hZElELCBjb25zdCBSZXNvdXJjZVJlcXVlc3QmIHJlcXVlc3QsIGNv bnN0IFJlc291cmNlUmVzcG9uc2UmIHJlc3BvbnNlKQogeworI2lmICFVU0UoTkVUV09SS19TRVNT SU9OKQorICAgIG1fZGlkQ29udmVydFRvRG93bmxvYWQgPSB0cnVlOworI2VuZGlmCiAgICAgQVNT RVJUKG1fbmV0d29ya0xvYWQpOwogICAgIE5ldHdvcmtQcm9jZXNzOjpzaW5nbGV0b24oKS5kb3du bG9hZE1hbmFnZXIoKS5jb252ZXJ0TmV0d29ya0xvYWRUb0Rvd25sb2FkKGRvd25sb2FkSUQsIFdU Rk1vdmUobV9uZXR3b3JrTG9hZCksIFdURk1vdmUobV9maWxlUmVmZXJlbmNlcyksIHJlcXVlc3Qs IHJlc3BvbnNlKTsKIH0KQEAgLTI5Myw3ICsyOTYsMTEgQEAgdm9pZCBOZXR3b3JrUmVzb3VyY2VM b2FkZXI6OmFib3J0KCkKICAgICBSRUxFQVNFX0xPR19JRl9BTExPV0VEKCJhYm9ydDogQ2FuY2Vs aW5nIHJlc291cmNlIGxvYWQgKHBhZ2VJRCA9ICUiIFBSSXU2NCAiLCBmcmFtZUlEID0gJSIgUFJJ dTY0ICIsIHJlc291cmNlSUQgPSAlIiBQUkl1NjQgIikiLAogICAgICAgICBtX3BhcmFtZXRlcnMu d2ViUGFnZUlELCBtX3BhcmFtZXRlcnMud2ViRnJhbWVJRCwgbV9wYXJhbWV0ZXJzLmlkZW50aWZp ZXIpOwogCisjaWYgVVNFKE5FVFdPUktfU0VTU0lPTikKICAgICBpZiAobV9uZXR3b3JrTG9hZCkg eworI2Vsc2UKKyAgICBpZiAobV9uZXR3b3JrTG9hZCAmJiAhbV9kaWRDb252ZXJ0VG9Eb3dubG9h ZCkgeworI2VuZGlmCiAjaWYgRU5BQkxFKE5FVFdPUktfQ0FDSEUpCiAgICAgICAgIGlmIChjYW5V c2VDYWNoZShtX25ldHdvcmtMb2FkLT5jdXJyZW50UmVxdWVzdCgpKSkgewogICAgICAgICAgICAg Ly8gV2UgbWlnaHQgYWxyZWFkeSBoYXZlIHVzZWQgZGF0YSBmcm9tIHRoaXMgaW5jb21wbGV0ZSBs b2FkLiBFbnN1cmUgb2xkZXIgdmVyc2lvbnMgZG9uJ3QgcmVtYWluIGluIHRoZSBjYWNoZSBhZnRl ciBjYW5jZWwuCkluZGV4OiBTb3VyY2UvV2ViS2l0Mi9OZXR3b3JrUHJvY2Vzcy9OZXR3b3JrUmVz b3VyY2VMb2FkZXIuaAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2ViS2l0Mi9OZXR3b3JrUHJvY2Vz cy9OZXR3b3JrUmVzb3VyY2VMb2FkZXIuaAkocmV2aXNpb24gMjA4ODY5KQorKysgU291cmNlL1dl YktpdDIvTmV0d29ya1Byb2Nlc3MvTmV0d29ya1Jlc291cmNlTG9hZGVyLmgJKHdvcmtpbmcgY29w eSkKQEAgLTE1Myw2ICsxNTMsOSBAQCBwcml2YXRlOgogICAgIGJvb2wgbV9kaWRDb25zdW1lU2Fu ZGJveEV4dGVuc2lvbnMgeyBmYWxzZSB9OwogICAgIGJvb2wgbV9kZWZlcnNMb2FkaW5nIHsgZmFs c2UgfTsKICAgICBib29sIG1faGFzUmVjZWl2ZWREYXRhIHsgZmFsc2UgfTsKKyNpZiAhVVNFKE5F VFdPUktfU0VTU0lPTikKKyAgICBib29sIG1fZGlkQ29udmVydFRvRG93bmxvYWQgeyBmYWxzZSB9 OworI2VuZGlmCiAKICAgICBXZWJDb3JlOjpUaW1lciBtX2J1ZmZlcmluZ1RpbWVyOwogI2lmIEVO QUJMRShORVRXT1JLX0NBQ0hFKQo= 295137 2016-11-17 23:16:40 -0800 2016-11-17 23:20:44 -0800 Patch bug-164631-20161117231634.patch text/plain 1845 achristensen SW5kZXg6IFNvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi S2l0Mi9DaGFuZ2VMb2cJKHJldmlzaW9uIDIwODg3OSkKKysrIFNvdXJjZS9XZWJLaXQyL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBACisyMDE2LTExLTE3ICBBbGV4IENo cmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtpdC5vcmc+CisKKyAgICAgICAgUkVHUkVTU0lP TjogQVBJIHRlc3QgX1dLRG93bmxvYWQuQ29udmVydFJlc3BvbnNlVG9Eb3dubG9hZCBpcyBhIGZs YWt5IHRpbWVvdXQKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dp P2lkPTE2NDYzMQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAg ICAgICogTmV0d29ya1Byb2Nlc3MvTmV0d29ya1Jlc291cmNlTG9hZGVyLmNwcDoKKyAgICAgICAg KFdlYktpdDo6TmV0d29ya1Jlc291cmNlTG9hZGVyOjpjb252ZXJ0VG9Eb3dubG9hZCk6CisgICAg ICAgIEkgY291bGQgcmVwcm9kdWNlIHRoZSBmbGFreSBmYWlsdXJlIDEwMCUgb2YgdGhlIHRpbWUg d2hlbiBub3QgdXNpbmcgTkVUV09SS19TRVNTSU9OCisgICAgICAgIGJ5IGFkZGluZyBhIHVzbGVl cCgxMDAwMDAwKSBpbiBOZXR3b3JrUmVzb3VyY2VMb2FkZXI6OmNvbnZlcnRUb0Rvd25sb2FkLiAg bV9uZXR3b3JrTG9hZAorICAgICAgICB3YXMgbm90IGJlaW5nIHNldCB0byBudWxscHRyIGJlZm9y ZSBpdCB3YXMgYmVpbmcgY2hlY2tlZCBpbiBOZXR3b3JrUmVzb3VyY2VMb2FkZXI6OmFib3J0CisK IDIwMTYtMTEtMTcgIEFsZXggQ2hyaXN0ZW5zZW4gIDxhY2hyaXN0ZW5zZW5Ad2Via2l0Lm9yZz4K IAogICAgICAgICBGaXggQ01ha2UgYnVpbGQgYWZ0ZXIgcjIwODg2NQpJbmRleDogU291cmNlL1dl YktpdDIvTmV0d29ya1Byb2Nlc3MvTmV0d29ya1Jlc291cmNlTG9hZGVyLmNwcAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvV2ViS2l0Mi9OZXR3b3JrUHJvY2Vzcy9OZXR3b3JrUmVzb3VyY2VMb2FkZXIu Y3BwCShyZXZpc2lvbiAyMDg4NjkpCisrKyBTb3VyY2UvV2ViS2l0Mi9OZXR3b3JrUHJvY2Vzcy9O ZXR3b3JrUmVzb3VyY2VMb2FkZXIuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yODMsNyArMjgzLDcg QEAgdm9pZCBOZXR3b3JrUmVzb3VyY2VMb2FkZXI6OmNsZWFudXAoKQogdm9pZCBOZXR3b3JrUmVz b3VyY2VMb2FkZXI6OmNvbnZlcnRUb0Rvd25sb2FkKERvd25sb2FkSUQgZG93bmxvYWRJRCwgY29u c3QgUmVzb3VyY2VSZXF1ZXN0JiByZXF1ZXN0LCBjb25zdCBSZXNvdXJjZVJlc3BvbnNlJiByZXNw b25zZSkKIHsKICAgICBBU1NFUlQobV9uZXR3b3JrTG9hZCk7Ci0gICAgTmV0d29ya1Byb2Nlc3M6 OnNpbmdsZXRvbigpLmRvd25sb2FkTWFuYWdlcigpLmNvbnZlcnROZXR3b3JrTG9hZFRvRG93bmxv YWQoZG93bmxvYWRJRCwgV1RGTW92ZShtX25ldHdvcmtMb2FkKSwgV1RGTW92ZShtX2ZpbGVSZWZl cmVuY2VzKSwgcmVxdWVzdCwgcmVzcG9uc2UpOworICAgIE5ldHdvcmtQcm9jZXNzOjpzaW5nbGV0 b24oKS5kb3dubG9hZE1hbmFnZXIoKS5jb252ZXJ0TmV0d29ya0xvYWRUb0Rvd25sb2FkKGRvd25s b2FkSUQsIHN0ZDo6ZXhjaGFuZ2UobV9uZXR3b3JrTG9hZCwgbnVsbHB0ciksIFdURk1vdmUobV9m aWxlUmVmZXJlbmNlcyksIHJlcXVlc3QsIHJlc3BvbnNlKTsKIH0KIAogdm9pZCBOZXR3b3JrUmVz b3VyY2VMb2FkZXI6OmFib3J0KCkK