Bug 233288

Summary: Drop ServiceWorkerClientIdentifier / DocumentIdentifier and use ScriptExecutionContextIdentifier instead
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebCore Misc.Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, annulen, benjamin, cmarcelo, darin, eric.carlson, esprehn+autocc, ews-watchlist, ggaren, glenn, gyuyoung.kim, hta, japhet, jer.noble, kangil.han, philipj, ryuan.choi, sam, sergio, tommyw, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 232436, 233323    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Chris Dumez 2021-11-17 15:57:15 PST
Drop ServiceWorkerClientIdentifier / DocumentIdentifier and use ScriptExecutionContextIdentifier instead.

We need to generic identifier type to identify script execution contexts globally. The proposal is to augment ScriptExecutionContextIdentifier with a process identifier to make it global, and then get rid of ServiceWorkerClientIdentifier & DocumentIdentifier.
This is useful for Web Lock API support as we need a clientId that is consistent between the Web Lock API and the service worker API (tested by imported/w3c/web-platform-tests/web-locks//clientids.tentative.https.html).
Comment 1 Chris Dumez 2021-11-17 16:21:11 PST
Created attachment 444596 [details]
Patch
Comment 2 Darin Adler 2021-11-18 03:56:06 PST
Comment on attachment 444596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444596&action=review

> Source/WebCore/dom/Document.cpp:8728
> +        m_serviceWorkerConnection->unregisterServiceWorkerClient(contextIdentifier());

This change is not needed and goes against a comment saying this function is deprecated.

> Source/WebCore/dom/ScriptExecutionContext.cpp:120
> +    allScriptExecutionContextsMap().add(m_contextIdentifier, const_cast<ScriptExecutionContext*>(this));

No const_cast here wanted or needed.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:774
> +        request.setClientIdentifierIfNeeded(document->contextIdentifier());

Change not needed, as above. Unless I am missing something?

> Source/WebCore/platform/ProcessQualified.h:109
> +    encoder << m_object;

I’ve been wondering why we don’t write these kinds of functions as one-liners.

> Source/WebCore/platform/ProcessQualified.h:117
> +    decoder >> object;

We really need a better idiom for writing these structure decoders. I think we could find a way to cut down on repetitive code somehow.

> Source/WebCore/platform/ProcessQualified.h:124
> +    return ProcessQualified { *object, *processIdentifier };

Could use two sets of braces instead of naming the type here.

> Source/WebCore/platform/ScriptExecutionContextIdentifier.h:28
> +#include "ProcessQualified.h"

Can this be a forward declaration instead of an include?

> Source/WebCore/platform/ScriptExecutionContextIdentifier.h:29
>  #include <wtf/ObjectIdentifier.h>

Wondering the same.

> Source/WebCore/testing/Internals.cpp:2762
> +    return Document::allDocumentsMap().contains(ScriptExecutionContextIdentifier { makeObjectIdentifier<ScriptExecutionContextIdentifierType>(documentIdentifier), Process::identifier() });

Can this be written any more tersely without so many type names?

> Source/WebCore/workers/service/ServiceWorker.cpp:124
> +        sourceIdentifier = context.contextIdentifier();

Why contextIdentifier instead of just identifier?

> Source/WebCore/workers/service/ServiceWorkerClientData.cpp:73
> +        context.contextIdentifier(),

Why contextIdentifier instead of just identifier?

> Source/WebCore/workers/service/ServiceWorkerClients.cpp:63
> +        if (!identifier || !*identifier)

Not sure why we mix *identifier with identifier.value() here. Choose one I say.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:633
> +    return scriptExecutionContext()->contextIdentifier();

Why contextIdentifier instead of just identifier?

> Source/WebCore/workers/service/server/SWServerRegistration.cpp:177
> +    auto addResult = m_clientsUsingRegistration.ensure(clientIdentifier.processIdentifier(), [] {

This could use add instead of ensure. An empty HashSet is just a null pointer, near zero cost to construct.
Comment 3 Chris Dumez 2021-11-18 07:31:56 PST
(In reply to Darin Adler from comment #2)
> Comment on attachment 444596 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444596&action=review
> 
> > Source/WebCore/dom/Document.cpp:8728
> > +        m_serviceWorkerConnection->unregisterServiceWorkerClient(contextIdentifier());
> 
> This change is not needed and goes against a comment saying this function is
> deprecated.

Yes, that's my bad. I updated identifier() to be identical to contextIdentifier() last minute and failed to update the rest of my patch accordingly.
Comment 4 Chris Dumez 2021-11-18 08:23:00 PST
Created attachment 444678 [details]
Patch
Comment 5 Darin Adler 2021-11-18 08:40:40 PST
Comment on attachment 444678 [details]
Patch

Generated tests failure looks real
Comment 6 Chris Dumez 2021-11-18 08:41:41 PST
(In reply to Darin Adler from comment #5)
> Comment on attachment 444678 [details]
> Patch
> 
> Generated tests failure looks real

The new output looks like this though:
```
diff --git a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
index 630a865b13a6..52e3c7638390 100644
--- a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
+++ b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
@@ -127,6 +127,9 @@
 #endif
 #include "TestWithCVPixelBufferMessages.h"
 #if USE(AVFOUNDATION)
+#include <WebCore/CVUtilities.h>
+#endif
+#if USE(AVFOUNDATION)
 #include <wtf/RetainPtr.h>
 #endif
 
diff --git a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
index ee7defb81297..56496ddba5ea 100644
--- a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
+++ b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
@@ -32,6 +32,9 @@
 #include "HandleMessage.h"
 #include "TestWithCVPixelBufferMessages.h"
 #if USE(AVFOUNDATION)
+#include <WebCore/CVUtilities.h>
+#endif
+#if USE(AVFOUNDATION)
 #include <wtf/RetainPtr.h>
 #endif
 
diff --git a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
index 1675bea59439..326500eb0a0e 100644
--- a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
+++ b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
@@ -28,6 +28,9 @@
 #include "Connection.h"
 #include "MessageNames.h"
 #include "TestWithCVPixelBufferMessagesReplies.h"
+#if PLATFORM(COCOA)
+#include <WebCore/CVUtilities.h>
+#endif
 #include <wtf/Forward.h>
 #include <wtf/RetainPtr.h>
 #include <wtf/ThreadSafeRefCounted.h>
```

Not sure how it relates to my patch yet.
Comment 7 Chris Dumez 2021-11-18 08:42:34 PST
(In reply to Chris Dumez from comment #6)
> (In reply to Darin Adler from comment #5)
> > Comment on attachment 444678 [details]
> > Patch
> > 
> > Generated tests failure looks real
> 
> The new output looks like this though:
> ```
> diff --git
> a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> index 630a865b13a6..52e3c7638390 100644
> --- a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> +++ b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> @@ -127,6 +127,9 @@
>  #endif
>  #include "TestWithCVPixelBufferMessages.h"
>  #if USE(AVFOUNDATION)
> +#include <WebCore/CVUtilities.h>
> +#endif
> +#if USE(AVFOUNDATION)
>  #include <wtf/RetainPtr.h>
>  #endif
>  
> diff --git
> a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.
> cpp
> b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> index ee7defb81297..56496ddba5ea 100644
> ---
> a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> +++
> b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> @@ -32,6 +32,9 @@
>  #include "HandleMessage.h"
>  #include "TestWithCVPixelBufferMessages.h"
>  #if USE(AVFOUNDATION)
> +#include <WebCore/CVUtilities.h>
> +#endif
> +#if USE(AVFOUNDATION)
>  #include <wtf/RetainPtr.h>
>  #endif
>  
> diff --git
> a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> index 1675bea59439..326500eb0a0e 100644
> --- a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> +++ b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> @@ -28,6 +28,9 @@
>  #include "Connection.h"
>  #include "MessageNames.h"
>  #include "TestWithCVPixelBufferMessagesReplies.h"
> +#if PLATFORM(COCOA)
> +#include <WebCore/CVUtilities.h>
> +#endif
>  #include <wtf/Forward.h>
>  #include <wtf/RetainPtr.h>
>  #include <wtf/ThreadSafeRefCounted.h>
> ```
> 
> Not sure how it relates to my patch yet.

I think it was more likely caused by https://bugs.webkit.org/show_bug.cgi?id=231945.
Comment 8 Chris Dumez 2021-11-18 08:54:56 PST
(In reply to Chris Dumez from comment #7)
> (In reply to Chris Dumez from comment #6)
> > (In reply to Darin Adler from comment #5)
> > > Comment on attachment 444678 [details]
> > > Patch
> > > 
> > > Generated tests failure looks real
> > 
> > The new output looks like this though:
> > ```
> > diff --git
> > a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> > b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> > index 630a865b13a6..52e3c7638390 100644
> > --- a/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> > +++ b/Source/WebKit/Scripts/webkit/tests/MessageArgumentDescriptions.cpp
> > @@ -127,6 +127,9 @@
> >  #endif
> >  #include "TestWithCVPixelBufferMessages.h"
> >  #if USE(AVFOUNDATION)
> > +#include <WebCore/CVUtilities.h>
> > +#endif
> > +#if USE(AVFOUNDATION)
> >  #include <wtf/RetainPtr.h>
> >  #endif
> >  
> > diff --git
> > a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.
> > cpp
> > b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> > index ee7defb81297..56496ddba5ea 100644
> > ---
> > a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> > +++
> > b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessageReceiver.cpp
> > @@ -32,6 +32,9 @@
> >  #include "HandleMessage.h"
> >  #include "TestWithCVPixelBufferMessages.h"
> >  #if USE(AVFOUNDATION)
> > +#include <WebCore/CVUtilities.h>
> > +#endif
> > +#if USE(AVFOUNDATION)
> >  #include <wtf/RetainPtr.h>
> >  #endif
> >  
> > diff --git
> > a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> > b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> > index 1675bea59439..326500eb0a0e 100644
> > --- a/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> > +++ b/Source/WebKit/Scripts/webkit/tests/TestWithCVPixelBufferMessages.h
> > @@ -28,6 +28,9 @@
> >  #include "Connection.h"
> >  #include "MessageNames.h"
> >  #include "TestWithCVPixelBufferMessagesReplies.h"
> > +#if PLATFORM(COCOA)
> > +#include <WebCore/CVUtilities.h>
> > +#endif
> >  #include <wtf/Forward.h>
> >  #include <wtf/RetainPtr.h>
> >  #include <wtf/ThreadSafeRefCounted.h>
> > ```
> > 
> > Not sure how it relates to my patch yet.
> 
> I think it was more likely caused by
> https://bugs.webkit.org/show_bug.cgi?id=231945.

Youenn has a patch in-flight to do the rebaseline already.
Comment 9 Darin Adler 2021-11-18 08:57:59 PST
My mistake
Comment 10 Darin Adler 2021-11-18 08:58:50 PST
Comment on attachment 444678 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=444678&action=review

Post landing feedback

> Source/WebCore/platform/ScriptExecutionContextIdentifier.h:29
> +template <typename> class ObjectIdentifier;

Ideally this should be done with Forward.h so we don’t have to write WTF prefixes. If it wasn’t for the WTF prefixes thing I guess we could do individual forward declarations like this. But would hate for Forward.h include costs to undercut the savings from using forward declarations to cut down number of headers included to speed up compiles. Maybe we should eventually abandon the “using WTF::String” thing and write the WTF prefixes everywhere.

Would like to nail down the idiom so we aren’t making different choices each time.

> Source/WebCore/platform/ScriptExecutionContextIdentifier.h:34
> +template <typename> class ProcessQualified;

I slightly prefer the formatting without the space after “template”.
Comment 11 Chris Dumez 2021-11-18 09:07:40 PST
Created attachment 444688 [details]
Patch
Comment 12 EWS 2021-11-18 10:27:35 PST
Committed r286012 (244405@main): <https://commits.webkit.org/244405@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444688 [details].
Comment 13 Radar WebKit Bug Importer 2021-11-18 10:28:24 PST
<rdar://problem/85557507>