We should support with CORS-preflighting on redirects.
<rdar://problem/33801370>
Created attachment 317714 [details] WIP Patch
Created attachment 317721 [details] Patch
Created attachment 317733 [details] Patch
Created attachment 317734 [details] Patch
Comment on attachment 317734 [details] Patch The only question is whether this is fine to do so without going through CSP or content blocker. Given this is a replacement over synchronous XHR, and that shipping this might allow us to remove synchronous XHR, this looks like an improvement to me. View in context: https://bugs.webkit.org/attachment.cgi?id=317734&action=review > Source/WebKit/NetworkProcess/PingLoad.cpp:46 > + , m_sameOriginRequest(securityOrigin().canRequest(m_parameters.request.url())) m_isSameOriginRequest might be a better name. > Source/WebKit/NetworkProcess/PingLoad.cpp:61 > + m_redirectHandler({ }); Why do we need to do that? > Source/WebKit/NetworkProcess/PingLoad.cpp:75 > + loadParameters.request = request; Aren't we creating two ResourceRequest, while we should only create one? > Source/WebKit/NetworkProcess/PingLoad.cpp:76 > + m_task = NetworkDataTask::create(*networkSession, *this, loadParameters); And we are not moving loadParameters, which probably means that NetworkDataTask is creating its own copy of ResourceRequest. Some refactoring might be useful there? > Source/WebKit/NetworkProcess/PingLoad.cpp:102 > + // Use a unique for subsequent loads if needed. Use a unique origin? > Source/WebKit/NetworkProcess/PingLoad.cpp:106 > + m_origin = SecurityOrigin::createUnique(); You could add a check so as to not create a new origin if m_origin is not unique. > LayoutTests/http/wpt/beacon/cors/cors-preflight-redirect-failure.html:21 > + response.text().then(body => { You can just do "return response.json()" here. > LayoutTests/http/wpt/beacon/cors/cors-preflight-redirect-failure.html:25 > + }), 1000); Cannot we use the new polling mechanism that landed in the other patch? > LayoutTests/http/wpt/beacon/cors/cors-preflight-redirect-failure.html:40 > + result = JSON.parse(json); No need for JSON.parse if using json() above. > LayoutTests/http/wpt/beacon/cors/cors-preflight-redirect-from-crossorigin-to-sameorigin.html:25 > + }), 1000); Ditto, or put in common some code of these two tests/merge them into one.
Comment on attachment 317734 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317734&action=review >> Source/WebKit/NetworkProcess/PingLoad.cpp:61 >> + m_redirectHandler({ }); > > Why do we need to do that? Yes, otherwise it can causes leaks. Andreas had to do the same in NetworkLoad recently. See Bug 175179. >> Source/WebKit/NetworkProcess/PingLoad.cpp:76 >> + m_task = NetworkDataTask::create(*networkSession, *this, loadParameters); > > And we are not moving loadParameters, which probably means that NetworkDataTask is creating its own copy of ResourceRequest. > Some refactoring might be useful there? I'll move loadParameters in. >> Source/WebKit/NetworkProcess/PingLoad.cpp:102 >> + // Use a unique for subsequent loads if needed. > > Use a unique origin? Ok. >> Source/WebKit/NetworkProcess/PingLoad.cpp:106 >> + m_origin = SecurityOrigin::createUnique(); > > You could add a check so as to not create a new origin if m_origin is not unique. Ok. >> LayoutTests/http/wpt/beacon/cors/cors-preflight-redirect-failure.html:21 >> + response.text().then(body => { > > You can just do "return response.json()" here. Ok. >> LayoutTests/http/wpt/beacon/cors/cors-preflight-redirect-failure.html:25 >> + }), 1000); > > Cannot we use the new polling mechanism that landed in the other patch? Tests using it are currently flaky so I don't want to use it until I understand why those tests are flaky. >> LayoutTests/http/wpt/beacon/cors/cors-preflight-redirect-failure.html:40 >> + result = JSON.parse(json); > > No need for JSON.parse if using json() above. Ok.
Created attachment 317743 [details] Patch
Comment on attachment 317743 [details] Patch Clearing flags on attachment: 317743 Committed r220497: <http://trac.webkit.org/changeset/220497>
All reviewed patches have been landed. Closing bug.