Bug 222856 - REGRESSION (r273541): Payloads requiring NSURL in serialization fail
Summary: REGRESSION (r273541): Payloads requiring NSURL in serialization fail
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-03-05 21:32 PST by Brent Fulgham
Modified: 2021-03-09 11:06 PST (History)
7 users (show)

See Also:


Attachments
Patch (1.86 KB, patch)
2021-03-05 21:36 PST, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2021-03-05 21:32:45 PST
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.
Comment 1 Brent Fulgham 2021-03-05 21:36:48 PST
Created attachment 422473 [details]
Patch
Comment 2 Sam Weinig 2021-03-07 09:03:30 PST
Comment on attachment 422473 [details]
Patch

Can we test this somehow? Perhaps via some ArgumentCoder API/Unit tests?
Comment 3 Brent Fulgham 2021-03-08 11:47:58 PST
(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.
Comment 4 Darin Adler 2021-03-08 14:11:51 PST
Comment on attachment 422473 [details]
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.
Comment 5 Darin Adler 2021-03-08 14:12:42 PST
(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.
Comment 6 EWS 2021-03-08 14:39:22 PST
Committed r274109: <https://commits.webkit.org/r274109>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 422473 [details].
Comment 7 Radar WebKit Bug Importer 2021-03-08 14:40:25 PST
<rdar://problem/75188264>
Comment 8 Brent Fulgham 2021-03-09 11:05:10 PST
(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).
Comment 9 Brent Fulgham 2021-03-09 11:06:03 PST
(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.