Bug 175386 – [Beacon][NetworkSession] Support CORS-preflighting on redirects

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

We should support with CORS-preflighting on redirects.

[Radar WebKit Bug Importer]

<rdar://problem/33801370>

[Chris Dumez]

Created attachment 317714 [details]
WIP Patch

[Chris Dumez]

Created attachment 317721 [details]
Patch

[Chris Dumez]

Created attachment 317733 [details]
Patch

[Chris Dumez]

Created attachment 317734 [details]
Patch

[youenn fablet]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 317743 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 317743 [details]
Patch

Clearing flags on attachment: 317743

Committed r220497: <http://trac.webkit.org/changeset/220497>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.