Bug 152022 – REGRESSION (r192796) WKBundlePageResourceLoadClient should be able to setHTTPBody in willSendRequestForFrame

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

REGRESSION (r192796) WKBundlePageResourceLoadClient should be able to setHTTPBody in willSendRequestForFrame

[Alex Christensen]

Created attachment 266949 [details]
Patch

[Alex Christensen]

rdar://problem/23763584

[Alex Christensen]

Created attachment 266960 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 266960 [details]
Patch

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

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageResourceLoadClient.cpp:53
> -        request.updateFromDelegatePreservingOldProperties(returnedRequest->resourceRequest());
> +        request = returnedRequest->resourceRequest();

I don't think that we can do this, it will make us drop everything from the request that cannot be round-tripped via an NSURLRequest.

[Alex Christensen]

Created attachment 267021 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 267021 [details]
Patch

Looks good to me; r- since you are still working on a test.

[Alex Christensen]

Created attachment 267042 [details]
Patch

[Build Bot]

Comment on attachment 267042 [details]
Patch

Attachment 267042 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/538383

New failing tests:
http/tests/misc/will-send-request-http-body.html

[Build Bot]

Created attachment 267046 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Alex Christensen]

Oops, forgot WK1

[Alex Christensen]

Created attachment 267047 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 267047 [details]
Patch

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

> Source/WebKit2/Shared/API/c/WKURLRequest.cpp:62
> +WKURLRequestRef WKURLRequestWithHTTPBody(WKURLRequestRef requestRef, WKDataRef body)

This function returns a retained object, so it should be named appropriately.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageResourceLoadClient.cpp:54
> +        RefPtr<FormData> returnedHTTPBody = returnedResourceRequest.httpBody();

What happens if the client tries to set a body stream? That can't work with NetworkProcess, so we should make sure that we fail cleanly (maybe CRASH()?)

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1175
> +            WKRetain(requestWithBody);

This retains it again, so I bet that there is a memory leak.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.h:215
> +    void setWillSendRequestHTTPBody(JSStringRef body) { m_willSendRequestHTTPBody = toWTFString(toWK(body)); }

This is not consistent with the naming of functions above. Maybe something like setWillSendRequestAddsHTTPBody?

> LayoutTests/ChangeLog:3
> +        REGRESSION (r192796) WKBundlePageResourceLoadClient should be able to setHTTPBody in willSendRequestForFrame

Can we also test what happens when the request already has a body, and the client sets it anyway.

> LayoutTests/ChangeLog:9
> +        * http/tests/misc/resources/post-echo.cgi: Copied from http/tests/misc/resources/post-echo-and-notify-done.cgi.

Can we move http/tests/xmlhttprequest/resources/post-echo.cgi?

> LayoutTests/TestExpectations:47
> +http/tests/misc/will-send-request-http-body.html [ Skip ]

will-send-request-with-client-provided-http-body.html ?

[Darin Adler]

Comment on attachment 267047 [details]
Patch

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

>> Source/WebKit2/Shared/API/c/WKURLRequest.cpp:62
>> +WKURLRequestRef WKURLRequestWithHTTPBody(WKURLRequestRef requestRef, WKDataRef body)
> 
> This function returns a retained object, so it should be named appropriately.

Meaning it needs the word Copy or Create in its name.

[Alex Christensen]

Created attachment 267114 [details]
Patch

[Alex Christensen]

I addressed all comments, except http/tests/xmlhttprequest/resources/post-echo.cgi is used by 14 xmlhttprequest tests, so I copied the file rather than change the location of the request in all those tests.

[Darin Adler]

Comment on attachment 267114 [details]
Patch

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

> Source/WebKit2/Shared/API/c/WKURLRequest.cpp:65
> +    requestCopy.setHTTPBody(FormData::create(WKDataGetBytes(body), WKDataGetSize(body)));

Is there a way to do this without copying the data object?

> Source/WebKit2/Shared/API/c/WKURLRequest.h:45
> +WK_EXPORT WKURLRequestRef WKURLRequestCopyWithHTTPBody(WKURLRequestRef, WKDataRef);

I think maybe it should be named CopySettingHTTPBody.

It also seems strange that we are doing this in the C API but not the Objective-C API.

> Source/WebKit2/Shared/API/c/mac/WKURLRequestNS.mm:38
> +    if ([urlRequest HTTPBodyStream])
> +        CRASH();

This is strange. Is a crash really what we want for this? In Objective-C APIs we use exceptions for this sort of thing.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageResourceLoadClient.cpp:54
> +        RefPtr<FormData> returnedHTTPBody = returnedResourceRequest.httpBody();

I think this needs a “why” comment.

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageResourceLoadClient.cpp:57
> +            request.setHTTPBody(returnedHTTPBody.release());

Can we use WTF::move here?

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundlePageResourceLoadClient.cpp:59
>          request = ResourceRequest();

Could change this to { } instead of ResourceRequest(), more terse so nice to read.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1173
> +            CString cBody = body.utf8();

This can fail if the string is not valid UTF-16; I think we’d want to handle that case but maybe not if this is just for testing.

> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1175
> +            WKURLRequestRef requestWithBody = WKURLRequestCopyWithHTTPBody(request, WKDataCreate(reinterpret_cast<const unsigned char*>(cBody.data()), cBody.length()));
> +            return requestWithBody;

Why the local variable here?

> LayoutTests/http/tests/misc/resources/post-echo.cgi:7
> +    read(STDIN, $request, $ENV{'CONTENT_LENGTH'})
> +                || die "Could not get query\n";

Would read nicer as a one-liner.

[Alex Christensen]

Comment on attachment 267114 [details]
Patch

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

>> Source/WebKit2/Shared/API/c/WKURLRequest.cpp:65
>> +    requestCopy.setHTTPBody(FormData::create(WKDataGetBytes(body), WKDataGetSize(body)));
> 
> Is there a way to do this without copying the data object?

Not really right now.  This is the only FormData::create that takes raw data.  This is just for testing, though.

>> Source/WebKit2/Shared/API/c/mac/WKURLRequestNS.mm:38
>> +        CRASH();
> 
> This is strange. Is a crash really what we want for this? In Objective-C APIs we use exceptions for this sort of thing.

I'll return nullptr.

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1173
>> +            CString cBody = body.utf8();
> 
> This can fail if the string is not valid UTF-16; I think we’d want to handle that case but maybe not if this is just for testing.

This is just for testing.

>> Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:1175
>> +            return requestWithBody;
> 
> Why the local variable here?

Not needed.  Will remove.

[Alex Christensen]

http://trac.webkit.org/changeset/193924