[webkit-reviews] review granted: [Bug 179846] ASSERTION FAILED: registration in WebCore::SWServerJobQueue::scriptContextStarted(ServiceWorkerIdentifier) : [Attachment 327329] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 179846: ASSERTION FAILED: registration in
WebCore::SWServerJobQueue::scriptContextStarted(ServiceWorkerIdentifier)
https://bugs.webkit.org/show_bug.cgi?id=179846

Attachment 327329: Patch

https://bugs.webkit.org/attachment.cgi?id=327329&action=review




--- Comment #5 from Darin Adler <darin at apple.com> ---
Comment on attachment 327329
  --> https://bugs.webkit.org/attachment.cgi?id=327329
Patch

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

> Source/WTF/wtf/ObjectIdentifier.h:96
> +    static uint64_t currentIdentifier = 0;

No need for the "= 0" here.

> Source/WTF/wtf/ObjectIdentifier.h:100
> +template<typename T> inline ObjectIdentifier<T>
generateThreadSafeObjectIdentifier()

Nothing prevents us from accidentally using generateThreadSafeObjectIdentifier
and generateObjectIdentifier for the same type. Might be nice if there was
something that made sure that never happened.

> Source/WTF/wtf/ObjectIdentifier.h:102
> +    static NeverDestroyed<std::atomic<uint64_t>> currentIdentifier;

Does std::atomic have a non-trivial destructor? I am surprised we have to use
NeverDestroyed here.

What prevents two threads from racing to construct the std::atomic<uint64_t>?
We turn off the code that normally protects globals from this kind of problem
with a compiler flag. I fear that while access to the atomic is safely guarded
against use from multiple threads, construction may not be.

> Source/WebCore/workers/service/ServiceWorkerJobDataIdentifier.h:41
> +    bool operator==(const ServiceWorkerJobDataIdentifier& other) const
> +    {
> +	   return connectionIdentifier == other.connectionIdentifier &&
jobIdentifier == other.jobIdentifier;
> +    }

Stylistically, I slightly prefer to make this a non-member function. Generally
speaking it’s better because then conversions can happen on the left side too,
and no special access to the struct internals is needed.