Blob registries should be keyed by session IDs
Created attachment 375903 [details] Patch
Plan is to split the patch in smaller pieces.
<rdar://problem/54120212>
Comment on attachment 375903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=375903&action=review This looks great! Not sure about the media change in the middle though. And it needs to pass all the tests. > Source/WebCore/html/HTMLMediaElement.cpp:-1597 > -#if ENABLE(MEDIA_STREAM) This change needs an explanation.
(In reply to Alex Christensen from comment #4) > Comment on attachment 375903 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=375903&action=review > > This looks great! Not sure about the media change in the middle though. > And it needs to pass all the tests. Sure, I am splitting the patch in smaller pieces so that this is more reviewable. Ultimately, this patch should be reduced to NetworkProcess related code. > > Source/WebCore/html/HTMLMediaElement.cpp:-1597 > > -#if ENABLE(MEDIA_STREAM) > > This change needs an explanation. This is handled as part of bug 200570.
Also, I think we should make the NetworkSession own a BlobRegistryImpl rather than having another map with SessionID as the key. We can do that after the big switch, though.
(In reply to Alex Christensen from comment #6) > Also, I think we should make the NetworkSession own a BlobRegistryImpl > rather than having another map with SessionID as the key. We can do that > after the big switch, though. Yep, this is in the patch. We should do that for other maps, like IDB and SW.
Created attachment 376056 [details] Rebasing
Created attachment 376057 [details] Rebasing
Attachment 376057 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/BlobRegistry.h:71: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 376061 [details] Rebasing
Attachment 376061 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/BlobRegistry.h:71: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 1 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 376063 [details] GTK
Attachment 376063 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/soup/ResourceRequest.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/BlobRegistry.h:71: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 376069 [details] Win fix
Attachment 376069 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/soup/ResourceRequest.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/BlobRegistry.h:71: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 40 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 376069 [details] Win fix View in context: https://bugs.webkit.org/attachment.cgi?id=376069&action=review > Source/WebKitLegacy/win/WebCoreSupport/WebPlatformStrategies.cpp:81 > - return new BlobRegistryImpl; > + return new WebBlobRegistry; r=me We should look into the uses of isBlobRegistryImpl and see if they're still needed, now that the networking is all done in the network process in WebKit2.
Created attachment 376142 [details] Patch for landing
Created attachment 376143 [details] Patch for landing
Attachment 376143 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/soup/ResourceRequest.h:36: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/platform/network/BlobRegistry.h:71: Inline functions should not be in classes annotated with WEBCORE_EXPORT. Remove the macro from the class and apply it to each appropriate method, or move the inline function definition out-of-line. [build/webcore_export] [4] Total errors found: 2 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 376143 [details] Patch for landing Clearing flags on attachment: 376143 Committed r248593: <https://trac.webkit.org/changeset/248593>
All reviewed patches have been landed. Closing bug.
(In reply to Alex Christensen from comment #17) > Comment on attachment 376069 [details] > Win fix > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376069&action=review > > > Source/WebKitLegacy/win/WebCoreSupport/WebPlatformStrategies.cpp:81 > > - return new BlobRegistryImpl; > > + return new WebBlobRegistry; > > r=me > We should look into the uses of isBlobRegistryImpl and see if they're still > needed, now that the networking is all done in the network process in > WebKit2. I removed it from BlobRegistry but not fro BlobRegistryImpl. Will fix it.
(In reply to youenn fablet from comment #23) > (In reply to Alex Christensen from comment #17) > > Comment on attachment 376069 [details] > > Win fix > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=376069&action=review > > > > > Source/WebKitLegacy/win/WebCoreSupport/WebPlatformStrategies.cpp:81 > > > - return new BlobRegistryImpl; > > > + return new WebBlobRegistry; > > > > r=me > > We should look into the uses of isBlobRegistryImpl and see if they're still > > needed, now that the networking is all done in the network process in > > WebKit2. > > I removed it from BlobRegistry but not fro BlobRegistryImpl. > Will fix it. Actually, I also removed it in BlobRegistryImpl in this patch.
It looks like the changes in https://trac.webkit.org/changeset/248593/webkit broke http/tests/workers/service/service-worker-cache-api.https.html History: http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fservice-worker-cache-api.https.html It is crashing on Debug builds and timing out on some release builds. I was able to reproduce this locally by just running the test on debug against 248593 which crashed and 248592 which passed.
(In reply to Truitt Savell from comment #25) > It looks like the changes in https://trac.webkit.org/changeset/248593/webkit > > broke http/tests/workers/service/service-worker-cache-api.https.html > > History: > http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=http%2Ftests%2Fworkers%2Fservice%2Fservice- > worker-cache-api.https.html > > It is crashing on Debug builds and timing out on some release builds. > > I was able to reproduce this locally by just running the test on debug > against 248593 which crashed and 248592 which passed. This should be fixed by patch from bug 200671 that I land tomorrow morning.
Comment on attachment 376143 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=376143&action=review > Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:375 > + auto resolvedFormData = formData.resolveBlobReferences(blobRegistry().blobRegistryImpl()); I may be missing something, but AFAIU blobRegistryImpl() is non-null only in WebKitLegacy, while this function is called from NetworkProcess (via createHTTPBodyNSInputStream). Is it correct?
(In reply to Konstantin Tokarev from comment #27) > Comment on attachment 376143 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376143&action=review > > > Source/WebCore/platform/network/cf/FormDataStreamCFNet.cpp:375 > > + auto resolvedFormData = formData.resolveBlobReferences(blobRegistry().blobRegistryImpl()); > > I may be missing something, but AFAIU blobRegistryImpl() is non-null only in > WebKitLegacy, while this function is called from NetworkProcess (via > createHTTPBodyNSInputStream). Is it correct? NetworkResourceLoader makes sure to resolve blob references at construction time, see NetworkConnectionToWebProcess::resolveBlobReferences. That way, createHTTPBodyNSInputStream does not need to deal with blobs in WK2. Agreed the code is not straightforward.