Bug 53925 – AssociatedURLLoader does not support Cross Origin Requests

• Component: Platform
• Status: RESOLVED FIXED

[Bill Budge]

The AssociatedURLLoader (/WebKit/Source/WebKit/Chromium/src/AssociatedURLLoader) doesn't support Cross Origin Requests.

[Bill Budge]

Created attachment 81901 [details]
Change AssociatedURLLoader to support CORS, using DocumentThreadableLoader

[WebKit Review Bot]

Attachment 81901 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Last 3072 characters of output:
 so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:88:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:89:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:90:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:91:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:92:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:93:  The parameter name "loader" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:93:  The parameter name "error" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:97:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:121:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:174:  web_error is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:202:  webcore_request is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:203:  webcore_document is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:216:  webcore_request is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.cpp:217:  webcore_document is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Source/WebKit/chromium/src/AssociatedURLLoader.h:58:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.h:59:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.h:66:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.h:67:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/chromium/src/AssociatedURLLoader.h:67:  The parameter name "options" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 26 in 13 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 81901 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7869107

[David Levin]

Comment on attachment 81901 [details]
Change AssociatedURLLoader to support CORS, using DocumentThreadableLoader

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

I tend to do two passes. In my first pass, I catch surface level things. In my second pass I try to understand the whole patch and make sense of it.

These are the comments from my first pass but there are so many things that it makes it harder for me to go more in depth right now, so I'm r- because I believe that they can be quickly addressed which will get rid of a lot of comment clutter in this review.

Also it appears that this patch is doing several things at once. It may speed the process of landing parts and improve the quality of review to try to keep these items separate if possible (and make my "second pass" easier). For example, it looks like m_downloadFilePath is pushed downward and that could be a patch by itself. (At this point I haven't yet figured out why that is needed for this change because I haven't really started my "second pass".)

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

There either need to be tests or an explanation of why tests aren't needed (or possible), but regardless this line should disappear.

> Source/WebCore/ChangeLog:11
> +        (WebCore::DocumentThreadableLoader::DocumentThreadableLoader):

This is where an explanation of why a change was done should go.

> Source/WebCore/loader/DocumentThreadableLoader.cpp:-301
> -    actualRequest->setHTTPOrigin(m_document->securityOrigin()->toString());

Why is this line being deleted?

> Source/WebCore/loader/DocumentThreadableLoader.h:58
> +        virtual void setDefersLoading(bool value);

value adds no information so it shouldn't be here.

> Source/WebCore/platform/network/chromium/ResourceRequest.h:124
> +        bool m_downloadToFile;

Good call adding it here, but this structure is used to copy ResourceRequestData and I don't see that code being modified in this change.

> Source/WebCore/platform/network/chromium/ResourceResponse.h:-100
> -        void setSocketAddress(const String& value) { m_socketAddress = value; }

Why is this being deleted?

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:37
> +

No blank line needed here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:80
> +// SubresourceLoaderClient, which is close to them WebURLLoaderClient.

typo? "close to them WebURLLoaderClient"

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:85
> +    Loader(AssociatedURLLoader* owner, Document* document, BlockingBehavior blockingBehavior, const ResourceRequest& request, const ThreadableLoaderOptions& options);

This constructor should be private (so that the create method is the only way to construct this).

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:87
> +    virtual void AssociatedURLLoader::Loader::willSendRequest(SubresourceLoader* loader, ResourceRequest& request, const ResourceResponse& redirectResponse);

s/AssociatedURLLoader::Loader::willSendRequest/willSendRequest/

The same for other functions declared here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:96
> +    class DummyClient : public ThreadableLoaderClient

Why not just use ThreadableLoaderClient?

>> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:121
>> +    : DocumentThreadableLoader(document, &m_dummyClient, blockingBehavior, request, options), m_owner(owner) {
> 
> Place brace on its own line for function definitions.  [whitespace/braces] [4]

The initializers should be on separate lines as well.

> Source/WebKit/chromium/src/WebURLResponse.cpp:383
> +  m_private->m_resourceResponse->setDownloadFilePath(downloadFilePath.data());

4 space indent.

[Bill Budge]

Created attachment 81907 [details]
AssociatedURLLoader supports CORS using DocumentThreadableLoader

[WebKit Review Bot]

Attachment 81907 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7870095

[David Levin]

Comment on attachment 81907 [details]
AssociatedURLLoader supports CORS using DocumentThreadableLoader

This particular patch is obsolete. It already has been broken down into at least one other change.

[Bill Budge]

I needed another method on ThreadableLoaderClient

[Bill Budge]

I needed another method on ThreadableLoaderClient

[Bill Budge]

Created attachment 83884 [details]
Proposed Patch

I'm not sure if this needs tests or not.

[David Levin]

Comment on attachment 83884 [details]
Proposed Patch

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

> Source/WebKit/chromium/ChangeLog:11
> +        (WTF::ConvertOptions):

Please fix this. (It isn't WTF::ConvertOptions. The script that generates this got confused.)

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Typically in WebKit land, additional years just get tacked on to the end as opposed to replacing the other year. (I know this is different from how chromium does things.)

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:37
> +

No need for a blank line here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:61
> +    switch (options.crossOriginRequestPolicy) {

You could add compile asserts to verify that these enums have the same values.
Then replace the switch with a static cast.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:177
> +    m_clientAdapter = PassOwnPtr<ClientAdapter>(new ClientAdapter(this, m_client, request.downloadToFile()));

Two things look wrong here:
1. adoptPtr should be used instead of PassOwnPtr.
2. Really, the typical pattern is to make the constructor private and and expose a static create method in ClientAdapter which returns PassOwnPtr<ClientAdapter> and does
return adoptPtr(new ClientAdapter(...));
internally.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:192
> +    m_clientAdapter = PassOwnPtr<ClientAdapter>(new ClientAdapter(this, m_client, request.downloadToFile()));

This code looks the same as loadSynchronously. Can they share more code?

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:199
> +       m_loader->cancel();

Indentation appears not to be 4 spaces.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:2
> + * Copyright (C) 2011 Google Inc. All rights reserved.

Same thing about the copyright years.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:42
> +class ThreadableLoaderClient;

ThreadableLoaderClient isn't used in this header (so remove the fwd decl.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:44
> +} // namespace WebCore

No need for the ending comment in WebKit (and in this case I would omit it since the namespace is so small).

> Source/WebKit/chromium/src/AssociatedURLLoader.h:71
> +class AssociatedURLLoader : public WebURLLoader {

I would add a WTF_MAKE_NONCOPYABLE(AssociatedURLLoader); here (from wtf/Noncopyable.h).

> Source/WebKit/chromium/src/AssociatedURLLoader.h:87
> +    class ClientAdapter;

This feels out of place. I would put it after private.

[Bill Budge]

Created attachment 84301 [details]
Proposed Patch

2 questions for David:
1) I noticed an AssertMatchingEnums file for syncing enums. It seemed better to me to do this in AssociatedURLLoader.cpp. Is that OK?
2) AssociatedURLLoader::loadSynchronously probably isn't working after this change, since its response, error, and data objects aren't passed through.

[David Levin]

(In reply to comment #12)
> Created an attachment (id=84301) [details]
> Proposed Patch
> 
> 2 questions for David:
> 1) I noticed an AssertMatchingEnums file for syncing enums. It seemed better to me to do this in AssociatedURLLoader.cpp. Is that OK?

Sounds even better (than what I said).

> 2) AssociatedURLLoader::loadSynchronously probably isn't working after this change, since its response, error, and data objects aren't passed through.

What's the question? :)

[Bill Budge]

I guess my question was "Do you think it's important for the synchronous case to work now (or ever) and can it be added later, or should I connect this through WebCore as I did for downloadToFile stuff?"

[David Levin]

Comment on attachment 84301 [details]
Proposed Patch

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

A few comments.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:71
> +    int m_downloadLength;

Can this overflow?

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:79
> +    , m_downloadingToFile(downloadingToFile) { }

put the {} on different lines.

Would be nice to assert(!client); to make it clear that it should have a value.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:160
> +COMPILE_ASSERT((int)WebKit::DenyCrossOriginRequests == (int)WebCore::DenyCrossOriginRequests, enum_mismatch);

If these casts remain (in another version), use C++ style casts (static_cast).

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:176
> +    m_clientAdapter = adoptPtr(new ClientAdapter(this, m_client, request.downloadToFile()));

Have a static create method do this and make the constructor private.

> Source/WebKit/chromium/src/AssociatedURLLoader.h:35
>  #include "WebURLLoaderClient.h"

This include isn't needed anymore (only a fwd declaration).

[David Levin]

(In reply to comment #14)
> I guess my question was "Do you think it's important for the synchronous case to work now (or ever) and can it be added later, or should I connect this through WebCore as I did for downloadToFile stuff?"

I don't understand a few things:
1. Why there is any implementation if it doesn't work? Why not just crash (for example)?
2. Why do we have this method if it will do nothing? How do we know that no one will call it?

btw, there appear to be a lot of methods in the cpp file like AssociatedURLLoader::loadSynchronously which aren't declared in the .h file. I don't understand how that compiles.

[Bill Budge]

Created attachment 84328 [details]
Proposed Patch

Added static create method to ClientAdapter.
Made loadSynchronously method fail to indicate it's not supported.

[Bill Budge]

Just realized I didn't fully address your questions. The loadSynchronously method is part of the interface. I could implement it but it would require another round of changes to DocumentThreadableLoader and SubresourceLoader, and this method isn't currently needed in the pepper URL loader which is the client for this. That class doesn't support synchronous loading either.

[David Levin]

Comment on attachment 84328 [details]
Proposed Patch

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

r- due to not returning PassRefPtr from constructor (and the question about overflow).

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:58
> +    static ClientAdapter* create(WebURLLoader*, WebURLLoaderClient*, bool /*downloadToFile*/);

This should return a PassRefPtr<ClientAdapter>

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:80
> +    return new ClientAdapter(loader, client, downloadToFile);

Use the adoptPtr here (to convert to PassRefPtr).

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:114
> +    m_downloadLength += lengthReceived;

I still wonder about overflow. (Not likely but overflow is where security issues creep in.)

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:161
> +COMPILE_ASSERT_MATCHING_ENUM(UseAccessControl,  UseAccessControl);

two spaces after comma.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:162
> +COMPILE_ASSERT_MATCHING_ENUM(AllowCrossOriginRequests,  AllowCrossOriginRequests);

Ditto.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:186
> +    m_clientAdapter = adoptPtr(ClientAdapter::create(this, m_client, request.downloadToFile()));

adoptPtr shouldn't be here.

> Source/WebKit/chromium/src/AssociatedURLLoader.cpp:200
> +       m_loader->setDefersLoading(defersLoading);

Should be indented one more space.

[Bill Budge]

Created attachment 84442 [details]
Proposed Patch

I think I can implement the loadSynchronously method in AssociatedURLLoader.cpp, without any other WebKit changes. Can that be another changelist?

[WebKit Commit Bot]

Comment on attachment 84442 [details]
Proposed Patch

Clearing flags on attachment: 84442

Committed r80219: <http://trac.webkit.org/changeset/80219>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mihai Parparita]

Reopening since this was rolled out with http://trac.webkit.org/changeset/80244

[Bill Budge]

Created attachment 84840 [details]
Proposed Patch

Default AssociatedURLLoader should allow credentials so as not to break tests.
AssociatedURLLoader::cancel should release DocumentThreadableLoader.

[WebKit Commit Bot]

Comment on attachment 84840 [details]
Proposed Patch

Clearing flags on attachment: 84840

Committed r80458: <http://trac.webkit.org/changeset/80458>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

This was rolled out again, see https://bugs.webkit.org/show_bug.cgi?id=53925

[Mihai Parparita]

(In reply to comment #27)
> This was rolled out again, see https://bugs.webkit.org/show_bug.cgi?id=53925

I think you meant https://bugs.webkit.org/show_bug.cgi?id=55863

[Bill Budge]

Created attachment 85345 [details]
Proposed Patch

The tests run identically to  the unpatched version on Mac. Will double check on Windows.

[Bill Budge]

Comment on attachment 85345 [details]
Proposed Patch

Hold off on reviewing. It still crashes on 1 test on Windows.

[David Levin]

(In reply to comment #29)
> Created an attachment (id=85345) [details]
> Proposed Patch
> 
> The tests run identically to  the unpatched version on Mac. Will double check on Windows.

Please indicate what changes were done between patches to make it easier to review. Thanks!

[Bill Budge]

This patch adds a clearClient method to the AssociatedURLLoader::ClientAdapter class. AssociatedURLLoader calls it to stop callbacks to the actual client after a load is canceled. This matches the previous implementations semantics.

I also fixed a typo that prevented the loadAsynchronously method from ever allowing cross origin requests.

As it stands, AssociatedURLLoader will allow cross origin requests if constructed with the original ctor. This is necessary for the following to pass:

/http/tests/security/local-video-source-from-remote.html

This makes another test pass which is supposed to fail:

/http/tests/security/xss-DENIED-xml-external-entity.xhtml

A future patch should make sure we can provide appropriately set up loaders to the different clients of WebFrame::CreateAssociatedLoader();

[Bill Budge]

Comment on attachment 85345 [details]
Proposed Patch

I reverted to the original and rebuilt DumpRenderTree, and it still crashes on this test, in exactly the same way. That leads me to think it's my local setup that is wrong. Do you think it's a bad idea to go ahead and see what the bots do?

[Bill Budge]

Tests pass on Windows. Please take a look.

[Bill Budge]

I want to make one minor change, which will help my next patch.

[Bill Budge]

Created attachment 85523 [details]
Proposed Patch

I changed AssociatedURLLoaderOptions struct to WebURLLoaderOptions, as this will need to be a parameter for WebFrame::createAssociatedURLLoader, and AssociatedURLLoader isn't part of the public WebKit interface.

I also made a minor semantic change, so the client will get didFinishLoading callbacks even if they cancel during the didDownloadData callback. This seems more correct, as at this point the load is finished and can't fail.

[Bill Budge]

Created attachment 85537 [details]
Proposed Patch

Fixed Copyright on WebURLLoader.h, and change WebURLLoaderCrossOriginRequestPolicy to WebCrossOriginRequestPolicy.

[David Levin]

Comment on attachment 85537 [details]
Proposed Patch

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

> Source/WebKit/chromium/public/WebURLLoader.h:51
> +    WebURLLoaderOptions() : sniffContent(false), allowCredentials(false), forcePreflight(false), crossOriginRequestPolicy(DenyCrossOriginRequests) {}

Note we typically put a space in this construct "{ }".

[WebKit Commit Bot]

Comment on attachment 85537 [details]
Proposed Patch

Clearing flags on attachment: 85537

Committed r81144: <http://trac.webkit.org/changeset/81144>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Levin]

So close, but this appears to have broken the ui test NPAPITesterBase.GetURLRedirectNotification, which I saw here: http://build.chromium.org/p/chromium.webkit/builders/Vista%20Tests/

So it is best to roll it out one more time. I have a good feeling about next time!

[David Levin]

Rolled out here: http://trac.webkit.org/changeset/81204

[Bill Budge]

Created attachment 86002 [details]
Proposed Patch

I forgot to derive the ClientAdapter from the DocumentThreadableLoaderClient class that we went to so much trouble to make. DocumentThreadableLoader wasn't calling the willSendRequest method of the adapter as a result.

[WebKit Commit Bot]

Comment on attachment 86002 [details]
Proposed Patch

Clearing flags on attachment: 86002

Committed r81303: <http://trac.webkit.org/changeset/81303>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.