Bug 158425 – Move preflight check code outside of DocumentThreadableLoader

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Having preflight checks done in DocumentThreadableLoader makes the code hard to parse and potentially difficult to reuse.
It would be more convenient to split the code.

[youenn fablet]

Created attachment 280606 [details]
Patch

[youenn fablet]

Comment on attachment 280606 [details]
Patch

Putting in r?
I kept the handling of redirect responses as close to the existent code.
But this does not seem very good.
At the very least we should check preflight from it.

It would be better to align with fetch spec and reject non-ok responses (https://fetch.spec.whatwg.org/#ok-status).

[Alex Christensen]

Comment on attachment 280606 [details]
Patch

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

This is to share the functionality with fetch, right?

> Source/WebCore/ChangeLog:12
> +        Behavior should be the same as before except in the case of a preflight response
> +        being a 3XX redirect response.

Could you add a test for this behavior?  I'd be interested in comparing our behavior in this case with other browsers.

[youenn fablet]

(In reply to comment #3)
> Comment on attachment 280606 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280606&action=review
> 
> This is to share the functionality with fetch, right?

This is mostly to make the code easier to read, maintain and relate to https://fetch.spec.whatwg.org/#cors-preflight-fetch.
In the future, it may also help if preflight is required outside of DocumentThreadableLoader.
For window.fetch, the plan is to keep calling ThreadableLoader::create to handle the actual loading which would handle the preflight.

> > Source/WebCore/ChangeLog:12
> > +        Behavior should be the same as before except in the case of a preflight response
> > +        being a 3XX redirect response.

The difference is minor really and the change of behavior should hopefully not be visible from JS.

With the patch, any 3XX redirect response will directly lead to a didRedirectFailCheck() call.

Without the patch, DocumentThreadableLoader::redirectReceived is called.
In that function, didRedirectFailCheck() will probably always be called (through a CSP check or through the regular response code path as request is cross-origin and not simple).

I think 3XX preflight responses should lead to a preflight failure, thus a call to didFailAccessControl.
This would have impact on the logging of the error but not on the JS observable behavior.

> Could you add a test for this behavior?  I'd be interested in comparing our
> behavior in this case with other browsers.

LayoutTests/http/tests/xmlhttprequest/access-control-and-redirects-async.html is testing that.
If you change the redirect response preflight code from 302 to 300 or 304 in redirect-cors.php, a preflight failure (and a call to didFailAccessContro) will happen.

Firefox and Chrome are failing on preflight responses with non-ok status, including 3XX.

[Darin Adler]

Comment on attachment 280606 [details]
Patch

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

> Source/WebCore/loader/CrossOriginPreflightChecker.cpp:55
> +        m_resource->removeClient(this);

Seems clear this function should take a reference instead of a pointer. Some day.

> Source/WebCore/loader/CrossOriginPreflightChecker.cpp:71
> +    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willReceiveResourceResponse(frame);

I suggest auto here instead of this long type name.

> Source/WebCore/loader/CrossOriginPreflightChecker.cpp:74
> +    String accessControlErrorDescription;

I think this could just be named description, since this has a pretty narrow scope here.

> Source/WebCore/loader/CrossOriginPreflightChecker.cpp:80
> +    auto preflightResult = std::make_unique<CrossOriginPreflightResultCacheItem>(loader.options().allowCredentials());

I think we could just call this result, since it’s in the scope of a function that’s all about preflighting.

> Source/WebCore/loader/CrossOriginPreflightChecker.cpp:104
> +    ThreadableLoaderOptions options = m_loader.options();

I suggest using auto here.

> Source/WebCore/loader/CrossOriginPreflightChecker.h:33
> +#include "CachedRawResource.h"

Does this actually need to be included or would a forward declaration suffice?

> Source/WebCore/loader/CrossOriginPreflightChecker.h:68
> +} //  namespace WebCore

Extra space here after the "//".

> Source/WebCore/loader/DocumentThreadableLoader.cpp:115
> +    auto crossOriginRequest = ResourceRequest(request);

No need to write ResourceRequest() here. Just auto x = y will make a copy of y.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:146
> +        m_preflightChecker = CrossOriginPreflightChecker(*this, WTFMove(request));

I think you could write this:

    m_preflightChecker = { *this, WTFMove(request) };

Might be better?

> Source/WebCore/loader/DocumentThreadableLoader.h:58
> +        friend CrossOriginPreflightChecker;

Doesn’t this need to be "friend class"? I’m surprised this syntax works.

[youenn fablet]

Thanks for the review.

(In reply to comment #5)
> Comment on attachment 280606 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280606&action=review
> 
> > Source/WebCore/loader/CrossOriginPreflightChecker.cpp:55
> > +        m_resource->removeClient(this);
> 
> Seems clear this function should take a reference instead of a pointer. Some
> day.

Right.

> > Source/WebCore/loader/CrossOriginPreflightChecker.cpp:71
> > +    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willReceiveResourceResponse(frame);
> 
> I suggest auto here instead of this long type name.

OK

> > Source/WebCore/loader/CrossOriginPreflightChecker.cpp:74
> > +    String accessControlErrorDescription;
> 
> I think this could just be named description, since this has a pretty narrow
> scope here.

OK

> > Source/WebCore/loader/CrossOriginPreflightChecker.cpp:80
> > +    auto preflightResult = std::make_unique<CrossOriginPreflightResultCacheItem>(loader.options().allowCredentials());
> 
> I think we could just call this result, since it’s in the scope of a
> function that’s all about preflighting.

OK

> > Source/WebCore/loader/CrossOriginPreflightChecker.cpp:104
> > +    ThreadableLoaderOptions options = m_loader.options();
> 
> I suggest using auto here.

OK

> > Source/WebCore/loader/CrossOriginPreflightChecker.h:33
> > +#include "CachedRawResource.h"
> 
> Does this actually need to be included or would a forward declaration
> suffice?

OK

> > Source/WebCore/loader/CrossOriginPreflightChecker.h:68
> > +} //  namespace WebCore
> 
> Extra space here after the "//".

OK

> > Source/WebCore/loader/DocumentThreadableLoader.cpp:115
> > +    auto crossOriginRequest = ResourceRequest(request);
> 
> No need to write ResourceRequest() here. Just auto x = y will make a copy of
> y.

OK

> > Source/WebCore/loader/DocumentThreadableLoader.cpp:146
> > +        m_preflightChecker = CrossOriginPreflightChecker(*this, WTFMove(request));
> 
> I think you could write this:
> 
>     m_preflightChecker = { *this, WTFMove(request) };
> 
> Might be better?

It is not working as preflightChecker is an Optional.
Maybe Optional should be upgraded to support that.

> > Source/WebCore/loader/DocumentThreadableLoader.h:58
> > +        friend CrossOriginPreflightChecker;
> 
> Doesn’t this need to be "friend class"? I’m surprised this syntax works.

I think it is a simplification introduced recently.

[youenn fablet]

Created attachment 281007 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 281007 [details]
Patch for landing

Clearing flags on attachment: 281007

Committed r201924: <http://trac.webkit.org/changeset/201924>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.