This is just refactoring.
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>