WebKit's implementation does not match section 6 of the XHR spec:
https://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#data:-urls-and-http
Behavior in case of GET and in case of POST both deviate.
In POST case, basically no request should be made at all, and an error 501 "Not Implemented" error response should be given.
Attachment 223093[details] did not pass style-queue:
ERROR: Source/WebCore/platform/network/ResourceHandle.cpp:30: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 1 in 13 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 223111[details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 223118[details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 223093[details]
WIP
View in context: https://bugs.webkit.org/attachment.cgi?id=223093&action=review
I think that long term, data URLs should probably be handled above ResourceHandle level. The reason is that ResourceHandle lives in NetworkProcess, and it's inefficient to send data URL content over IPC.
The difficulty here is to make sure that any loading policies implemented by ResourceHandle on NetworkProcess side are still in effect for data loads, and step one would be to gain understanding of what the relevant policies are, if any. One thing I have in mind is that we need to support pausing loads through deferCallbacks.
> Source/WebCore/platform/network/DataURLHandle.cpp:40
> +class DataURLResourceHandle : public ResourceHandle {
It is a design mistake of BlobResourceHandle that it is a subclass of ResourceHandle. ResourceHandle is really a leaf class by its function, and trying to "patch" it with overridden functions to support entirely different functionality is super messy.
Please don't repeat this mistake here.
(In reply to comment #7)
> (From update of attachment 223093[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=223093&action=review
>
> I think that long term, data URLs should probably be handled above ResourceHandle level. The reason is that ResourceHandle lives in NetworkProcess, and it's inefficient to send data URL content over IPC.
Patch in bug 118068 is a step forward for that purpose.
Basically, it does optimize data url parsing for images (which is a common case) using the parseDataURL function directly.
It may be extended to other resource types in the future.
I would be interested in your feedback on this patch.
Maybe it could be updated so that data url parsing is made async.
Overall, this patch is also a step towards that goal: it removes the need for underlying network backends (soup, cf...) to handle data urls.
> > Source/WebCore/platform/network/DataURLHandle.cpp:40
> > +class DataURLResourceHandle : public ResourceHandle {
>
> It is a design mistake of BlobResourceHandle that it is a subclass of ResourceHandle. ResourceHandle is really a leaf class by its function, and trying to "patch" it with overridden functions to support entirely different functionality is super messy.
>
> Please don't repeat this mistake here.
Making it a subclass allows capturing whether loading is cancelled at some point. Capture of that information is happening in BlobResourceHandle as well as Soup and Curl specific bits of ResourceHandle (I do not know about cf...).
If factored out within ResourceHandle, there would be no need of subclassing ResourceHandle here (maybe BlobResourceHandle as well).
I already put a FIXME in the patch for that purpose and was planning to study that after that patch.
Would this approach works with you?
> If factored out within ResourceHandle, there would be no need of subclassing ResourceHandle here (maybe BlobResourceHandle as well).
> I already put a FIXME in the patch for that purpose and was planning to study that after that patch.> Would this approach works with you?
I don't think that I understand, could you please elaborate?
Ultimately, both blob loading and data URL loading should be moved away from ResourceHandle level. So, designing the best way to fit them within ResourceHandle may not be the best use of effort.
(In reply to comment #9)
> > If factored out within ResourceHandle, there would be no need of subclassing ResourceHandle here (maybe BlobResourceHandle as well).
> > I already put a FIXME in the patch for that purpose and was planning to study that after that patch.
>
> > Would this approach works with you?
>
> I don't think that I understand, could you please elaborate?
>
> Ultimately, both blob loading and data URL loading should be moved away from ResourceHandle level. So, designing the best way to fit them within ResourceHandle may not be the best use of effort.
Very speculatively, this would mean:
1. Factor out code to capture cancel of loads within ResourceHandle
2. Create a NetworkResourceHandle class and move ResourceHandle network specific stuff into that class. That would leave in ResourceHandle stuff that is common to DataURLHandle, BlobURLHandle and NetworkResourceHandle (capturing cancel of loads, pausing loads...)
3. Move abstract ResourceHandle stuff, DataURLHandle and BlobResourceHandle into WebProcess
I don't think that we need any of ResourceHandle in WebProcess. The purpose of this class is to be an abstraction for platform specific network libraries, nothing more.
Loading that happens in WebProcess should be implemented at ResourceLoader level (obviously, by specialized classes, but ones driven by ResourceLoader nonetheless). This is especially true of Blobs, which are not a networking concept, and know about Web specific things like security origins..
Comment on attachment 223474[details]
Direct handling within ResourceLoader and FrameLoader
Attachment 223474[details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5383910955417600
New failing tests:
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html
Created attachment 223493[details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 232632[details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 232636[details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Created attachment 232649[details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
(In reply to comment #11)
> I don't think that we need any of ResourceHandle in WebProcess. The purpose of this class is to be an abstraction for platform specific network libraries, nothing more.
>
> Loading that happens in WebProcess should be implemented at ResourceLoader level (obviously, by specialized classes, but ones driven by ResourceLoader nonetheless). This is especially true of Blobs, which are not a networking concept, and know about Web specific things like security origins..
I do not think we can currently avoid data URL handling at ResourceHandle level: ping loader, gstreamer binding, download manager may all be loading data URLs without going through ResourceLoader.
To load these URLs within WebProcess, data:// (and blob://) URLs could be intercepted when going through ResourceLoader similarly to what is done with appcache.
To remain consistent whatever the loading path, the same code should be used, hence implementing data URL handling at ResourceHandle level. Bug 137005 is a preliminary step for that.
Created attachment 239105[details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 239107[details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06 Port: mac-mountainlion Platform: Mac OS X 10.8.5
(In reply to comment #34)
> Created an attachment (id=239198) [details]
> Improved style
This patch introduces a LocalResourceLoader interface to load resources without going through ResourceHandle. This is currently only used for data URLs.
It may also be used for handling appcache and archive resource loading. If that helps the review, I can upload the corresponding patch.
Comment on attachment 239198[details]
Improved style
View in context: https://bugs.webkit.org/attachment.cgi?id=239198&action=review> Source/WebCore/loader/LocalResourceLoader.h:49
> +class LocalResourceLoader : public RefCounted<LocalResourceLoader> {
> + public:
> + static PassRefPtr<LocalResourceLoader> mayCreateLocalLoader(ResourceLoader*, const ResourceRequest&);
> + static bool maybeLoadSynchronously(ResourceRequest&, ResourceError&, ResourceResponse&, Vector<char>&);
> + virtual void load(ResourceLoader*) = 0;
> +
> + virtual ~LocalResourceLoader() { };
> +};
I don't think we want to introduce yet another class hierarchy called *ResourceLoader. We have way too many poorly defined Loader classes already.
This should to be factored in some other way.
> Source/WebCore/platform/network/DataURL.h:49
> +class DataURLResourceLoader: public LocalResourceLoader {
> + public:
> + static void loadSynchronously(ResourceRequest&, ResourceError&, ResourceResponse&, Vector<char>&);
> + void load(ResourceLoader*);
> + DataURLResourceLoader(const ResourceRequest&);
> +
> + private:
> + ResourceRequest m_request;
> +};
This is a layering violation. Code in WebCore/platform/ should not depend on types elsewhere in WebCore (WebCore/loader/ in this case)
Considering this carries almost no data I'm not sure I see the need for this type or its base class.
Comment on attachment 241701[details]
Fixing platform separation
View in context: https://bugs.webkit.org/attachment.cgi?id=241701&action=review> Source/WebCore/platform/network/ResourceResolver.h:49
> +class ResourceResolver : public RefCounted<ResourceResolver> {
> +public:
> + static PassRefPtr<ResourceResolver> create(NetworkingContext*, const ResourceRequest&, ResourceHandleClient*, bool defersLoading, bool shouldContentSniff);
What does this type do?
A problem with the current loading system is that there are many vaguely defined classes. This makes it easy to implement functionality in wrong place making things even more vague (see blobs). I don't think another glue type without functionality is going to help.
As I mentioned a good starting for a cleanup might be moving blob handling to higher level (SubresourceLoader?) and devirtualizing ResourceHandle. Then ResourceHandle has a clear purpose (it is the interface to the platform loader). That in turn might make clear how to handle data URLs.
(In reply to comment #39)
> Comment on attachment 241701[details]
> Fixing platform separation
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=241701&action=review
>
> > Source/WebCore/platform/network/ResourceResolver.h:49
> > +class ResourceResolver : public RefCounted<ResourceResolver> {
> > +public:
> > + static PassRefPtr<ResourceResolver> create(NetworkingContext*, const ResourceRequest&, ResourceHandleClient*, bool defersLoading, bool shouldContentSniff);
>
> What does this type do?
The "handle" needs at the minimum to be cancellable.
This is the main purpose of this abstraction as a wrapper above ResourceHandle.
> A problem with the current loading system is that there are many vaguely
> defined classes. This makes it easy to implement functionality in wrong
> place making things even more vague (see blobs). I don't think another glue
> type without functionality is going to help.
Agreed.
A different approach is to subclass SubresourceLoader to specialize it for different resource types: blobs, data URLs...
That would mean moving ResourceHandle-specific ResourceLoader stuff in specialized classes for WK1 and WK2.
That may also mean merging ResourceLoader and SubresourceLoader in one abstract class.
> As I mentioned a good starting for a cleanup might be moving blob handling
> to higher level (SubresourceLoader?) and devirtualizing ResourceHandle. Then
> ResourceHandle has a clear purpose (it is the interface to the platform
> loader). That in turn might make clear how to handle data URLs.
I will create a bug for this entry and give it a try.
Created attachment 283411[details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 283412[details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 283414[details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.5
Created attachment 283415[details]
Archive of layout-test-results from ews115 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-yosemite Platform: Mac OS X 10.10.5
(In reply to comment #50)
> Comment on attachment 283408[details]
> Patch
>
> Attachment 283408[details] did not pass mac-debug-ews (mac):
> Output: http://webkit-queues.webkit.org/results/1669113
>
> New failing tests:
> fast/forms/multiple-form-submission-protection-mouse.html
> fast/forms/button-state-restore.html
Failure is due to trying to do POST on data URLs.
I removed changes to ResourceLoader in the current patch to minimise compatibility risks.
Comment on attachment 283416[details]
Removing new data URL error checks
View in context: https://bugs.webkit.org/attachment.cgi?id=283416&action=review> Source/WebCore/loader/ThreadableLoader.h:70
> + ThreadableLoaderOptions(const ResourceLoaderOptions&, PreflightPolicy, CrossOriginRequestPolicy, ContentSecurityPolicyEnforcement, String&& initiator, bool);
This should have a name or be a 2-value enum to indicate what the bool is.
(In reply to comment #54)
> Comment on attachment 283416[details]
> Removing new data URL error checks
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283416&action=review
>
> > Source/WebCore/loader/ThreadableLoader.h:70
> > + ThreadableLoaderOptions(const ResourceLoaderOptions&, PreflightPolicy, CrossOriginRequestPolicy, ContentSecurityPolicyEnforcement, String&& initiator, bool);
>
> This should have a name or be a 2-value enum to indicate what the bool is.
Right, I will add a parameter name here.
Any more feedback?
(In reply to comment #56)
> Created attachment 286693[details]
> Patch
I rebased the patch and removed method checking in DocumentThreadableLoader, based on on-going fetch discussions (https://github.com/whatwg/fetch/issues/339).
The same-origin-data-url flag is currently a ThreadableLoader option.
But it should be moved below so that this information is handled by CachedResource/ResourceLoader.
2014-02-04 03:33 PST, youenn fablet
2014-02-04 07:11 PST, Build Bot
2014-02-04 07:46 PST, Build Bot
2014-02-07 11:05 PST, youenn fablet
2014-02-07 13:54 PST, Build Bot
2014-02-09 13:47 PST, youenn fablet
2014-02-12 07:31 PST, youenn fablet
2014-02-13 07:53 PST, youenn fablet
2014-04-17 05:03 PDT, youenn fablet
2014-06-06 09:35 PDT, youenn fablet
2014-06-06 14:33 PDT, youenn fablet
2014-06-06 15:56 PDT, Build Bot
2014-06-06 16:59 PDT, Build Bot
2014-06-06 20:47 PDT, Build Bot
2014-10-02 04:26 PDT, youenn fablet
2014-10-02 06:13 PDT, Build Bot
2014-10-02 06:36 PDT, Build Bot
2014-10-02 08:14 PDT, youenn fablet
2014-10-03 02:41 PDT, youenn fablet
2014-11-14 02:12 PST, youenn fablet
2014-11-17 05:08 PST, youenn fablet
2016-07-12 08:09 PDT, youenn fablet
2016-07-12 08:55 PDT, Build Bot
2016-07-12 09:00 PDT, Build Bot
2016-07-12 09:09 PDT, Build Bot
2016-07-12 09:10 PDT, Build Bot
2016-07-12 09:26 PDT, youenn fablet
2016-08-23 02:49 PDT, youenn fablet
2016-08-29 00:03 PDT, youenn fablet
2016-08-29 00:37 PDT, youenn fablet