Bug 200567 – Blob registries should be keyed by session IDs

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[youenn fablet]

Blob registries should be keyed by session IDs

[youenn fablet]

Created attachment 375903 [details]
Patch

[youenn fablet]

Plan is to split the patch in smaller pieces.

[Radar WebKit Bug Importer]

<rdar://problem/54120212>

[Alex Christensen]

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.

[youenn fablet]

(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.

[Alex Christensen]

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.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 376056 [details]
Rebasing

[youenn fablet]

Created attachment 376057 [details]
Rebasing

[EWS Watchlist]

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.

[youenn fablet]

Created attachment 376061 [details]
Rebasing

[EWS Watchlist]

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.

[youenn fablet]

Created attachment 376063 [details]
GTK

[EWS Watchlist]

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.

[youenn fablet]

Created attachment 376069 [details]
Win fix

[EWS Watchlist]

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.

[Alex Christensen]

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.

[youenn fablet]

Created attachment 376142 [details]
Patch for landing

[youenn fablet]

Created attachment 376143 [details]
Patch for landing

[EWS Watchlist]

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.

[WebKit Commit Bot]

Comment on attachment 376143 [details]
Patch for landing

Clearing flags on attachment: 376143

Committed r248593: <https://trac.webkit.org/changeset/248593>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

(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.

[youenn fablet]

(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.

[Truitt Savell]

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.

[youenn fablet]

(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.

[Konstantin Tokarev]

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?

[youenn fablet]

(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.