Bug 235958 – Add support for sharing Shared Workers (including across WebProcesses)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Add support for sharing Shared Workers (including across WebProcesses).

[Radar WebKit Bug Importer]

<rdar://problem/88330666>

[Chris Dumez]

Created attachment 450537 [details]
WIP Patch

[Chris Dumez]

Created attachment 450548 [details]
WIP Patch

[Chris Dumez]

Created attachment 450554 [details]
WIP Patch

[Chris Dumez]

Created attachment 450561 [details]
WIP Patch

[Chris Dumez]

Created attachment 450565 [details]
WIP Patch

[Chris Dumez]

Created attachment 450581 [details]
WIP Patch

[Chris Dumez]

The patch is huge so I'll try and split parts of it out to facilitate review a bit.

[Chris Dumez]

Created attachment 450602 [details]
WIP Patch

[Chris Dumez]

Created attachment 450610 [details]
WIP Patch

[Chris Dumez]

Created attachment 450662 [details]
WIP Patch

[Chris Dumez]

Created attachment 450683 [details]
WIP Patch

[Chris Dumez]

Created attachment 450684 [details]
WIP Patch

[Chris Dumez]

Created attachment 450691 [details]
WIP Patch

[Chris Dumez]

Created attachment 450694 [details]
WIP Patch

[Chris Dumez]

Created attachment 450710 [details]
WIP Patch

[Chris Dumez]

Created attachment 450727 [details]
WIP Patch

[Chris Dumez]

Created attachment 450777 [details]
WIP Patch

[Chris Dumez]

Created attachment 450791 [details]
WIP Patch

[Chris Dumez]

Created attachment 450801 [details]
WIP Patch

[Chris Dumez]

Created attachment 450803 [details]
WIP Patch

[Chris Dumez]

Created attachment 450806 [details]
WIP Patch

[Chris Dumez]

Created attachment 450829 [details]
WIP Patch

[Chris Dumez]

Created attachment 450840 [details]
WIP Patch

[Chris Dumez]

Created attachment 450906 [details]
WIP Patch

[Chris Dumez]

Created attachment 450921 [details]
Patch

[Chris Dumez]

Created attachment 450928 [details]
Patch

[Chris Dumez]

Created attachment 450934 [details]
Patch

[Chris Dumez]

Created attachment 450943 [details]
Patch

[Chris Dumez]

Created attachment 450954 [details]
Patch

[Darin Adler]

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); }

[Darin Adler]

Forgot the "void" above:

    inline void add ...

But otherwise the code should work.

[Chris Dumez]

Created attachment 451107 [details]
Patch

[Chris Dumez]

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.

[Darin Adler]

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.

[Chris Dumez]

Created attachment 451136 [details]
Patch

[Chris Dumez]

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

[Darin Adler]

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!

[Chris Dumez]

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

[Darin Adler]

Still kind of mysterious, because in any context where we are converting to a string, then we should be hashing the string.

[Chris Dumez]

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

[Chris Dumez]

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

[Chris Dumez]

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.

[Darin Adler]

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

[Darin Adler]

Move the data into an array, sort the array, then build the string, I think.

[Chris Dumez]

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

[Chris Dumez]

Comment on attachment 451136 [details]
Patch

Clearing flags on attachment: 451136

Committed r289247 (246931@trunk): <https://commits.webkit.org/246931@trunk>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Jonathan Bedard]

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.