Add support for sharing Shared Workers (including across WebProcesses).
<rdar://problem/88330666>
Created attachment 450537 [details] WIP Patch
Created attachment 450548 [details] WIP Patch
Created attachment 450554 [details] WIP Patch
Created attachment 450561 [details] WIP Patch
Created attachment 450565 [details] WIP Patch
Created attachment 450581 [details] WIP Patch
The patch is huge so I'll try and split parts of it out to facilitate review a bit.
Created attachment 450602 [details] WIP Patch
Created attachment 450610 [details] WIP Patch
Created attachment 450662 [details] WIP Patch
Created attachment 450683 [details] WIP Patch
Created attachment 450684 [details] WIP Patch
Created attachment 450691 [details] WIP Patch
Created attachment 450694 [details] WIP Patch
Created attachment 450710 [details] WIP Patch
Created attachment 450727 [details] WIP Patch
Created attachment 450777 [details] WIP Patch
Created attachment 450791 [details] WIP Patch
Created attachment 450801 [details] WIP Patch
Created attachment 450803 [details] WIP Patch
Created attachment 450806 [details] WIP Patch
Created attachment 450829 [details] WIP Patch
Created attachment 450840 [details] WIP Patch
Created attachment 450906 [details] WIP Patch
Created attachment 450921 [details] Patch
Created attachment 450928 [details] Patch
Created attachment 450934 [details] Patch
Created attachment 450943 [details] Patch
Created attachment 450954 [details] Patch
Comment on attachment 450954 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=450954&action=review Looks good, I didn’t spot any mistakes. > Source/WebCore/workers/shared/SharedWorkerKey.h:52 > + Hasher hasher; > + add(hasher, origin.hash()); > + add(hasher, url); > + add(hasher, name); > + return hasher.hash(); Instead, we should write: return computeHash(origin.hash(), url, name); That does the same thing. But also, it’s not good to hash a hash. Instead, we should write these functions: inline add(Hasher& hasher, const SharedWorkerKey& key) { hasher.add(key.origin, key.url, key.name); } inline add(Hasher& hasher, const ClientOrigin& origin) { add(hasher, origin.topOrigin, origin.clientOrigin); } inline add(Hasher& hasher, const SecurityOriginData& data) { add(hasher, data.protocol, data.host, data.port); } These don’t have to be inlined, and of course they should go in the appropriate headers. Once those functions are all defined, we can call computeHash on any of these types, without any hashing of a hash getting involved. If we want, we can still define SharedWorkerKey::hash, but it should just call computeHash: unsigned hash() const { return computeHash(*this); }
Forgot the "void" above: inline void add ... But otherwise the code should work.
Created attachment 451107 [details] Patch
Hmm, http/tests/cache-storage/cache-origins.https.html appears to be failing on EWS. I'll have to investigate. It is not involving Shared Workers but maybe I messed up the new hash functions somehow.
Comment on attachment 451107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451107&action=review > Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.h:32 > +#include <wtf/Hasher.h> > #include <wtf/text/StringHash.h> > #include <wtf/text/WTFString.h> We don’t need any of these includes. ClientOrigin.h already pulls them in (I’m not sure about StringHash, but if it’s needed it’s pulled in). > Source/WebCore/page/SecurityOriginData.h:103 > + add(hasher, data.protocol, data.host, data.port); If we wanted to more closely match the old hash function, this would need to be data.port.value_or(0) unless we want "no port" to hash differently than a port number. We probably do want them distinct just the way this is written, but I noticed it is a difference. > Source/WebCore/workers/shared/SharedWorkerKey.h:31 > +#include <wtf/Hasher.h> > +#include <wtf/URL.h> > +#include <wtf/text/WTFString.h> Don’t need this any of these. They are all included by ClientOrigin.h already.
Created attachment 451136 [details] Patch
(In reply to Darin Adler from comment #35) > Comment on attachment 451107 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451107&action=review > > > Source/WebCore/Modules/indexeddb/IDBDatabaseIdentifier.h:32 > > +#include <wtf/Hasher.h> > > #include <wtf/text/StringHash.h> > > #include <wtf/text/WTFString.h> > > We don’t need any of these includes. ClientOrigin.h already pulls them in > (I’m not sure about StringHash, but if it’s needed it’s pulled in). > > > Source/WebCore/page/SecurityOriginData.h:103 > > + add(hasher, data.protocol, data.host, data.port); > > If we wanted to more closely match the old hash function, this would need to > be data.port.value_or(0) unless we want "no port" to hash differently than a > port number. We probably do want them distinct just the way this is written, > but I noticed it is a difference. It appears this was the cause of the regression, thanks!
Comment on attachment 451107 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=451107&action=review >> Source/WebCore/page/SecurityOriginData.h:103 >> + add(hasher, data.protocol, data.host, data.port); > > If we wanted to more closely match the old hash function, this would need to be data.port.value_or(0) unless we want "no port" to hash differently than a port number. We probably do want them distinct just the way this is written, but I noticed it is a difference. You said this caused the regression. That is very mysterious! Because the == operator does not treat std::nullopt as equal to 0, so it should not matter whether the hash function does or not!
(In reply to Darin Adler from comment #38) > Comment on attachment 451107 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=451107&action=review > > >> Source/WebCore/page/SecurityOriginData.h:103 > >> + add(hasher, data.protocol, data.host, data.port); > > > > If we wanted to more closely match the old hash function, this would need to be data.port.value_or(0) unless we want "no port" to hash differently than a port number. We probably do want them distinct just the way this is written, but I noticed it is a difference. > > You said this caused the regression. That is very mysterious! Because the == > operator does not treat std::nullopt as equal to 0, so it should not matter > whether the hash function does or not! My bet is that this has to do with conversion of SecurityOriginData to and from String. In particular, the databaseIdentifier() / fromDatabaseIdentifier() implementation seem to treat a port that is 0 the same way has not having a port.
Still kind of mysterious, because in any context where we are converting to a string, then we should be hashing the string.
(In reply to Darin Adler from comment #40) > Still kind of mysterious, because in any context where we are converting to > a string, then we should be hashing the string. What I meant was: 1. Add a SecurityOriginData S1 "http://foo:0" to a HashMap H 2. Convert the SecurityOriginData S1 to a database identifier and save in database 3. Read identifier from database and convert back to a SecurityOriginData S2 "http://foo" (because fromDatabaseIdentifier() strips the port). 4. Check if S2 is present in HashMap H -> Says false (should probably say true?) That said, because operator==(S1, S2) would say false before and after my change, then it should say S2 is not in the map, even if the hashes for S1 and S2 were to match... I am perplexed too.
(In reply to Chris Dumez from comment #41) > (In reply to Darin Adler from comment #40) > > Still kind of mysterious, because in any context where we are converting to > > a string, then we should be hashing the string. > > What I meant was: > 1. Add a SecurityOriginData S1 "http://foo:0" to a HashMap H > 2. Convert the SecurityOriginData S1 to a database identifier and save in > database > 3. Read identifier from database and convert back to a SecurityOriginData S2 > "http://foo" (because fromDatabaseIdentifier() strips the port). > 4. Check if S2 is present in HashMap H -> Says false (should probably say > true?) > > That said, because operator==(S1, S2) would say false before and after my > change, then it should say S2 is not in the map, even if the hashes for S1 > and S2 were to match... > > I am perplexed too. I added some logging and we never hash a SecurityOriginData that has a port that is 0. All security origins getting hashed have a valid port, except for "file://" which has no port. Haven't had any success figuring out yet why the value_or(0) is needed but it is definitely what fixes the test.
Oh, I think I got it. The hashing is actually correct either way. What changes is the order of keys in the map (map of ClientOrigin keys). http/tests/cache-storage/cache-origins.https.html seems to rely on a specific ordering and needs to be fixed.
(In reply to Chris Dumez from comment #43) > http/tests/cache-storage/cache-origins.https.html seems to rely on a > specific ordering and needs to be fixed. Yes, WebKit::CacheStorage::Engine::representation needs to add things to the string in an order not based on hash table ordering.
Move the data into an array, sort the array, then build the string, I think.
(In reply to Darin Adler from comment #45) > Move the data into an array, sort the array, then build the string, I think. I uploaded a proposal at https://bugs.webkit.org/show_bug.cgi?id=236263. Just fixing the test.
Comment on attachment 451136 [details] Patch Clearing flags on attachment: 451136 Committed r289247 (246931@trunk): <https://commits.webkit.org/246931@trunk>
All reviewed patches have been landed. Closing bug.
https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fworkers%2Fsemantics%2Fmultiple-workers%2F004.html indicates that 246931@main caused imported/w3c/web-platform-tests/workers/semantics/multiple-workers/004.html to start flakily failing.