REGRESSION (r192796) WKBundlePageResourceLoadClient should be able to setHTTPBody in willSendRequestForFrame
Created attachment 266949 [details] Patch
rdar://problem/23763584
Created attachment 266960 [details] Patch
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.
Created attachment 267021 [details] Patch
Comment on attachment 267021 [details] Patch Looks good to me; r- since you are still working on a test.
Created attachment 267042 [details] Patch
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
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
Oops, forgot WK1
Created attachment 267047 [details] Patch
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 ?
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.
Created attachment 267114 [details] Patch
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.
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.
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.
http://trac.webkit.org/changeset/193924