|
Description
Dominik Röttsches (drott)
2013-02-07 08:53:06 PST
Created attachment 223093 [details]
WIP
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 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 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 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 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.. Created attachment 223474 [details]
Direct handling within ResourceLoader and FrameLoader
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 223647 [details]
Fixed windows built & removed status code setting
Created attachment 223971 [details]
second try at fixing builds
Created attachment 224069 [details]
Fixing mac build
Created attachment 229537 [details]
Rebasing and improving WK2 integration
Created attachment 232617 [details]
Adding missing WebCore symbols
Created attachment 232629 [details]
Rebasing
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 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 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 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 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 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 239102 [details]
Data URL loading within local loader
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 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 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 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
Created attachment 239109 [details]
Fixing case of empty but valid data URL
Created attachment 239198 [details]
Improved style
(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. Created attachment 241567 [details]
Without adding new loader classes
Created attachment 241701 [details]
Fixing platform separation
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. > I will create a bug for this entry and give it a try. BlobResourceHandle specific bug is bug 138835 Created attachment 283408 [details]
Patch
*** Bug 13968 has been marked as a duplicate of this bug. *** 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 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 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 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 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 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 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 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
Created attachment 283416 [details]
Removing new data URL error checks
(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? Created attachment 286693 [details]
Patch
(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 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. Created attachment 287251 [details]
Patch for landing
Patch is rebased against master. It also includes setting/testing statusText to "OK" Created attachment 287256 [details]
Patch for landing
Comment on attachment 287256 [details] Patch for landing Clearing flags on attachment: 287256 Committed r205113: <http://trac.webkit.org/changeset/205113> All reviewed patches have been landed. Closing bug. |