Bug 233945 – ActiveDOMObject::suspendIfNeeded() should not be called within constructors

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Antoine Quint]

ActiveDOMObject::suspendIfNeeded() should not be called within constructors

[Antoine Quint]

Created attachment 446215 [details]
Patch

[Chris Dumez]

Comment on attachment 446215 [details]
Patch

r=me assuming the bots are happy

[Darin Adler]

Comment on attachment 446215 [details]
Patch

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

Really great patch. Too bad we can’t get the compiler or an assertion to catch this mistake, but this is nicely thorough.

Maybe we should rename this function suspendIfNeededDoNotCallInConstructor (just kidding, but maybe not kidding).

> Source/WebCore/Modules/cache/DOMCache.h:40
> +    static Ref<DOMCache> create(ScriptExecutionContext&, String&&, uint64_t, Ref<CacheStorageConnection>&&);

Not sure that we should remove *all* of these names. The argument names "name" and "identifier" are probably still helpful.

> Source/WebCore/Modules/geolocation/Geolocation.cpp:137
> -    geolocation.get().suspendIfNeeded();
> +    geolocation->suspendIfNeeded();

Ah, brevity competes with "looks like a risky pointer dereference, but is not". I think the more terse version is better, and it’s what we use almost everywhere, but I am conflicted. There’s something cool about ".get()." not looking like a pointer is involved.

[Antoine Quint]

(In reply to Darin Adler from comment #3)
> Comment on attachment 446215 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=446215&action=review
> 
> Really great patch. Too bad we can’t get the compiler or an assertion to
> catch this mistake, but this is nicely thorough.

Not quite! There are 17 cases left to fix, those were just the easy ones. I'll look into fixing the other ones piecemeal.

> Maybe we should rename this function suspendIfNeededDoNotCallInConstructor
> (just kidding, but maybe not kidding).
> 
> > Source/WebCore/Modules/cache/DOMCache.h:40
> > +    static Ref<DOMCache> create(ScriptExecutionContext&, String&&, uint64_t, Ref<CacheStorageConnection>&&);
> 
> Not sure that we should remove *all* of these names. The argument names
> "name" and "identifier" are probably still helpful.

Will restore those.

> > Source/WebCore/Modules/geolocation/Geolocation.cpp:137
> > -    geolocation.get().suspendIfNeeded();
> > +    geolocation->suspendIfNeeded();
> 
> Ah, brevity competes with "looks like a risky pointer dereference, but is
> not". I think the more terse version is better, and it’s what we use almost
> everywhere, but I am conflicted. There’s something cool about ".get()." not
> looking like a pointer is involved.

I personally don't mind either way, I stumbled onto this code looking at all call sites to ActiveDOMObject::suspendIfNeeded() and it was the only call on a Ref that called .get(). Felt right to put it in line with the rest.

[Darin Adler]

(In reply to Antoine Quint from comment #4)
> Not quite! There are 17 cases left to fix, those were just the easy ones.
> I'll look into fixing the other ones piecemeal.

!!

> Felt right to put it in line with the rest.

Agreed.

[Antoine Quint]

Created attachment 447120 [details]
Patch for landing

[Radar WebKit Bug Importer]

<rdar://problem/86481861>

[EWS]

Committed r287066 (245261@main): <https://commits.webkit.org/245261@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447120 [details].

[Antoine Quint]

Lots to do on this left, this first commit was only part of the work.

[Antoine Quint]

Created attachment 447234 [details]
Patch

[Chris Dumez]

Comment on attachment 447234 [details]
Patch

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

r=me with suggestions.

> Source/WebCore/Modules/indexeddb/IDBIndex.cpp:45
> +std::unique_ptr<IDBIndex> IDBIndex::create(ScriptExecutionContext& context, const IDBIndexInfo& info, IDBObjectStore& objectStore)

We may want to return a UniqueRef instead since this cannot return null.

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:58
> +std::unique_ptr<IDBObjectStore> IDBObjectStore::create(ScriptExecutionContext& context, const IDBObjectStoreInfo& info, IDBTransaction& transaction)

ditto.

[Antoine Quint]

Created attachment 447245 [details]
Patch

[Chris Dumez]

Comment on attachment 447245 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:532
> +    m_referencedIndexes.set(indexName, std::unique_ptr<IDBIndex>(&index));

You want `index.moveToUniquePtr()`. This adopts the pointer so you will double-free.

[Antoine Quint]

Created attachment 447262 [details]
Patch

[Antoine Quint]

Created attachment 447270 [details]
Patch

[Darin Adler]

Comment on attachment 447270 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:530
> +    Ref<IDBIndex> referencedIndex { index };

Since we’re touching this code, it can likely be changed to Ref instead of Ref<IDBIndex> since we have type deduction now.

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:753
> +    auto index = IDBIndex::create(*scriptExecutionContext(), info, objectStore);
> +    return index.moveToUniquePtr();

Should just be able to write:

    return IDBIndex::create(*scriptExecutionContext(), info, objectStore);

Move construction of unique_ptr from UniqueRef should work.

[Antoine Quint]

Created attachment 447279 [details]
Patch for landing

[Antoine Quint]

Created attachment 447327 [details]
Patch for landing

[Antoine Quint]

Committed r287129 (?): <https://commits.webkit.org/r287129>

[Antoine Quint]

Created attachment 447346 [details]
Patch

[Antoine Quint]

Created attachment 447349 [details]
Patch

[Darin Adler]

Comment on attachment 447349 [details]
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?

[Antoine Quint]

(In reply to Darin Adler from comment #22)
> Comment on attachment 447349 [details]
> 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.

That was exactly my thought. If I were subclassing a class and looking at an existing subclass, I would see `initialize()` and think "I better call this", but if I saw `suspendIfNeeded()` without knowing necessarily much about ActiveDOMObject, it might not be as clear that it should be called.

But this is just me, and it might in fact be a missed opportunity for the person subclassing to ask themselves the right questions and engage with a domain expert about what `suspendIfNeeded()` is.

So I'm leaning towards removing initialize() and replacing its calls with `suspendIfNeeded()`.

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

I'll put it back.

> > 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"?

Yes, copying-and-pasting without thinking.

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

OK, I will switch to a forward declaration and hopefully the bots will prove that is just fine.

> > 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?

I don't know, but the ActiveDOMObject constructor takes a ScriptExecutionContext* and this just passes this along to that constructor:

ActiveDOMObject::ActiveDOMObject(ScriptExecutionContext* scriptExecutionContext)
    : ActiveDOMObject(suitableScriptExecutionContext(scriptExecutionContext), CheckedScriptExecutionContext)
{
}

… which calls this method:

static inline ScriptExecutionContext* suitableScriptExecutionContext(ScriptExecutionContext* scriptExecutionContext)
{
    // For detached documents, make sure we observe their context document instead.
    return is<Document>(scriptExecutionContext) ? &downcast<Document>(*scriptExecutionContext).contextDocument() : scriptExecutionContext;
}

… and calls this other constructor:

inline ActiveDOMObject::ActiveDOMObject(ScriptExecutionContext* context, CheckedScriptExecutionContextType)
    : ContextDestructionObserver(context)
{
    ASSERT(!is<Document>(context) || &downcast<Document>(context)->contextDocument() == downcast<Document>(context));
    if (!context)
        return;

    ASSERT(context->isContextThread());
    context->didCreateActiveDOMObject(*this);
}

[Antoine Quint]

Created attachment 447437 [details]
Patch for landing

[Antoine Quint]

Committed r287177 (245346@trunk): <https://commits.webkit.org/245346@trunk>

[Antoine Quint]

Created attachment 447447 [details]
Patch

[Antoine Quint]

Created attachment 447448 [details]
Patch

[Chris Dumez]

Comment on attachment 447448 [details]
Patch

r=me

[EWS]

Committed r287186 (245353@main): <https://commits.webkit.org/245353@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447448 [details].

[Darin Adler]

We should also consider adding a checker that asserts if we make this error, to make sure this mistake doesn’t creep back in. I have some ideas about how to add that, for any objects that are put into our Ref/RefPtr/UniqueRef after creating them. No idea how to do the same for objects that go into unique_ptr, though.