Bug 53925

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

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.

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

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.

Created attachment 81907 [details] AssociatedURLLoader supports CORS using DocumentThreadableLoader

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

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.

I needed another method on ThreadableLoaderClient

I needed another method on ThreadableLoaderClient

Created attachment 83884 [details] Proposed Patch I'm not sure if this needs tests or not.

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.

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.

(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? :)

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?"

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).

(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.

Created attachment 84328 [details] Proposed Patch Added static create method to ClientAdapter. Made loadSynchronously method fail to indicate it's not supported.

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.

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.

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?

Comment on attachment 84442 [details] Proposed Patch Clearing flags on attachment: 84442 Committed r80219: <http://trac.webkit.org/changeset/80219>

All reviewed patches have been landed. Closing bug.

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

Created attachment 84840 [details] Proposed Patch Default AssociatedURLLoader should allow credentials so as not to break tests. AssociatedURLLoader::cancel should release DocumentThreadableLoader.

Comment on attachment 84840 [details] Proposed Patch Clearing flags on attachment: 84840 Committed r80458: <http://trac.webkit.org/changeset/80458>

All reviewed patches have been landed. Closing bug.

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

(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

Created attachment 85345 [details] Proposed Patch The tests run identically to the unpatched version on Mac. Will double check on Windows.

Comment on attachment 85345 [details] Proposed Patch Hold off on reviewing. It still crashes on 1 test on Windows.

(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!

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();

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?

Tests pass on Windows. Please take a look.

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

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.

Created attachment 85537 [details] Proposed Patch Fixed Copyright on WebURLLoader.h, and change WebURLLoaderCrossOriginRequestPolicy to WebCrossOriginRequestPolicy.

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 "{ }".

Comment on attachment 85537 [details] Proposed Patch Clearing flags on attachment: 85537 Committed r81144: <http://trac.webkit.org/changeset/81144>

All reviewed patches have been landed. Closing bug.

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!

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

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.

Comment on attachment 86002 [details] Proposed Patch Clearing flags on attachment: 86002 Committed r81303: <http://trac.webkit.org/changeset/81303>

All reviewed patches have been landed. Closing bug.