Bug 200567 - Blob registries should be keyed by session IDs
Summary: Blob registries should be keyed by session IDs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on: 200571 200572
Blocks:
  Show dependency treegraph
 
Reported: 2019-08-09 03:09 PDT by youenn fablet
Modified: 2019-08-13 14:27 PDT (History)
13 users (show)

See Also:


Attachments
Patch (179.07 KB, patch)
2019-08-09 03:17 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (84.00 KB, patch)
2019-08-12 05:48 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (84.46 KB, patch)
2019-08-12 06:10 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Rebasing (85.03 KB, patch)
2019-08-12 07:24 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
GTK (90.83 KB, patch)
2019-08-12 08:10 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Win fix (93.97 KB, patch)
2019-08-12 09:29 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (94.74 KB, patch)
2019-08-13 00:38 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (94.33 KB, patch)
2019-08-13 00:49 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 youenn fablet 2019-08-09 03:09:23 PDT
Blob registries should be keyed by session IDs
Comment 1 youenn fablet 2019-08-09 03:17:55 PDT
Created attachment 375903 [details]
Patch
Comment 2 youenn fablet 2019-08-09 04:05:05 PDT
Plan is to split the patch in smaller pieces.
Comment 3 Radar WebKit Bug Importer 2019-08-09 04:05:09 PDT
<rdar://problem/54120212>
Comment 4 Alex Christensen 2019-08-09 09:08:24 PDT
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.
Comment 5 youenn fablet 2019-08-09 09:32:12 PDT
(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.
Comment 6 Alex Christensen 2019-08-09 11:17:18 PDT
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.
Comment 7 youenn fablet 2019-08-09 13:42:11 PDT
(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.
Comment 8 youenn fablet 2019-08-12 05:48:57 PDT
Created attachment 376056 [details]
Rebasing
Comment 9 youenn fablet 2019-08-12 06:10:33 PDT
Created attachment 376057 [details]
Rebasing
Comment 10 Build Bot 2019-08-12 06:12:00 PDT
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.
Comment 11 youenn fablet 2019-08-12 07:24:42 PDT
Created attachment 376061 [details]
Rebasing
Comment 12 Build Bot 2019-08-12 07:27:14 PDT
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.
Comment 13 youenn fablet 2019-08-12 08:10:04 PDT
Created attachment 376063 [details]
GTK
Comment 14 Build Bot 2019-08-12 08:13:20 PDT
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.
Comment 15 youenn fablet 2019-08-12 09:29:52 PDT
Created attachment 376069 [details]
Win fix
Comment 16 Build Bot 2019-08-12 09:32:31 PDT
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 17 Alex Christensen 2019-08-12 13:33:01 PDT
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.
Comment 18 youenn fablet 2019-08-13 00:38:59 PDT
Created attachment 376142 [details]
Patch for landing
Comment 19 youenn fablet 2019-08-13 00:49:13 PDT
Created attachment 376143 [details]
Patch for landing
Comment 20 Build Bot 2019-08-13 00:51:14 PDT
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 21 WebKit Commit Bot 2019-08-13 03:06:23 PDT
Comment on attachment 376143 [details]
Patch for landing

Clearing flags on attachment: 376143

Committed r248593: <https://trac.webkit.org/changeset/248593>
Comment 22 WebKit Commit Bot 2019-08-13 03:06:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 youenn fablet 2019-08-13 03:44:18 PDT
(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.
Comment 24 youenn fablet 2019-08-13 04:15:12 PDT
(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.
Comment 25 Truitt Savell 2019-08-13 11:43:46 PDT
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.
Comment 26 youenn fablet 2019-08-13 14:27:19 PDT
(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.