Bug 222145 – Serialize NSURLRequest (rather than CFURLRequest) in IPC

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

When building on modern Cocoa platforms we should serialize the actual NSURLRequest, rather than bridging to CFURLRequest and serializing that. This allows us to send more state across IPC (since CFURLRequest has been largely unchanged in the past 5+ years and does not reflect all of the features of NSURLRequest).

[Radar WebKit Bug Importer]

<rdar://problem/74500963>

[Brent Fulgham]

Created attachment 420888 [details]
Patch

[Brent Fulgham]

I believe this patch could be extended a bit, since Cache Policy seems to be brought over with the NSURLRequest NSData object. We might be able to skip manually serializing responseContentDispositionEncodingFallbackArray as well.

It looks like 'requester' is not serialized through NSURLRequest (it's probably a WebKit concept).

[Darin Adler]

Comment on attachment 420888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420888&action=review

Looks OK but I think we can do better.

> Source/WebKit/ChangeLog:16
> +        * Shared/mac/WebCoreArgumentCodersMac.mm:

Shouldn't this be in the Cocoa.mm file, not the Mac.mm file? I assume we use it on iOS, not just Mac.

> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:219
> +    NSError *error = nil;
> +    auto *archivedData = [NSKeyedArchiver archivedDataWithRootObject:requestToSerialize.get() requiringSecureCoding:YES error:&error];
> +    if (error)
> +        LOG_ERROR("Failed to encode NSURLRequest: %d", error.code);
> +    IPC::encode(encoder, (__bridge CFDataRef)archivedData);

This should use WebCoreArgumentCodersCocoa.h. I believe it can just be this:

    encode(encoder, requestToSerialize.get());

> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:223
>      encoder << resourceRequest.responseContentDispositionEncodingFallbackArray();

Is this still correct with NSURLRequest serialization?

> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:257
> +    RetainPtr<CFDataRef> data;
> +    if (!IPC::decode(decoder, data) || !data)
> +        return false;
> +
> +    NSError *error = nil;
> +    auto nsURLRequest = retainPtr([NSKeyedUnarchiver unarchivedObjectOfClass:[NSURLRequest class] fromData:(__bridge NSData *)data.get() error:&error]);
> +    if (error) {
> +        LOG_ERROR("Failed to decode NSURLRequest: %d", error.code);
> +        return false;
> +    }

This would just be:

    auto request = decode(decoder, NSURLRequest.class);
    if (!request)
        return false;

[Brent Fulgham]

Comment on attachment 420888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420888&action=review

Thanks for reviewing. I’m going to investigate the api-Mac failure in case it’s related, and see if I can move this implementation to the Cocoa file.

>> Source/WebKit/ChangeLog:16
>> +        * Shared/mac/WebCoreArgumentCodersMac.mm:
> 
> Shouldn't this be in the Cocoa.mm file, not the Mac.mm file? I assume we use it on iOS, not just Mac.

It should! It caused me a lot of confusion initially, but I didn’t want to move too many things in one patch. Maybe I could do a separate change for that.

>> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:223
>>      encoder << resourceRequest.responseContentDispositionEncodingFallbackArray();
> 
> Is this still correct with NSURLRequest serialization?

I don’t think so — I think only “requester” needs to be serialized after this change.

[Alex Christensen]

Comment on attachment 420888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420888&action=review

> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:53
> +#if USE(CFURLCONNECTION)

I think we should just remove this code.  USE(CFURLCONNECTION) is only true on the AppleWin port, which doesn't support WebKit2.

[Brent Fulgham]

Comment on attachment 420888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420888&action=review

>>> Source/WebKit/ChangeLog:16
>>> +        * Shared/mac/WebCoreArgumentCodersMac.mm:
>> 
>> Shouldn't this be in the Cocoa.mm file, not the Mac.mm file? I assume we use it on iOS, not just Mac.
> 
> It should! It caused me a lot of confusion initially, but I didn’t want to move too many things in one patch. Maybe I could do a separate change for that.

Actually, I think moving the NSURLRequest version (and leaving the old CFURLRequest version) makes sense.

>> Source/WebKit/Shared/mac/WebCoreArgumentCodersMac.mm:257
>> +    }
> 
> This would just be:
> 
>     auto request = decode(decoder, NSURLRequest.class);
>     if (!request)
>         return false;

Nice!

[Darin Adler]

Comment on attachment 420888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420888&action=review

>>>> Source/WebKit/ChangeLog:16
>>>> +        * Shared/mac/WebCoreArgumentCodersMac.mm:
>>> 
>>> Shouldn't this be in the Cocoa.mm file, not the Mac.mm file? I assume we use it on iOS, not just Mac.
>> 
>> It should! It caused me a lot of confusion initially, but I didn’t want to move too many things in one patch. Maybe I could do a separate change for that.
> 
> Actually, I think moving the NSURLRequest version (and leaving the old CFURLRequest version) makes sense.

See Alex’s comments that make it clear we can remove the old CFURLRequest version entirely.

[Brent Fulgham]

Created attachment 421017 [details]
More complete change.

[Brent Fulgham]

Created attachment 421020 [details]
Rebaselined

[Brent Fulgham]

Created attachment 421026 [details]
Another try.

[Brent Fulgham]

Created attachment 421027 [details]
Yet another try

[Alex Christensen]

Comment on attachment 421027 [details]
Yet another try

View in context: https://bugs.webkit.org/attachment.cgi?id=421027&action=review

> Tools/ChangeLog:10
> +        and to test the updated IPC serialization for NSURLRequest.:q

:q

> Tools/TestWebKitAPI/Tests/WebKitCocoa/LoadInvalidURLRequest.mm:96
> +        EXPECT_WK_STREQ([error.userInfo[@"NSErrorFailingURLKey"] absoluteString], "http://%E2%80%80");

Note: this test verifies that nothing crashes during this.  The expectation changed surprisingly in https://bugs.webkit.org/show_bug.cgi?id=214314 and this is changing it back, which is probably a good thing.  Important are the facts that this doesn't crash and the URLExtras_ParsingError API test doesn't change expectations.

[Brent Fulgham]

(In reply to Alex Christensen from comment #13)
> Comment on attachment 421027 [details]
> Yet another try
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421027&action=review
> 
> > Tools/ChangeLog:10
> > +        and to test the updated IPC serialization for NSURLRequest.:q
> 
> :q
vi!!!!
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/LoadInvalidURLRequest.mm:96
> > +        EXPECT_WK_STREQ([error.userInfo[@"NSErrorFailingURLKey"] absoluteString], "http://%E2%80%80");
> 
> Note: this test verifies that nothing crashes during this.  The expectation
> changed surprisingly in https://bugs.webkit.org/show_bug.cgi?id=214314 and
> this is changing it back, which is probably a good thing.  Important are the
> facts that this doesn't crash and the URLExtras_ParsingError API test
> doesn't change expectations.

[Brent Fulgham]

Created attachment 421042 [details]
Patch

[Alex Christensen]

Comment on attachment 421042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421042&action=review

I'm assuming you've verified that NSURLRequest does encode responseContentDispositionEncodingFallbackArray, cachePolicy, protocolProperties, expectedContentLength, version, archiveList, and mimeType where CFURLRequest does not.

> Tools/ChangeLog:10
> +        and to test the updated IPC serialization for NSURLRequest.:q

:q

[Brent Fulgham]

Comment on attachment 421042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421042&action=review

>> Tools/ChangeLog:10
>> +        and to test the updated IPC serialization for NSURLRequest.:q
> 
> :q

!!

[Brent Fulgham]

(In reply to Alex Christensen from comment #16)
> Comment on attachment 421042 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421042&action=review
> 
> I'm assuming you've verified that NSURLRequest does encode
> responseContentDispositionEncodingFallbackArray, cachePolicy,
> protocolProperties, expectedContentLength, version, archiveList, and
> mimeType where CFURLRequest does not.

It looks like we definitely do pick up the responseContentDispositionEncodingFallbackArray, cachePolicy, protocolProperties, and version, but I can't find evidence that expectedContentLength and mimeType are picked up. (archiveList is just a detail of our serialization work).

I'll side-serialize expectedContentLength and mimeType.

[Brent Fulgham]

(In reply to Brent Fulgham from comment #18)
> (In reply to Alex Christensen from comment #16)
> > Comment on attachment 421042 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=421042&action=review
> > 
> > I'm assuming you've verified that NSURLRequest does encode
> > responseContentDispositionEncodingFallbackArray, cachePolicy,
> > protocolProperties, expectedContentLength, version, archiveList, and
> > mimeType where CFURLRequest does not.
> 
> It looks like we definitely do pick up the
> responseContentDispositionEncodingFallbackArray, cachePolicy,
> protocolProperties, and version, but I can't find evidence that
> expectedContentLength and mimeType are picked up. (archiveList is just a
> detail of our serialization work).
> 
> I'll side-serialize expectedContentLength and mimeType.

Wait -- no. Those are for NSURLResponse, and were only in that method because it was shared for NSURLResponse serialization.

[Darin Adler]

Comment on attachment 421042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421042&action=review

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:549
> +    RetainPtr<NSURLRequest> requestToSerialize = resourceRequest.nsURLRequest(WebCore::HTTPBodyUpdatePolicy::DoNotUpdateHTTPBody);

I suggest this style:

    auto requestToSerialize = retainPtr(xxx);

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:562
> +        requestToSerialize = adoptNS([requestToSerialize mutableCopy]);
> +        [(NSMutableURLRequest *)requestToSerialize setHTTPBody:nil];
> +        [(NSMutableURLRequest *)requestToSerialize setHTTPBodyStream:nil];

I’d write:

    auto mutableRequest = adoptNS([requestToSerialize mutableCopy]);
    [mutableRequest setHTTPBody:nil];
    [mutableRequest setHTTPBodyStream:nil];
    requestToSerialize = WTFMove(mutableRequest);

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:586
> +    resourceRequest = WebCore::ResourceRequest(request->get());

I would move this down after the last return false.

> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:591
> +    resourceRequest.setRequester(requester);

I would move this down after the last return false.

[Brent Fulgham]

Comment on attachment 421042 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421042&action=review

>> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:549
>> +    RetainPtr<NSURLRequest> requestToSerialize = resourceRequest.nsURLRequest(WebCore::HTTPBodyUpdatePolicy::DoNotUpdateHTTPBody);
> 
> I suggest this style:
> 
>     auto requestToSerialize = retainPtr(xxx);

Will do. I also see some ITP-related errors, so I'm going to do a bit more validation of the serialization to make sure we weren't relying on some idiosyncrasy of the CFURLRequest before landing. I'll be sure to adopt these suggestions, too.

>> Source/WebKit/Shared/Cocoa/WebCoreArgumentCodersCocoa.mm:562
>> +        [(NSMutableURLRequest *)requestToSerialize setHTTPBodyStream:nil];
> 
> I’d write:
> 
>     auto mutableRequest = adoptNS([requestToSerialize mutableCopy]);
>     [mutableRequest setHTTPBody:nil];
>     [mutableRequest setHTTPBodyStream:nil];
>     requestToSerialize = WTFMove(mutableRequest);

Will do.

[Brent Fulgham]

Created attachment 421077 [details]
WIP: Trying to locate test failure.

[Brent Fulgham]

(In reply to Brent Fulgham from comment #22)
> Created attachment 421077 [details]
> WIP: Trying to locate test failure.

I added some assertions to try to understand why the ITP tests would fail with this change.

[Brent Fulgham]

Created attachment 421079 [details]
WIP: Trying to locate test failure.

[Brent Fulgham]

Created attachment 421108 [details]
Logging to identify serialization error.

[Brent Fulgham]

Created attachment 421119 [details]
Additional logging

[Brent Fulgham]

Created attachment 421122 [details]
Now switch serializer and log

[Brent Fulgham]

Created attachment 421338 [details]
Patch

[Darin Adler]

Comment on attachment 421338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421338&action=review

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:115
> +        _URL = [[NSURL _URLWithData:[NSData dataWithBytesNoCopy:bytes length:length freeWhenDone:NO] relativeToURL:nil] retain];

WebCore uses CFURLCreateAbsoluteURLWithBytes. Why is it important to use something different here?

[David Kilzer (:ddkilzer)]

Comment on attachment 421338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421338&action=review

r- to fix leaks.  Nits are optional.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:58
> +@interface NSURL (NSURLExtrasInternal)
> ++ (NSURL *)_URLWithData:(NSData *)data relativeToURL:(NSURL *)baseURL;
>  @end

Nit: Should this be declared in an SPI.h header instead of here?

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:63
> +@interface WKEncodedURL : NSObject <NSSecureCoding>
> +- (instancetype)initWithURL:(NSURL *)url;
> +@property (nonatomic, readonly) NSURL *URL;
> +@end

Nit:  Why use `url` for one variable but `URL` in the other?

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:70
>      if ([object isKindOfClass:[NSFont class]]) {

Nit: Could use dynamic_objc_cast<NSFont>() here.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:78
> +    if ([object isKindOfClass:[NSURL class]])

Nit: Could use dynamic_objc_cast<NSURL>() here.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:82
> +        return [[WKEncodedURL alloc] initWithURL:object];
> +
> +    return object;
> +}

Does this method return a +1 retained object or an (autoreleased/non-retained) object?

If it's meant to return a +1 retained object (probably not based on the method signature—check if the protocol has NS_RETURNS_RETAINED on the definition), then you're under-retaining `object` when returning it here.

If it's meant to return an autoreleased/non-retained object, then you're leaking the WKEncodedURLhere, which should have -autorelease called on it instead to fix the bug.

You probably need to call -autorelease on the WKEncodedURL object (or do the equivalent RetainPtr<> thing if you store the object in a RetainPtr<> via adoptNS()).

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:86
> +    if ([object isKindOfClass:[WKEncodedURL class]])

Nit: Could use dynamic_objc_cast<WKEncodedURL>() here.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:113
> +        auto bytes = (uint8_t *)[coder decodeBytesWithReturnedLength:&length];
> +        if (!bytes)
> +            return nil;

You ned to release `self` here in the early return, otherwise it will leak.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:117
> +        _URL = [[NSURL _URLWithData:[NSData dataWithBytesNoCopy:bytes length:length freeWhenDone:NO] relativeToURL:nil] retain];
> +        if (!_URL)
> +            return nil;

Ditto.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:130
> +@end

You are missing a -dealloc method here to release the `_URL` instance variable, so `_URL` will leak when the WKEncodedURL is released.  (If you do this, don't forget the call to `[super dealloc]` at the end of the method.)

Could also solve this by using a RetainPtr<WKEncodedURL> for the instance variable, along with other syntax changes needed for that, then -dealloc is not needed.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewLoadAPIs.mm:157
> +    [[NSFileManager defaultManager] removeItemAtURL:testFile error:nil];
> +    [[NSFileManager defaultManager] removeItemAtURL:testDirectory error:nil];

Nit: Could pull out `[NSFileManager defaultManager]` into a local variable to save 5 extra Objective-C method calls.

[Brent Fulgham]

Created attachment 421417 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 421417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421417&action=review

r=me (no changes required).  Leaving r? since I assume you want another review.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:64
> -- (id)archiver:(NSKeyedArchiver *)archiver willEncodeObject:(id)object
> +- (id _Nullable)archiver:(NSKeyedArchiver *_Nonnull)archiver willEncodeObject:(id _Nonnull)object

Nit: Does this method signature still match the NSKeyedArchiverDelegate protocol method?  No, it doesn't match exactly (via <Foundation/NSKeyedArchiver.h>):

- (nullable id)archiver:(NSKeyedArchiver *)archiver willEncodeObject:(id)object;

However, it's probably okay if the method signature doesn't match exactly since nullability hints are for the compiler, and the ObjC runtime doesn't care as long as the rest of the signature matches.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:80
> +- (id _Nullable)unarchiver:(NSKeyedUnarchiver * _Nonnull)unarchiver didDecodeObject:(id _Nullable) NS_RELEASES_ARGUMENT object NS_RETURNS_RETAINED

Good catch matching the NSKeyedUnarchiverDelegate protocol definition!

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:84
> +        [encodedURL release];

Nit: The clang static analyzer may be able to reason about this method better if you used this instead:

        [object release];

In practice, it doesn't matter whether you use `object` or `encodedURL`, though.

[Geoffrey Garen]

Comment on attachment 421417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421417&action=review

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:75
> +        return [[[WKEncodedURL alloc] initWithURL:url] autorelease];

I think Chris Dumez would suggest you do adoptNS([[[WKEncodedURL alloc] initWithURL:url]).autorelease() instead. Otherwise, this is tiny step backwards in smart pointer adoption.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:86
> +    if (auto *encodedURL = dynamic_objc_cast<WKEncodedURL>(object)) {
> +        auto url = retainPtr(encodedURL.url);
> +        [encodedURL release];
> +        return url.leakRef();
> +    }

Manual -release is a tiny step backwards in smart pointer adoption.

I'd suggest:

auto objectPtr = adoptNS(object);
if (auto *encodedURL = dynamic_objc_cast<WKEncodedURL>(objectPtr.get()))
    return retainPtr(encodedURL.url).leakRef();

return objectPtr.leakRef();

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:116
> +    if (self = [super init]) {

Let's put self in a RetainPtr to remove manual release.

[Brent Fulgham]

Comment on attachment 421417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421417&action=review

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:64
>> +- (id _Nullable)archiver:(NSKeyedArchiver *_Nonnull)archiver willEncodeObject:(id _Nonnull)object
> 
> Nit: Does this method signature still match the NSKeyedArchiverDelegate protocol method?  No, it doesn't match exactly (via <Foundation/NSKeyedArchiver.h>):
> 
> - (nullable id)archiver:(NSKeyedArchiver *)archiver willEncodeObject:(id)object;
> 
> However, it's probably okay if the method signature doesn't match exactly since nullability hints are for the compiler, and the ObjC runtime doesn't care as long as the rest of the signature matches.

The compiler complained about nullability declarations until I got it to this point. I'm not sure why it's happy with the NSKeyedArchiver.h version and not this one.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:75
>> +        return [[[WKEncodedURL alloc] initWithURL:url] autorelease];
> 
> I think Chris Dumez would suggest you do adoptNS([[[WKEncodedURL alloc] initWithURL:url]).autorelease() instead. Otherwise, this is tiny step backwards in smart pointer adoption.

Oh! Will fix.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:80
>> +- (id _Nullable)unarchiver:(NSKeyedUnarchiver * _Nonnull)unarchiver didDecodeObject:(id _Nullable) NS_RELEASES_ARGUMENT object NS_RETURNS_RETAINED
> 
> Good catch matching the NSKeyedUnarchiverDelegate protocol definition!

I'm learning!

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:84
>> +        [encodedURL release];
> 
> Nit: The clang static analyzer may be able to reason about this method better if you used this instead:
> 
>         [object release];
> 
> In practice, it doesn't matter whether you use `object` or `encodedURL`, though.

I'll use 'object' to help the analyzer.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:86
>> +    }
> 
> Manual -release is a tiny step backwards in smart pointer adoption.
> 
> I'd suggest:
> 
> auto objectPtr = adoptNS(object);
> if (auto *encodedURL = dynamic_objc_cast<WKEncodedURL>(objectPtr.get()))
>     return retainPtr(encodedURL.url).leakRef();
> 
> return objectPtr.leakRef();

Will do.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:116
>> +    if (self = [super init]) {
> 
> Let's put self in a RetainPtr to remove manual release.

Will do.

[Brent Fulgham]

Created attachment 421430 [details]
Patch

[Darin Adler]

Comment on attachment 421417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421417&action=review

>>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:64
>>> +- (id _Nullable)archiver:(NSKeyedArchiver *_Nonnull)archiver willEncodeObject:(id _Nonnull)object
>> 
>> Nit: Does this method signature still match the NSKeyedArchiverDelegate protocol method?  No, it doesn't match exactly (via <Foundation/NSKeyedArchiver.h>):
>> 
>> - (nullable id)archiver:(NSKeyedArchiver *)archiver willEncodeObject:(id)object;
>> 
>> However, it's probably okay if the method signature doesn't match exactly since nullability hints are for the compiler, and the ObjC runtime doesn't care as long as the rest of the signature matches.
> 
> The compiler complained about nullability declarations until I got it to this point. I'm not sure why it's happy with the NSKeyedArchiver.h version and not this one.

The one in NSKeyedArchiver.h is written differently because it's inside a NS_ASSUME_NONNULL_BEGIN/NS_ASSUME_NONNULL_END pair, but it means the same as what’s written above.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:111
> +    WTF::URLCharBuffer urlBytes;
> +    WTF::getURLBytes((__bridge CFURLRef)m_url.get(), urlBytes);
> +    [coder encodeBytes:urlBytes.data() length:urlBytes.size()];

This doesn’t handle the case where m_url is null; seems strange to go out of our way always to say it’s nullable but then not handle the null value.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:124
> +        m_url = retainPtr(WTF::URLWithData([NSData dataWithBytesNoCopy:bytes length:length freeWhenDone:NO], nil));

Should not need to write retainPtr here. Assigning into a RetainPtr should work without that.

I think we should avoid creating and autoreleasing an NSData object here by making a function like URLWithData that takes a pointer and a length; it would be very easy to refactor that function to work without an object. Could do that as a follow-up refactor to improve efficiency; doesn’t seem necessary to do it in the first cut.

[Brent Fulgham]

Created attachment 421456 [details]
Patch

[Brent Fulgham]

Comment on attachment 421417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421417&action=review

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:111
>> +    [coder encodeBytes:urlBytes.data() length:urlBytes.size()];
> 
> This doesn’t handle the case where m_url is null; seems strange to go out of our way always to say it’s nullable but then not handle the null value.

Ah! Okay -- I will address that. I uploaded another patch before I saw these comments, so I will upload another patch soon.

[Brent Fulgham]

Comment on attachment 421417 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421417&action=review

>>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:111
>>> +    [coder encodeBytes:urlBytes.data() length:urlBytes.size()];
>> 
>> This doesn’t handle the case where m_url is null; seems strange to go out of our way always to say it’s nullable but then not handle the null value.
> 
> Ah! Okay -- I will address that. I uploaded another patch before I saw these comments, so I will upload another patch soon.

Actually, after playing with this a bit I think we should mark the URL as _Nonnull. The serialization doesn't expect to encounter null URLs (we always serialize an empty string URL), and the result of this serialization is passed to CFURL functions that do not expect a null URL.

[Darin Adler]

(In reply to Brent Fulgham from comment #39)
> Actually, after playing with this a bit I think we should mark the URL as
> _Nonnull. The serialization doesn't expect to encounter null URLs (we always
> serialize an empty string URL), and the result of this serialization is
> passed to CFURL functions that do not expect a null URL.

Excellent.

[Brent Fulgham]

Created attachment 421470 [details]
Patch

[Darin Adler]

Comment on attachment 421470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421470&action=review

Looks good. I still have style comments (all optional, but most are good ideas), and one comment that *might* be a correctness issue, the return value of initWithCoder: when decoding fails.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:58
> +- (instancetype _Nullable)initWithURL:(NSURL *_Nonnull)url;

On reflection after reading multiple iterations of this patch, it seems to me we want to wrap the contents of this file into NS_ASSUME_NONNULL_BEGIN/END. We then would only have to annotate nullable things rather than also having to annotate all the non-null things.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:59
> +@property (nonatomic, readonly) NSURL *_Nonnull url;

Could avoid the lowercase url by naming this wrappedURL (or targetURL, or underlyingURL).

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:67
> +    if (auto *font = dynamic_objc_cast<NSFont>(object)) {

I slightly prefer:

    if (auto font = xxx

Maybe we could debate the merits?

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:74
> +    if (auto *url = dynamic_objc_cast<NSURL>(object))

Ditto.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:82
> +    auto objectPtr = adoptNS(object);

Not sure objectPtr is a great name. After all "object" is already a pointer. Maybe adoptedObject?

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:83
> +    if (auto *encodedURL = dynamic_objc_cast<WKEncodedURL>(objectPtr.get()))

Same comment about auto.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:91
> +@implementation WKEncodedURL {

I don’t think "encoded URL" is a great name for this. The URL in this object is not "encoded". This is a proxy that facilitates encoding and decoding the URL. It does in fact function as the "decoded URL" in the decoding process, but during the encoding process it is not at all the "encoded" URL.

I would name this class something like "WKURLArchivalWrapper" or "WKSecureCodingURLWrapper".

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:92
> +    RetainPtr<NSURL> m_url;

Could avoid the lowercase url by naming this m_wrappedURL (or m_targetURL or m_underlyingURL).

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:125
> +            m_url = retainPtr([NSURL URLWithString:@""]);

Should not need to call retainPtr here.

But also shouldn’t we return nil here, letting self get released, instead of setting the URL to ""? We want to correctly indicate failure rather than emptying out the URL and incorrectly indicating success.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:196
> +static inline bool isSerializableFont(CTFontRef _Nullable font)

What determines where we use "nullable" and where we use "_Nullable"?

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:441
> +    auto allowedClassSet = [NSMutableSet setWithArray:allowedClasses];

Could remove one autorelease by doing this instead:

    auto allowedClassSet = adoptNS([[NSMutableSet alloc] initWithArray:allowedClasses]);

[Brent Fulgham]

Comment on attachment 421470 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421470&action=review

Thanks for the review!

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:58
>> +- (instancetype _Nullable)initWithURL:(NSURL *_Nonnull)url;
> 
> On reflection after reading multiple iterations of this patch, it seems to me we want to wrap the contents of this file into NS_ASSUME_NONNULL_BEGIN/END. We then would only have to annotate nullable things rather than also having to annotate all the non-null things.

It definitely makes the patch smaller -- I'll do so.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:67
>> +    if (auto *font = dynamic_objc_cast<NSFont>(object)) {
> 
> I slightly prefer:
> 
>     if (auto font = xxx
> 
> Maybe we could debate the merits?

No objections!

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:82
>> +    auto objectPtr = adoptNS(object);
> 
> Not sure objectPtr is a great name. After all "object" is already a pointer. Maybe adoptedObject?

Will do.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:83
>> +    if (auto *encodedURL = dynamic_objc_cast<WKEncodedURL>(objectPtr.get()))
> 
> Same comment about auto.

Done.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:91
>> +@implementation WKEncodedURL {
> 
> I don’t think "encoded URL" is a great name for this. The URL in this object is not "encoded". This is a proxy that facilitates encoding and decoding the URL. It does in fact function as the "decoded URL" in the decoding process, but during the encoding process it is not at all the "encoded" URL.
> 
> I would name this class something like "WKURLArchivalWrapper" or "WKSecureCodingURLWrapper".

I'll use WKSecureCodingURLWrapper.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:92
>> +    RetainPtr<NSURL> m_url;
> 
> Could avoid the lowercase url by naming this m_wrappedURL (or m_targetURL or m_underlyingURL).

Done. m_wrappedURL.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:125
>> +            m_url = retainPtr([NSURL URLWithString:@""]);
> 
> Should not need to call retainPtr here.
> 
> But also shouldn’t we return nil here, letting self get released, instead of setting the URL to ""? We want to correctly indicate failure rather than emptying out the URL and incorrectly indicating success.

Good point. I'll do that.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:134
> +        m_url = retainPtr(url);

Based on your other comments it occurs to me that I shouldn't do a retainPtr() here.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:196
>> +static inline bool isSerializableFont(CTFontRef _Nullable font)
> 
> What determines where we use "nullable" and where we use "_Nullable"?

I based it on what the compiler accepted. It seems like "nullable" is allowed in property declarations, but function signatures need _Nullable. But I might be mistaken.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:441
>> +    auto allowedClassSet = [NSMutableSet setWithArray:allowedClasses];
> 
> Could remove one autorelease by doing this instead:
> 
>     auto allowedClassSet = adoptNS([[NSMutableSet alloc] initWithArray:allowedClasses]);

Oh! Good catch.

[Brent Fulgham]

Created attachment 421486 [details]
Patch

[Brent Fulgham]

(In reply to Darin Adler from comment #42)
> Comment on attachment 421470 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=421470&action=review
> 
> Looks good. I still have style comments (all optional, but most are good
> ideas), and one comment that *might* be a correctness issue, the return
> value of initWithCoder: when decoding fails.

Just to complicate things, I took another look at how CFURL had been serialized, and tried to more closely follow that model (and how it's done in Foundation). I now think that this accurately models how URL's are serialized both in WebKit, and in Foundation, and should now be correct.

Let's see whether the bots find anything.

[Darin Adler]

Comment on attachment 421486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421486&action=review

> Source/WTF/wtf/cocoa/NSURLExtras.mm:169
> +            // So only return early if the character after the slash is somethihg else.

Typo in the word "something" here.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:60
> +- (instancetype _Nullable)initWithURL:(NSURL *)url;

Could use the wrappedURL name here too, to make it look nicer.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:76
> +    if (auto url = dynamic_objc_cast<NSURL>(object))

Could use the wrappedURL name here too, to make it look nicer.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:107
> +NSString * const baseURLExistsKey = @"WK.baseURLExists";

This is not used in the code below. If this was marked static, you would have gotten a compiler warning about it being unused.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:108
> +NSString * const baseURLKey = @"WK.baseURL";

Since this is limited to the file it should have static. Could use constexpr for this. The syntax is nicer:

    static constexpr NSString *baseURLKey = @"WK.baseURL";

Might also not even need "static" with constexpr, but I think it’s correct to do both.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:128
> +    auto selfPtr = adoptNS([super init]);
> +    if (selfPtr) {

Can just write this instead of nesting the entire function:

    auto selfPtr = adoptNS([super init]);
    if (!selfPtr)
        return nil;

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:134
> +            baseURL = (NSURL *)[coder decodeObjectOfClass:NSURL.class forKey:baseURLKey];

Does this end up recursively using this method?

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:472
>          [unarchiver finishDecoding];
> +        unarchiver.get().delegate = nil;

Does this code *actually* run before the function returns even though we have return inside the @try and @catch?

[Brent Fulgham]

Comment on attachment 421486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421486&action=review

>> Source/WTF/wtf/cocoa/NSURLExtras.mm:169
>> +            // So only return early if the character after the slash is somethihg else.
> 
> Typo in the word "something" here.

Doh!

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:60
>> +- (instancetype _Nullable)initWithURL:(NSURL *)url;
> 
> Could use the wrappedURL name here too, to make it look nicer.

Will do.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:76
>> +    if (auto url = dynamic_objc_cast<NSURL>(object))
> 
> Could use the wrappedURL name here too, to make it look nicer.

Good idea. And I should use "wrapper" instead of "encodedURL" below.

> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:86
> +        return retainPtr(encodedURL.wrappedURL).leakRef();

... so this becomes "wrapper.wrappedURL", which is more precise.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:107
>> +NSString * const baseURLExistsKey = @"WK.baseURLExists";
> 
> This is not used in the code below. If this was marked static, you would have gotten a compiler warning about it being unused.

Oh! yes, I thought I would need to use this but didn't. I'll remove it.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:108
>> +NSString * const baseURLKey = @"WK.baseURL";
> 
> Since this is limited to the file it should have static. Could use constexpr for this. The syntax is nicer:
> 
>     static constexpr NSString *baseURLKey = @"WK.baseURL";
> 
> Might also not even need "static" with constexpr, but I think it’s correct to do both.

Will do.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:128
>> +    if (selfPtr) {
> 
> Can just write this instead of nesting the entire function:
> 
>     auto selfPtr = adoptNS([super init]);
>     if (!selfPtr)
>         return nil;

Will do.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:134
>> +            baseURL = (NSURL *)[coder decodeObjectOfClass:NSURL.class forKey:baseURLKey];
> 
> Does this end up recursively using this method?

This is the algorithm used by Foundation to serialize these objects, so I believe it is correct. I will add another TestWebKitAPI test that confirms we terminate.

>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:472
>> +        unarchiver.get().delegate = nil;
> 
> Does this code *actually* run before the function returns even though we have return inside the @try and @catch?

Yes -- I confirmed this in the debugger before putting it here. I originally set the delegate to nil in the @try and @catch blocks, but found that serialization failed because 'finishDecoding' was unsuccessful without the delegate (which had been cleared before reaching @finally).

[Darin Adler]

Comment on attachment 421486 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421486&action=review

>>> Source/WebKit/Shared/Cocoa/ArgumentCodersCocoa.mm:134
>>> +            baseURL = (NSURL *)[coder decodeObjectOfClass:NSURL.class forKey:baseURLKey];
>> 
>> Does this end up recursively using this method?
> 
> This is the algorithm used by Foundation to serialize these objects, so I believe it is correct. I will add another TestWebKitAPI test that confirms we terminate.

Sure, and looks good, but want to make sure that it does correctly end up using this function. I don’t think it will infinitely recurse because eventually we will have a URL without a base.

[Brent Fulgham]

Created attachment 421553 [details]
Patch for landing

[EWS]

Committed r273503: <https://commits.webkit.org/r273503>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421553 [details].

[WebKit Commit Bot]

Re-opened since this is blocked by bug 222442

[Brent Fulgham]

Created attachment 421592 [details]
Patch to address Blob test

[Brent Fulgham]

Comment on attachment 421592 [details]
Patch to address Blob test

Sadly, WTF::URLFromBytes() doesn't handle URL fragments (e.g., "/action" from a Form) properly, so I'm switching to the behavior of ArgumentCodersCF (which I should have done from the beginning).

If EWS is green across all tests, I'll land.

[EWS]

Committed r273541: <https://commits.webkit.org/r273541>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421592 [details].