Bug 175482

Created attachment 317939 [details] WIP Patch

Attachment 317939 [details] did not pass style-queue: ERROR: Source/WebCore/loader/cache/CachedResourceLoader.cpp:844: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1 in 31 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 317941 [details] WIP Patch

Created attachment 317951 [details] WIP Patch

Created attachment 317955 [details] Patch

Comment on attachment 317955 [details] Patch Attachment 317955 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4297646 New failing tests: imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count.html imported/w3c/web-platform-tests/fetch/http-cache/cc-request.html inspector/worker/resources-in-worker.html imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-worker.html imported/w3c/web-platform-tests/fetch/api/request/request-cache-only-if-cached.html

Created attachment 317961 [details] Archive of layout-test-results from ews101 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 317955 [details] Patch Attachment 317955 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4297675 New failing tests: imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count.html inspector/worker/resources-in-worker.html imported/w3c/web-platform-tests/fetch/api/redirect/redirect-count-worker.html

Created attachment 317965 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Created attachment 317968 [details] Patch

Created attachment 317971 [details] Patch

Comment on attachment 317971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317971&action=review > Source/WebCore/loader/DocumentThreadableLoader.cpp:455 > + ResourceError error; > + m_resource = m_document.cachedResourceLoader().requestRawResource(WTFMove(newRequest), &error); Rather than out parameters, what do you think about adding a ResourceErrorOr<> class?

(In reply to Sam Weinig from comment #12) > Comment on attachment 317971 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=317971&action=review > > > Source/WebCore/loader/DocumentThreadableLoader.cpp:455 > > + ResourceError error; > > + m_resource = m_document.cachedResourceLoader().requestRawResource(WTFMove(newRequest), &error); > > Rather than out parameters, what do you think about adding a > ResourceErrorOr<> class? I tried to keep the refactoring as minimal as possible in this patch to limit size but I do agree that it'd be nice if we used a better design. I don't know that this deserves a new class though? I was personally considering WTF::Expected<CachedResourceHandle<CachedResource>>, ResourceError>.

(In reply to Chris Dumez from comment #13) > (In reply to Sam Weinig from comment #12) > > Comment on attachment 317971 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=317971&action=review > > > > > Source/WebCore/loader/DocumentThreadableLoader.cpp:455 > > > + ResourceError error; > > > + m_resource = m_document.cachedResourceLoader().requestRawResource(WTFMove(newRequest), &error); > > > > Rather than out parameters, what do you think about adding a > > ResourceErrorOr<> class? > > I tried to keep the refactoring as minimal as possible in this patch to > limit size but I do agree that it'd be nice if we used a better design. I > don't know that this deserves a new class though? I was personally > considering WTF::Expected<CachedResourceHandle<CachedResource>>, > ResourceError>. That would be cool too.

(In reply to Sam Weinig from comment #14) > (In reply to Chris Dumez from comment #13) > > (In reply to Sam Weinig from comment #12) > > > Comment on attachment 317971 [details] > > > Patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=317971&action=review > > > > > > > Source/WebCore/loader/DocumentThreadableLoader.cpp:455 > > > > + ResourceError error; > > > > + m_resource = m_document.cachedResourceLoader().requestRawResource(WTFMove(newRequest), &error); > > > > > > Rather than out parameters, what do you think about adding a > > > ResourceErrorOr<> class? > > > > I tried to keep the refactoring as minimal as possible in this patch to > > limit size but I do agree that it'd be nice if we used a better design. I > > don't know that this deserves a new class though? I was personally > > considering WTF::Expected<CachedResourceHandle<CachedResource>>, > > ResourceError>. > > That would be cool too. Will do this refactoring in a follow-up. I think it will look better indeed.

Created attachment 317976 [details] Patch

Comment on attachment 317971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317971&action=review > Source/WebCore/ChangeLog:11 > + This partly works for Beacon was well, meaning that no Beacon with a body s/was/as. > Source/WebCore/Modules/fetch/FetchBodyOwner.h:91 > + void didFail(const ResourceError&) final; Should probably be a ResourceError&& although in practice, ResourceError might not allow us today to move some of its internals like its description. > Source/WebCore/loader/cache/CachedResourceLoader.cpp:184 > +CachedResourceHandle<CachedImage> CachedResourceLoader::requestImage(CachedResourceRequest&& request, ResourceError* error) Couldn't we try passing the error through CachedResourceHandle? > Source/WebCore/loader/cache/CachedResourceLoader.cpp:831 > + if (resource->options().keepAlive && !m_keepaliveRequestTracker.canLoadRequest(*resource)) { I am not sure we should do that there. This is probably fine in practice for fetch but in the future this assumption might be broken. Why not doing it in CachedResource::load() instead? > Source/WebCore/loader/cache/KeepaliveRequestTracker.cpp:27 > +#include "KeepaliveRequestTracker.h" Not sure what is the rule but KeepAlive would read better than Keepalive to me. > Source/WebCore/loader/cache/KeepaliveRequestTracker.cpp:31 > +const uint64_t maxInflightKeepaliveBytes { 65536 }; // 64 kibibytes as per Fetch specification. s/kibi/kilo/ > Source/WebCore/platform/network/FormData.cpp:143 > + auto* blobData = static_cast<BlobRegistryImpl&>(blobRegistry()).getBlobDataFromURL(m_url); I am not sure this actually works to go through blobRegistry. Do we have tests for that case (blob+keepalive)? To get the size of the blob, we should probably get the actual Blob and read its size directly. Unfortunately, this is only known at FetchRequest level so there would be a need to pass it all through CachedResourceLoader.

cdumez, before committing, can you check the blob+keepalive first?

In the meantime, I'll stop the cq.

Comment on attachment 317971 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317971&action=review >> Source/WebCore/ChangeLog:11 >> + This partly works for Beacon was well, meaning that no Beacon with a body > > s/was/as. Ok. >> Source/WebCore/Modules/fetch/FetchBodyOwner.h:91 >> + void didFail(const ResourceError&) final; > > Should probably be a ResourceError&& although in practice, ResourceError might not allow us today to move some of its internals like its description. None of the clients actually want to take ownership of the ResourceError. Also, in requestResource(), there is one instance we we return an error from the resource. This would force me to copy the error in this case if I used a ResourceError&&. >> Source/WebCore/loader/cache/CachedResourceLoader.cpp:184 >> +CachedResourceHandle<CachedImage> CachedResourceLoader::requestImage(CachedResourceRequest&& request, ResourceError* error) > > Couldn't we try passing the error through CachedResourceHandle? I don't know that I would like this design. I think of a CachedResourceHandle similarly as a RefPtr, it is ref counting for resources. I'd rather use WTF::Expected as discussed with Sam earlier. I already filed a bug about refactoring this in a follow-up to minimize the size of this patch. >> Source/WebCore/loader/cache/CachedResourceLoader.cpp:831 >> + if (resource->options().keepAlive && !m_keepaliveRequestTracker.canLoadRequest(*resource)) { > > I am not sure we should do that there. > This is probably fine in practice for fetch but in the future this assumption might be broken. > Why not doing it in CachedResource::load() instead? Hmm. I can probably do this in CachedResource::load(). I'll try it out and see how it looks. >> Source/WebCore/loader/cache/KeepaliveRequestTracker.cpp:27 >> +#include "KeepaliveRequestTracker.h" > > Not sure what is the rule but KeepAlive would read better than Keepalive to me. I am not sure what the rule is either so I went with the casing that is used in the fetch spec and in the IDL. >> Source/WebCore/loader/cache/KeepaliveRequestTracker.cpp:31 >> +const uint64_t maxInflightKeepaliveBytes { 65536 }; // 64 kibibytes as per Fetch specification. > > s/kibi/kilo/ Why? The spec says "64 kibibytes". >> Source/WebCore/platform/network/FormData.cpp:143 >> + auto* blobData = static_cast<BlobRegistryImpl&>(blobRegistry()).getBlobDataFromURL(m_url); > > I am not sure this actually works to go through blobRegistry. > Do we have tests for that case (blob+keepalive)? > > To get the size of the blob, we should probably get the actual Blob and read its size directly. > Unfortunately, this is only known at FetchRequest level so there would be a need to pass it all through CachedResourceLoader. I tried a blob like new Blob(["123"]) and it seemed to work but I'll confirm.

(In reply to youenn fablet from comment #18) > cdumez, before committing, can you check the blob+keepalive first? It does appear you're right and Blob size is not properly computed. I thought I tested this earlier.. Will look into this.

(In reply to Chris Dumez from comment #21) > (In reply to youenn fablet from comment #18) > > cdumez, before committing, can you check the blob+keepalive first? > > It does appear you're right and Blob size is not properly computed. I > thought I tested this earlier.. Will look into this. ThreadableBlobRegistry::blobSize(m_url) seems to work.

Created attachment 317984 [details] Patch

Created attachment 317985 [details] Patch

Comment on attachment 317985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317985&action=review > Source/WebCore/loader/cache/CachedResource.cpp:262 > + if (m_options.keepAlive) { Moved the logic to CachedResource::load() as suggested. > Source/WebCore/platform/network/FormData.cpp:142 > + return blobRegistry().blobSize(m_url); Now using this to retrieve the blob size, which seems to work. > LayoutTests/http/wpt/beacon/beacon-quota.html:10 > + return new Blob(["*".repeat(payloadSize)]); Updated this test to use a Blob for better coverage.

Comment on attachment 317985 [details] Patch LGTM. View in context: https://bugs.webkit.org/attachment.cgi?id=317985&action=review > Source/WebCore/loader/cache/KeepaliveRequestTracker.cpp:87 > + m_inflightKeepaliveBytes -= resource.resourceRequest().httpBody()->lengthInBytes(); I wonder whether we should not have m_inflightKeepaliveRequests be a map<request, size_t> resource.resourceRequest() could be modified throughout the load and become null for instance. Or the blob registry may change and lengthInBytes be changed inadvertently. I also wonder whether we should ensure that m_inflightKeepaliveBytes is greater than resource.resourceRequest().httpBody()->lengthInBytes()

Comment on attachment 317985 [details] Patch Clearing flags on attachment: 317985 Committed r220622: <http://trac.webkit.org/changeset/220622>

All reviewed patches have been landed. Closing bug.

<rdar://problem/33860695>

> I wonder whether we should not have m_inflightKeepaliveRequests be a > map<request, size_t> > resource.resourceRequest() could be modified throughout the load and become > null for instance. > Or the blob registry may change and lengthInBytes be changed inadvertently. Maybe something like: - create blob1 of 60ko - fetch(keepalive+blob1) on a script taking some time to answer - GC the blob so that the blob URL is no longer in the registry - wait for fetch to complete - create blob2 of 60ko - fetch(keepalive+blob2) and check that it does not reject

> Maybe something like: > - create blob1 of 60ko > - fetch(keepalive+blob1) on a script taking some time to answer > - GC the blob so that the blob URL is no longer in the registry > - wait for fetch to complete > - create blob2 of 60ko > - fetch(keepalive+blob2) and check that it does not reject Ah no, won't work

(In reply to youenn fablet from comment #31) > > Maybe something like: > > - create blob1 of 60ko > > - fetch(keepalive+blob1) on a script taking some time to answer > > - GC the blob so that the blob URL is no longer in the registry > > - wait for fetch to complete > > - create blob2 of 60ko > > - fetch(keepalive+blob2) and check that it does not reject > > Ah no, won't work Also note that FormData is caching is size so something would also need to invalidate the cached size for this to be an issue.

> Also note that FormData is caching is size so something would also need to > invalidate the cached size for this to be an issue. I wonder whether this is a good thing given that the cached size may be out of sync with its actual size. It seems safer to have KeepaliveRequestTracker cache this value.

(In reply to youenn fablet from comment #33) > > Also note that FormData is caching is size so something would also need to > > invalidate the cached size for this to be an issue. > > I wonder whether this is a good thing given that the cached size may be out > of sync with its actual size. It seems safer to have KeepaliveRequestTracker > cache this value. No, operations that would impact the size should invalidate the cached size. Also, I would not expect the request of a CachedResource to change.

> No, operations that would impact the size should invalidate the cached size. AFAIK, FormData is not aware of any change done to the blob registry. When a blob gets destroyed, how will FormData know to set its cached size to zero? If the same FormData is used in two different loads, that could end up be a problem. A small code change to fetch request cloning could make this real. > Also, I would not expect the request of a CachedResource to change. CachedResource has a public non-const accessor to the request right now. Not sure this is a good thing but hey... Any part of the code can actually nullify the body of the CachedResource request. I don't think this is the case right now, but I could see that done in some redirection/error cases for instance. If so, the current code could run into a null pointer dereference.

Comment on attachment 317985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317985&action=review > Source/WebCore/platform/network/FormData.cpp:137 > + if (getFileSize(m_shouldGenerateFile ? m_generatedFilename : m_filename, fileSize)) As a side note, is it ok to read the file size from WebProcess with regards to sandboxing?

(In reply to WebKit Commit Bot from comment #27) > Comment on attachment 317985 [details] > Patch > > Clearing flags on attachment: 317985 > > Committed r220622: <http://trac.webkit.org/changeset/220622> This change caused 38 LayoutTest crashes and 11 failures on iOS: https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Release%20WK2%20%28Tests%29/builds/3578

(In reply to Ryan Haddad from comment #37) > (In reply to WebKit Commit Bot from comment #27) > > Comment on attachment 317985 [details] > > Patch > > > > Clearing flags on attachment: 317985 > > > > Committed r220622: <http://trac.webkit.org/changeset/220622> > This change caused 38 LayoutTest crashes and 11 failures on iOS: > > https://build.webkit.org/builders/ > Apple%20iOS%2010%20Simulator%20Release%20WK2%20%28Tests%29/builds/3578 Follow-up for iOS landed in https://trac.webkit.org/changeset/220696/webkit.

Committed r220707: <http://trac.webkit.org/changeset/220707>

Rolled out in <http://trac.webkit.org/changeset/220707> due to iOS failures.

Created attachment 318048 [details] Patch

Created attachment 318051 [details] Patch

Created attachment 318058 [details] Patch

Comment on attachment 318058 [details] Patch Attachment 318058 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4313931 New failing tests: http/tests/navigation/page-cache-xhr.html imported/w3c/web-platform-tests/XMLHttpRequest/send-network-error-sync-events.sub.htm

Created attachment 318065 [details] Archive of layout-test-results from ews100 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Comment on attachment 318058 [details] Patch Attachment 318058 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4313947 New failing tests: http/tests/navigation/page-cache-xhr.html

Created attachment 318068 [details] Archive of layout-test-results from ews107 for mac-elcapitan-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6

Comment on attachment 318058 [details] Patch Attachment 318058 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4313948 New failing tests: http/tests/navigation/page-cache-xhr.html imported/w3c/web-platform-tests/XMLHttpRequest/send-network-error-sync-events.sub.htm

Created attachment 318073 [details] Archive of layout-test-results from ews115 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-elcapitan Platform: Mac OS X 10.11.6

Created attachment 318097 [details] Patch

Comment on attachment 318097 [details] Patch Attachment 318097 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/4315715 New failing tests: imported/w3c/web-platform-tests/cors/response-headers.htm imported/w3c/web-platform-tests/cors/redirect-userinfo.htm imported/w3c/web-platform-tests/fetch/api/policies/referrer-origin-when-cross-origin-worker.html imported/w3c/web-platform-tests/fetch/api/request/request-bad-port.html imported/w3c/web-platform-tests/eventsource/interfaces.html fast/history/pagehide-remove-iframe-crash.html imported/w3c/web-platform-tests/XMLHttpRequest/getresponseheader-error-state.htm imported/w3c/web-platform-tests/eventsource/eventsource-constructor-non-same-origin.htm imported/w3c/web-platform-tests/XMLHttpRequest/timeout-cors-async.htm imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies.any.html imported/w3c/web-platform-tests/XMLHttpRequest/event-error.sub.html imported/w3c/web-platform-tests/fetch/api/policies/referrer-origin-when-cross-origin.html imported/w3c/web-platform-tests/fetch/api/cors/cors-cookies.any.worker.html imported/w3c/web-platform-tests/cors/late-upload-events.htm

Created attachment 318110 [details] Archive of layout-test-results from ews126 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.5

Created attachment 318131 [details] Patch

Comment on attachment 318131 [details] Patch Clearing flags on attachment: 318131 Committed r220751: <http://trac.webkit.org/changeset/220751>

All reviewed patches have been landed. Closing bug.