Bug 109199 - data:// URL behavior of XHR does not match spec
Summary: data:// URL behavior of XHR does not match spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: WebExposed
: 13968 (view as bug list)
Depends on: 133678
Blocks: 123978 151937
  Show dependency treegraph
 
Reported: 2013-02-07 08:53 PST by Dominik Röttsches (drott)
Modified: 2016-08-29 01:08 PDT (History)
23 users (show)

See Also:


Attachments
WIP (28.78 KB, patch)
2014-02-04 03:33 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (518.51 KB, application/zip)
2014-02-04 07:11 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (623.42 KB, application/zip)
2014-02-04 07:46 PST, Build Bot
no flags Details
Direct handling within ResourceLoader and FrameLoader (32.23 KB, patch)
2014-02-07 11:05 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2 (471.65 KB, application/zip)
2014-02-07 13:54 PST, Build Bot
no flags Details
Fixed windows built & removed status code setting (39.65 KB, patch)
2014-02-09 13:47 PST, youenn fablet
no flags Details | Formatted Diff | Diff
second try at fixing builds (42.17 KB, patch)
2014-02-12 07:31 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing mac build (42.65 KB, patch)
2014-02-13 07:53 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing and improving WK2 integration (44.77 KB, patch)
2014-04-17 05:03 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Adding missing WebCore symbols (46.48 KB, patch)
2014-06-06 09:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (46.36 KB, patch)
2014-06-06 14:33 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (501.71 KB, application/zip)
2014-06-06 15:56 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (500.17 KB, application/zip)
2014-06-06 16:59 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (482.35 KB, application/zip)
2014-06-06 20:47 PDT, Build Bot
no flags Details
Data URL loading within local loader (44.65 KB, patch)
2014-10-02 04:26 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (721.23 KB, application/zip)
2014-10-02 06:13 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (762.14 KB, application/zip)
2014-10-02 06:36 PDT, Build Bot
no flags Details
Fixing case of empty but valid data URL (45.40 KB, patch)
2014-10-02 08:14 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Improved style (45.33 KB, patch)
2014-10-03 02:41 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Without adding new loader classes (36.17 KB, patch)
2014-11-14 02:12 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing platform separation (45.59 KB, patch)
2014-11-17 05:08 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (23.41 KB, patch)
2016-07-12 08:09 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.06 MB, application/zip)
2016-07-12 08:55 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews102 for mac-yosemite (801.66 KB, application/zip)
2016-07-12 09:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (641.70 KB, application/zip)
2016-07-12 09:09 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-yosemite (1.44 MB, application/zip)
2016-07-12 09:10 PDT, Build Bot
no flags Details
Removing new data URL error checks (21.65 KB, patch)
2016-07-12 09:26 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (21.69 KB, patch)
2016-08-23 02:49 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (35.03 KB, patch)
2016-08-29 00:03 PDT, youenn fablet
youennf: commit-queue+
Details | Formatted Diff | Diff
Patch for landing (23.16 KB, patch)
2016-08-29 00:37 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominik Röttsches (drott) 2013-02-07 08:53:06 PST
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.
Comment 1 youenn fablet 2014-02-04 03:33:57 PST
Created attachment 223093 [details]
WIP
Comment 2 WebKit Commit Bot 2014-02-04 05:57:56 PST
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.
Comment 3 Build Bot 2014-02-04 07:11:33 PST
Comment on attachment 223093 [details]
WIP

Attachment 223093 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4757255899578368

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
Comment 4 Build Bot 2014-02-04 07:11:43 PST
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
Comment 5 Build Bot 2014-02-04 07:46:25 PST
Comment on attachment 223093 [details]
WIP

Attachment 223093 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6082228681441280

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
Comment 6 Build Bot 2014-02-04 07:46:27 PST
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 7 Alexey Proskuryakov 2014-02-04 09:46:45 PST
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.
Comment 8 youenn fablet 2014-02-04 11:55:52 PST
(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?
Comment 9 Alexey Proskuryakov 2014-02-04 12:15:39 PST
> 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.
Comment 10 youenn fablet 2014-02-05 09:42:09 PST
(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
Comment 11 Alexey Proskuryakov 2014-02-05 11:12:16 PST
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 12 youenn fablet 2014-02-07 11:05:11 PST
Created attachment 223474 [details]
Direct handling within ResourceLoader and FrameLoader
Comment 13 Build Bot 2014-02-07 13:54:45 PST
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
Comment 14 Build Bot 2014-02-07 13:54:49 PST
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
Comment 15 youenn fablet 2014-02-09 13:47:55 PST
Created attachment 223647 [details]
Fixed windows built & removed status code setting
Comment 16 youenn fablet 2014-02-12 07:31:09 PST
Created attachment 223971 [details]
second try at fixing builds
Comment 17 youenn fablet 2014-02-13 07:53:30 PST
Created attachment 224069 [details]
Fixing mac build
Comment 18 youenn fablet 2014-04-17 05:03:57 PDT
Created attachment 229537 [details]
Rebasing and improving WK2 integration
Comment 19 youenn fablet 2014-06-06 09:35:14 PDT
Created attachment 232617 [details]
Adding missing WebCore symbols
Comment 20 youenn fablet 2014-06-06 14:33:40 PDT
Created attachment 232629 [details]
Rebasing
Comment 21 Build Bot 2014-06-06 15:56:24 PDT
Comment on attachment 232629 [details]
Rebasing

Attachment 232629 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5411486037966848

New failing tests:
editing/selection/find-yensign-and-backslash.html
Comment 22 Build Bot 2014-06-06 15:56:30 PDT
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
Comment 23 Build Bot 2014-06-06 16:59:18 PDT
Comment on attachment 232629 [details]
Rebasing

Attachment 232629 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5196180938031104

New failing tests:
editing/selection/find-yensign-and-backslash.html
Comment 24 Build Bot 2014-06-06 16:59:27 PDT
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
Comment 25 Build Bot 2014-06-06 20:47:30 PDT
Comment on attachment 232629 [details]
Rebasing

Attachment 232629 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6142310156861440

New failing tests:
editing/selection/find-yensign-and-backslash.html
Comment 26 Build Bot 2014-06-06 20:47:40 PDT
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
Comment 27 youenn fablet 2014-09-30 13:30:44 PDT
(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.
Comment 28 youenn fablet 2014-10-02 04:26:59 PDT
Created attachment 239102 [details]
Data URL loading within local loader
Comment 29 Build Bot 2014-10-02 06:12:48 PDT
Comment on attachment 239102 [details]
Data URL loading within local loader

Attachment 239102 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4663877363040256

New failing tests:
http/tests/security/xssAuditor/meta-tag-http-refresh-x-frame-options.html
http/tests/security/xssAuditor/full-block-javascript-link.html
html5lib/generated/run-tests1-data.html
http/tests/security/xssAuditor/full-block-iframe-javascript-url.html
fast/dom/HTMLHeadElement/head-check.html
http/tests/security/xssAuditor/full-block-script-tag.html
http/tests/security/xssAuditor/full-block-get-from-iframe.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html
http/tests/security/xssAuditor/full-block-script-tag-cross-domain.html
http/tests/security/xssAuditor/xss-protection-parsing-01.html
http/tests/security/xssAuditor/full-block-base-href.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html
plugins/destroy-stream-twice.html
http/tests/security/xssAuditor/xss-protection-parsing-03.html
fast/replaced/frame-removed-during-resize-smaller.html
userscripts/user-script-plugin-document.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html
http/tests/security/xssAuditor/full-block-link-onclick.html
fast/events/attribute-listener-extracted-from-frameless-doc-context-2.html
http/tests/security/xssAuditor/full-block-post-from-iframe.html
http/tests/security/xssAuditor/full-block-script-tag-with-source.html
fast/events/attribute-listener-cloned-from-frameless-doc-context-2.html
http/tests/security/xssAuditor/xss-protection-parsing-04.html
http/tests/security/xssAuditor/block-does-not-leak-location.html
http/tests/security/xssAuditor/block-does-not-leak-referrer.html
http/tests/security/xssAuditor/report-script-tag-full-block.html
http/tests/security/xssAuditor/full-block-object-tag.html
Comment 30 Build Bot 2014-10-02 06:13:03 PDT
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
Comment 31 Build Bot 2014-10-02 06:35:57 PDT
Comment on attachment 239102 [details]
Data URL loading within local loader

Attachment 239102 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5284155466186752

New failing tests:
http/tests/security/xssAuditor/meta-tag-http-refresh-x-frame-options.html
http/tests/security/xssAuditor/full-block-javascript-link.html
html5lib/generated/run-tests1-data.html
http/tests/security/xssAuditor/full-block-iframe-javascript-url.html
fast/dom/HTMLHeadElement/head-check.html
http/tests/security/xssAuditor/full-block-script-tag.html
http/tests/security/xssAuditor/full-block-get-from-iframe.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html
http/tests/security/xssAuditor/full-block-script-tag-cross-domain.html
http/tests/security/xssAuditor/xss-protection-parsing-01.html
http/tests/security/xssAuditor/full-block-base-href.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-in-body.html
plugins/destroy-stream-twice.html
http/tests/security/xssAuditor/xss-protection-parsing-03.html
fast/replaced/frame-removed-during-resize-smaller.html
userscripts/user-script-plugin-document.html
http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html
http/tests/loading/pdf-commit-load-callbacks.html
http/tests/security/xssAuditor/full-block-link-onclick.html
fast/events/attribute-listener-extracted-from-frameless-doc-context-2.html
http/tests/security/xssAuditor/full-block-post-from-iframe.html
http/tests/security/xssAuditor/full-block-script-tag-with-source.html
fast/events/attribute-listener-cloned-from-frameless-doc-context-2.html
http/tests/security/xssAuditor/xss-protection-parsing-04.html
http/tests/security/xssAuditor/block-does-not-leak-location.html
http/tests/security/xssAuditor/block-does-not-leak-referrer.html
http/tests/security/xssAuditor/report-script-tag-full-block.html
http/tests/security/xssAuditor/full-block-object-tag.html
Comment 32 Build Bot 2014-10-02 06:36:12 PDT
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
Comment 33 youenn fablet 2014-10-02 08:14:02 PDT
Created attachment 239109 [details]
Fixing case of empty but valid data URL
Comment 34 youenn fablet 2014-10-03 02:41:23 PDT
Created attachment 239198 [details]
Improved style
Comment 35 youenn fablet 2014-10-03 04:39:30 PDT
(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 36 Antti Koivisto 2014-11-10 02:00:06 PST
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 37 youenn fablet 2014-11-14 02:12:36 PST
Created attachment 241567 [details]
Without adding new loader classes
Comment 38 youenn fablet 2014-11-17 05:08:07 PST
Created attachment 241701 [details]
Fixing platform separation
Comment 39 Antti Koivisto 2014-11-17 06:35:38 PST
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.
Comment 40 youenn fablet 2014-11-18 07:50:00 PST
(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.
Comment 41 youenn fablet 2014-11-18 08:01:50 PST
> I will create a bug for this entry and give it a try.

BlobResourceHandle specific bug is bug 138835
Comment 42 youenn fablet 2016-07-12 08:09:33 PDT
Created attachment 283408 [details]
Patch
Comment 43 youenn fablet 2016-07-12 08:10:05 PDT
*** Bug 13968 has been marked as a duplicate of this bug. ***
Comment 44 Build Bot 2016-07-12 08:55:44 PDT
Comment on attachment 283408 [details]
Patch

Attachment 283408 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/1669073

New failing tests:
fast/forms/multiple-form-submission-protection-mouse.html
fast/forms/button-state-restore.html
Comment 45 Build Bot 2016-07-12 08:55:52 PDT
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
Comment 46 Build Bot 2016-07-12 09:00:00 PDT
Comment on attachment 283408 [details]
Patch

Attachment 283408 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/1669101

New failing tests:
fast/forms/multiple-form-submission-protection-mouse.html
fast/forms/button-state-restore.html
Comment 47 Build Bot 2016-07-12 09:00:07 PDT
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
Comment 48 Build Bot 2016-07-12 09:08:55 PDT
Comment on attachment 283408 [details]
Patch

Attachment 283408 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1669109

New failing tests:
fast/forms/button-state-restore.html
Comment 49 Build Bot 2016-07-12 09:09:03 PDT
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
Comment 50 Build Bot 2016-07-12 09:10:49 PDT
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
Comment 51 Build Bot 2016-07-12 09:10:56 PDT
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
Comment 52 youenn fablet 2016-07-12 09:26:52 PDT
Created attachment 283416 [details]
Removing new data URL error checks
Comment 53 youenn fablet 2016-07-12 09:27:44 PDT
(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 54 Alex Christensen 2016-07-13 10:36:32 PDT
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.
Comment 55 youenn fablet 2016-08-02 03:21:28 PDT
(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?
Comment 56 youenn fablet 2016-08-23 02:49:59 PDT
Created attachment 286693 [details]
Patch
Comment 57 youenn fablet 2016-08-23 02:57:11 PDT
(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.
Comment 58 youenn fablet 2016-08-25 07:36:47 PDT
Comment on attachment 286693 [details]
Patch

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

> Source/WebCore/ChangeLog:18
> +        As can be seen from the rebased tests, no constraint is put on the method used as the fetch specification is about to allow all methods for data URLs.

This has landed in the fetch spec: https://github.com/whatwg/fetch/commit/c4cb4f4a5fa90a72f1000449495aec04f6c8c96b
WPT tests will have to be updated accordingly.
Comment 59 youenn fablet 2016-08-29 00:03:09 PDT
Created attachment 287251 [details]
Patch for landing
Comment 60 youenn fablet 2016-08-29 00:04:10 PDT
Patch is rebased against master.
It also includes setting/testing statusText to "OK"
Comment 61 youenn fablet 2016-08-29 00:37:31 PDT
Created attachment 287256 [details]
Patch for landing
Comment 62 WebKit Commit Bot 2016-08-29 01:08:18 PDT
Comment on attachment 287256 [details]
Patch for landing

Clearing flags on attachment: 287256

Committed r205113: <http://trac.webkit.org/changeset/205113>
Comment 63 WebKit Commit Bot 2016-08-29 01:08:30 PDT
All reviewed patches have been landed.  Closing bug.