222856 2021-03-05 21:32:45 -0800 REGRESSION (r273541): Payloads requiring NSURL in serialization fail 2021-03-09 11:06:03 -0800 1 1 1 Unclassified WebKit WebKit Misc. WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=222957 InRadar P2 Normal --- 1 bfulgham bfulgham achristensen aestes bfulgham darin ggaren sam webkit-bug-importer oldest_to_newest 1736748 0 bfulgham 2021-03-05 21:32:45 -0800 In Bug 222145 I converted serialization od CFURLRequest to NSURLRequest. As part of that change, I used a new encoder (WKSecureCodingURLWrapper) to wrap the NSURL to ensure proper handling of encoded URLs. I should have declared the WKSecureCodingURLWrapper as a subclass of NSURL, so that NSSecureCoding would respect it as a valid way of serializing an NSURL. This patch corrects that oversight. 1736749 1 422473 bfulgham 2021-03-05 21:36:48 -0800 Created attachment 422473 Patch 1736929 2 422473 sam 2021-03-07 09:03:30 -0800 Comment on attachment 422473 Patch Can we test this somehow? Perhaps via some ArgumentCoder API/Unit tests? 1737195 3 bfulgham 2021-03-08 11:47:58 -0800 (In reply to Sam Weinig from comment #2) > Comment on attachment 422473 [details] > Patch > > Can we test this somehow? Perhaps via some ArgumentCoder API/Unit tests? We could, if such infrastructure existed. It doesn't appear to. We would have caught this if there were tests for Apple Pay PaymentSetupConfiguration, but there don't appear to be any. It might require special server stuff to do so. 1737289 4 422473 darin 2021-03-08 14:11:51 -0800 Comment on attachment 422473 Patch It’s bad that there is no test. It’s non-obvious that this is required. It’s strange that all the fields of NSURL are allocated and all the methods of NSURL are available on this object. But we should land this code quickly to fix the major regression and think about those other things afterward. 1737290 5 darin 2021-03-08 14:12:42 -0800 (In reply to Brent Fulgham from comment #3) > > Can we test this somehow? Perhaps via some ArgumentCoder API/Unit tests? > > We could, if such infrastructure existed. It doesn't appear to. > > We would have caught this if there were tests for Apple Pay > PaymentSetupConfiguration, but there don't appear to be any. It might > require special server stuff to do so. I feel like we need to add this testing. It’s not optional. Every test we wrote was hard at first, time to make this testable. 1737299 6 ews-feeder 2021-03-08 14:39:22 -0800 Committed r274109: <https://commits.webkit.org/r274109> All reviewed patches have been landed. Closing bug and clearing flags on attachment 422473. 1737301 7 webkit-bug-importer 2021-03-08 14:40:25 -0800 <rdar://problem/75188264> 1737669 8 bfulgham 2021-03-09 11:05:10 -0800 (In reply to Sam Weinig from comment #2) > Comment on attachment 422473 [details] > Patch > > Can we test this somehow? Perhaps via some ArgumentCoder API/Unit tests? I got started trying to make this work in Bug 222957. Maybe you could take a look (well, once I get the tvOS/watchOS stuff fixed). 1737670 9 bfulgham 2021-03-09 11:06:03 -0800 (In reply to Darin Adler from comment #5) > (In reply to Brent Fulgham from comment #3) > > > Can we test this somehow? Perhaps via some ArgumentCoder API/Unit tests? > > > > We could, if such infrastructure existed. It doesn't appear to. > > > > We would have caught this if there were tests for Apple Pay > > PaymentSetupConfiguration, but there don't appear to be any. It might > > require special server stuff to do so. > > I feel like we need to add this testing. It’s not optional. Every test we > wrote was hard at first, time to make this testable. I wrote a small test that exercises the specific Apple Pay thing that broke. I'm going to work with Andy/Alex to get something more full-featured in place. See Bug 222957. 422473 2021-03-05 21:36:48 -0800 2021-03-08 14:39:23 -0800 Patch bug-222856-20210305213647.patch text/plain 1906 bfulgham U3VidmVyc2lvbiBSZXZpc2lvbjogMjc0MDIxCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L0No YW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCmluZGV4IGM4NGE1NGI0M2MxODQ4OGMw Njc2MzkzMTdmYmU1ODMwYmE3YjhiYjcuLjNmMWI0ZGE4MjhjY2IyNGUzZmJmYTVkNWQ5ODdkNWQ1 ODU5NGZkOWUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvQ2hhbmdlTG9nCisrKyBiL1NvdXJj ZS9XZWJLaXQvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjIgQEAKKzIwMjEtMDMtMDUgIEJyZW50IEZ1 bGdoYW0gIDxiZnVsZ2hhbUBhcHBsZS5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTiAocjI3MzU0 MSk6IFBheWxvYWRzIHJlcXVpcmluZyBOU1VSTCBpbiBzZXJpYWxpemF0aW9uIGZhaWwgCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMjI4NTYKKyAgICAg ICAgPHJkYXI6Ly9wcm9ibGVtLzc1MDM2NDUyPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIEluIEJ1ZyAyMjIxNDUgSSBjb252ZXJ0ZWQgc2VyaWFsaXph dGlvbiBvZiBDRlVSTFJlcXVlc3QgdG8gTlNVUkxSZXF1ZXN0LiBBcyBwYXJ0IG9mIHRoYXQgY2hh bmdlLAorICAgICAgICBJIHVzZWQgYSBuZXcgZW5jb2RlciAoV0tTZWN1cmVDb2RpbmdVUkxXcmFw cGVyKSB0byB3cmFwIHRoZSBOU1VSTCB0byBlbnN1cmUgcHJvcGVyIGhhbmRsaW5nIG9mCisgICAg ICAgIGVuY29kZWQgVVJMcy4KKworICAgICAgICBUaGUgV0tTZWN1cmVDb2RpbmdVUkxXcmFwcGVy IHNob3VsZCBoYXZlIGJlZW4gZGVjbGFyZWQgYXMgYSBzdWJjbGFzcyBvZiBOU1VSTCwgc28gdGhh dCBOU1NlY3VyZUNvZGluZworICAgICAgICB3b3VsZCByZXNwZWN0IGl0IGFzIGEgdmFsaWQgd2F5 IG9mIHNlcmlhbGl6aW5nIGFuIE5TVVJMLgorCisgICAgICAgIFRoaXMgcGF0Y2ggY29ycmVjdHMg dGhhdCBvdmVyc2lnaHQuCisKKyAgICAgICAgKiBTaGFyZWQvQ29jb2EvQXJndW1lbnRDb2RlcnND b2NvYS5tbToKKwogMjAyMS0wMy0wNSAgQ2hyaXMgRHVtZXogIDxjZHVtZXpAYXBwbGUuY29tPgog CiAgICAgICAgIE1ha2UgSFRNTE1lZGlhRWxlbWVudDo6Y2xlYXJNZWRpYUNhY2hlRm9yT3JpZ2lu cygpIHRha2UgaW4gU2VjdXJpdHlPcmlnaW5EYXRhIG9iamVjdHMKZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJLaXQvU2hhcmVkL0NvY29hL0FyZ3VtZW50Q29kZXJzQ29jb2EubW0gYi9Tb3VyY2UvV2Vi S2l0L1NoYXJlZC9Db2NvYS9Bcmd1bWVudENvZGVyc0NvY29hLm1tCmluZGV4IDQyNDVhYWY2MmIy NzNiYmYyOGU3ODc2NzIxNjE4MjM5NzE1MWZmYTkuLmNiOTJlMTM1YjAxZWRlYTVmZTcyMGU3ODkz NjJlZTgzNTBhZTgyMTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvU2hhcmVkL0NvY29hL0Fy Z3VtZW50Q29kZXJzQ29jb2EubW0KKysrIGIvU291cmNlL1dlYktpdC9TaGFyZWQvQ29jb2EvQXJn dW1lbnRDb2RlcnNDb2NvYS5tbQpAQCAtNTYsNyArNTYsNyBAQCBOU19BU1NVTUVfTk9OTlVMTF9C RUdJTgogQGludGVyZmFjZSBXS1NlY3VyZUNvZGluZ0FyY2hpdmluZ0RlbGVnYXRlIDogTlNPYmpl Y3QgPE5TS2V5ZWRBcmNoaXZlckRlbGVnYXRlLCBOU0tleWVkVW5hcmNoaXZlckRlbGVnYXRlPgog QGVuZAogCi1AaW50ZXJmYWNlIFdLU2VjdXJlQ29kaW5nVVJMV3JhcHBlciA6IE5TT2JqZWN0IDxO U1NlY3VyZUNvZGluZz4KK0BpbnRlcmZhY2UgV0tTZWN1cmVDb2RpbmdVUkxXcmFwcGVyIDogTlNV UkwgPE5TU2VjdXJlQ29kaW5nPgogLSAoaW5zdGFuY2V0eXBlIF9OdWxsYWJsZSlpbml0V2l0aFVS TDooTlNVUkwgKil3cmFwcGVkVVJMOwogQHByb3BlcnR5IChub25hdG9taWMsIHJlYWRvbmx5KSBO U1VSTCAqIHdyYXBwZWRVUkw7CiBAZW5kCg==