[webkit-reviews] review granted: [Bug 233945] ActiveDOMObject::suspendIfNeeded() should not be called within constructors : [Attachment 447349] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antoine Quint <graouts at webkit.org>'s
request for review:
Bug 233945: ActiveDOMObject::suspendIfNeeded() should not be called within
constructors
https://bugs.webkit.org/show_bug.cgi?id=233945

Attachment 447349: Patch

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




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

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

Looks fantastic

> Source/WebCore/Modules/entriesapi/FileSystemEntry.cpp:57
> +void FileSystemEntry::initialize()
> +{
> +    suspendIfNeeded();
> +}

Why do we need these initialize functions instead of calling suspendIfNeeded
directly?

I’m sure that you thought about a reason, but I don’t know what it is. Would be
nice to have one less function if there’s no abstraction here and
suspendIfNeeded would do the same thing.

If what we’re trying to mitigate is callers neglecting to call suspendIfNeeded,
I’m not sure that renaming it to initialize will help. If we do want to tackle
that problem there may be some compile time techniques or some runtime ones,
like adding a debug-only boolean set every time suspendIfNeeded and finding a
good place to assert afterward to catch cases where it wasn’t called even once.
Could even use a micro-task to do that assertion.

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:60
> +void FetchBodyOwner::initialize()
> +{
> +    suspendIfNeeded();
> +}

Ditto.

> Source/WebCore/Modules/fetch/FetchRequest.h:57
> +    static Ref<FetchRequest> create(ScriptExecutionContext&,
std::optional<FetchBody>&&, Ref<FetchHeaders>&&, ResourceRequest&&,
FetchOptions&&, String&&);

Not sure we should have removed the name "referrer".

> Source/WebCore/Modules/filesystemaccess/FileSystemHandle.cpp:51
> +void FileSystemHandle::initialize()
> +{
> +    suspendIfNeeded();
> +}

Ditto.

> Source/WebCore/Modules/mediastream/RTCDTMFSender.cpp:47
> +    auto result = adoptRef(* new RTCDTMFSender(context, sender,
WTFMove(backend)));

Maybe get rid of the space between the "*" and "new"?

> Source/WebCore/workers/service/ServiceWorkerContainer.h:44
> +#include <wtf/UniqueRef.h>

Not something that should block landing this patch as-is, but: This should
require only a forward-declaration of UniqueRef, which should be provided by
Forward.h. Files where ServiceWorkerContainer::create are called will need
UniquerRef.h, but this header should not require it.

> Source/WebCore/workers/service/ServiceWorkerContainer.h:61
> +    static UniqueRef<ServiceWorkerContainer> create(ScriptExecutionContext*,
NavigatorBase&);

Another thing not about this patch: Do you know why this takes
ScriptExecutionContext* rather than ScriptExecutionContext&? Is nullptr OK?