Bug 213037

Created attachment 401560 [details]
Patch

Comment on attachment 401560 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:46
> +    , m_pendingActivity(nullptr)

Please remove. Not needed. RefPtr is initialized to nullptr without explicit initialization.

> Source/WebCore/Modules/fetch/FetchBodyOwner.h:73
> +    void setPendingActivity()

Is there a reason these function bodies need to be entirely inlined in the header? That seems like premature optimization. Let’s put them into the .cpp file instead.

> Source/WebCore/Modules/fetch/FetchBodyOwner.h:85
> +    void unsetPendingActivity()
> +    {
> +        if (m_pendingActivity)
> +            m_pendingActivity->deref();
> +    }

This seems like it needs to ASSERT(m_pendingActivity), not just silently do nothing.

This is wrong and will over-release. If you call setPendingActivity(), then unsetPendingActivity(), and then delete the FetchBodyOwner, m_pendingActivity will be ref'd once, but deref'd twice.

Explicit calls to ref/deref are almost never correct, and this shows one example of why.

I'll rethink this patch.

Created attachment 401652 [details]
Patch

Comment on attachment 401652 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBodyOwner.h:138
> +    unsigned m_pendingInstanceCount { 0 };

I seem that it might be good to provide a way to increment/decrement ActiveDOMObject::m_pendingActivityInstanceCount instead of holding and manipulate this member.

Why did we obsolete ActiveDOMObject::setPendingActivity() in bug 209754?
I could not find the reason for its change.

Chris, maybe you can help?

Comment on attachment 401652 [details]
Patch

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

>> Source/WebCore/Modules/fetch/FetchBodyOwner.h:138
>> +    unsigned m_pendingInstanceCount { 0 };
> 
> I seem that it might be good to provide a way to increment/decrement ActiveDOMObject::m_pendingActivityInstanceCount instead of holding and manipulate this member.
> 
> Why did we obsolete ActiveDOMObject::setPendingActivity() in bug 209754?
> I could not find the reason for its change.

mismatched setPendingActivity() / unsetPendingActivity() calls was a frequent source of leaks. The modern way to keep the JS wrapper alive is to call makePendingActivity() and store the returned object, as documented in the code. This makes it a bit harder to make mistakes.

Comment on attachment 401652 [details]
Patch

I don't think the new code is any better than the old one. You just moved the problem from ActiveDOMObject to FetchBodyOwner.

Comment on attachment 401652 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:260
> +    setPendingActivity();

This one..

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:269
> +    unsetPendingActivity();

... and this one are easily avoidable by..

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:391
> +    return m_pendingInstanceCount > 0;

... doing this:
return !!m_blobLoader;

This way, the wrapper will be kept alive as long as we have blob loader.

> Source/WebCore/Modules/fetch/FetchBodySource.cpp:45
> +        m_bodyOwner->setPendingActivity();

For these, maybe the caller should call makePendingActivity() instead and store the pending activity here.

> Source/WebCore/Modules/fetch/FetchBodySource.cpp:52
> +        m_bodyOwner->unsetPendingActivity();

Then clear the pending activity they stored, here.

Thank you, Chris.

I'll address the patch.

Created attachment 401722 [details]
Patch

Comment on attachment 401722 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:-259
> -    setPendingActivity(*this);

We are no longer refing the FetchBodyOwner while loading the blo.
This is a potential behavioural change if we do not have a JSFetchRequest/JSFetchResponse to keep the FetchBodyOwner alive.
Looking at the call sites, this should be fine.

> Source/WebCore/Modules/fetch/FetchBodyOwner.h:106
> +    bool virtualHasPendingActivity() const override;

Could probably be final.

> Source/WebCore/Modules/fetch/FetchBodySource.cpp:53
>      if (m_bodyOwner)

No need for this if check.

(In reply to youenn fablet from comment #12)
> Comment on attachment 401722 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401722&action=review
> 
> > Source/WebCore/Modules/fetch/FetchBodyOwner.cpp:-259
> > -    setPendingActivity(*this);
> 
> We are no longer refing the FetchBodyOwner while loading the blo.
> This is a potential behavioural change if we do not have a
> JSFetchRequest/JSFetchResponse to keep the FetchBodyOwner alive.
> Looking at the call sites, this should be fine.

I confirmed that JSFetchRequest/JSFetchResponse (as derived class of JSDOMWrapper) hold FetchRequest/FetchResponse, they have FetchBodyOwner as a base class and keep alive it.

This is just a question.

FetchBodyOwner holds FetchBodySource, and FetchBodySource holds FetchBodyOwner as a raw pointer because of cutting the reference cycle. Is my recognization right?

Created attachment 401740 [details]
Patch

I addressed youenn's review and updated changelog.

Committed r262972: <https://trac.webkit.org/changeset/262972>

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

<rdar://problem/64310110>