ActiveDOMObject::hasPendingActivity() should stop preventing wrapper collection after ActiveDOMObject::stop() has been called
https://bugs.webkit.org/show_bug.cgi?id=209886
Reviewed by Ryosuke Niwa.
ActiveDOMObject::stop() gets called when the script execution context is about to be destroyed.
ActiveDOMObject objects should no longer run script after that and there is therefore no point
in keeping the JS wrapper alive once stop() has been called. Worse, depending on the
implementation of virtualHasPendingActivity(), keeping the wrapper alive past this point may
actually cause JS wrapper leaks. Some of the virtualHasPendingActivity() were properly checking
if the context was stopped but not all of them. To address the issue, we now check
ActiveDOMObject::isContextStopped() in the JS bindings, in addition to
ActiveDOMObject::hasPendingActivity(), so that it is no longer possible to keep a JS wrapper
alive past the point where the script execution context has been stopped. This new approach
is a lot less leak/error prone.
* Modules/indexeddb/IDBDatabase.cpp:
(WebCore::IDBDatabase::virtualHasPendingActivity const):
* Modules/indexeddb/IDBOpenDBRequest.cpp:
(WebCore::IDBOpenDBRequest::requestCompleted):
* Modules/indexeddb/IDBRequest.cpp:
(WebCore::IDBRequest::virtualHasPendingActivity const):
(WebCore::IDBRequest::stop):
(WebCore::IDBRequest::enqueueEvent):
(WebCore::IDBRequest::dispatchEvent):
* Modules/indexeddb/IDBRequest.h:
* Modules/indexeddb/IDBTransaction.cpp:
(WebCore::IDBTransaction::virtualHasPendingActivity const):
(WebCore::IDBTransaction::stop):
(WebCore::IDBTransaction::notifyDidAbort):
(WebCore::IDBTransaction::enqueueEvent):
(WebCore::IDBTransaction::dispatchEvent):
* Modules/indexeddb/IDBTransaction.h:
* Modules/mediastream/MediaDevices.cpp:
(WebCore::MediaDevices::virtualHasPendingActivity const):
* bindings/scripts/CodeGeneratorJS.pm:
(GenerateImplementation):
* bindings/scripts/test/JS/JSTestInterface.cpp:
(WebCore::JSTestInterfaceOwner::isReachableFromOpaqueRoots):
* bindings/scripts/test/JS/JSTestNamedConstructor.cpp:
(WebCore::JSTestNamedConstructorOwner::isReachableFromOpaqueRoots):
* css/FontFace.cpp:
(WebCore::FontFace::virtualHasPendingActivity const):
* dom/ActiveDOMObject.h:
* html/HTMLCanvasElement.cpp:
(WebCore::HTMLCanvasElement::virtualHasPendingActivity const):
* workers/service/ServiceWorkerRegistration.cpp:
(WebCore::ServiceWorkerRegistration::getOrCreate):
(WebCore::ServiceWorkerRegistration::update):
(WebCore::ServiceWorkerRegistration::unregister):
(WebCore::ServiceWorkerRegistration::queueTaskToFireUpdateFoundEvent):
(WebCore::ServiceWorkerRegistration::stop):
(WebCore::ServiceWorkerRegistration::virtualHasPendingActivity const):
* workers/service/ServiceWorkerRegistration.h:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@259419 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index a5cabaa..5941298 100644
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,60 @@
+2020-04-02 Chris Dumez <cdumez@apple.com>
+
+ ActiveDOMObject::hasPendingActivity() should stop preventing wrapper collection after ActiveDOMObject::stop() has been called
+ https://bugs.webkit.org/show_bug.cgi?id=209886
+
+ Reviewed by Ryosuke Niwa.
+
+ ActiveDOMObject::stop() gets called when the script execution context is about to be destroyed.
+ ActiveDOMObject objects should no longer run script after that and there is therefore no point
+ in keeping the JS wrapper alive once stop() has been called. Worse, depending on the
+ implementation of virtualHasPendingActivity(), keeping the wrapper alive past this point may
+ actually cause JS wrapper leaks. Some of the virtualHasPendingActivity() were properly checking
+ if the context was stopped but not all of them. To address the issue, we now check
+ ActiveDOMObject::isContextStopped() in the JS bindings, in addition to
+ ActiveDOMObject::hasPendingActivity(), so that it is no longer possible to keep a JS wrapper
+ alive past the point where the script execution context has been stopped. This new approach
+ is a lot less leak/error prone.
+
+ * Modules/indexeddb/IDBDatabase.cpp:
+ (WebCore::IDBDatabase::virtualHasPendingActivity const):
+ * Modules/indexeddb/IDBOpenDBRequest.cpp:
+ (WebCore::IDBOpenDBRequest::requestCompleted):
+ * Modules/indexeddb/IDBRequest.cpp:
+ (WebCore::IDBRequest::virtualHasPendingActivity const):
+ (WebCore::IDBRequest::stop):
+ (WebCore::IDBRequest::enqueueEvent):
+ (WebCore::IDBRequest::dispatchEvent):
+ * Modules/indexeddb/IDBRequest.h:
+ * Modules/indexeddb/IDBTransaction.cpp:
+ (WebCore::IDBTransaction::virtualHasPendingActivity const):
+ (WebCore::IDBTransaction::stop):
+ (WebCore::IDBTransaction::notifyDidAbort):
+ (WebCore::IDBTransaction::enqueueEvent):
+ (WebCore::IDBTransaction::dispatchEvent):
+ * Modules/indexeddb/IDBTransaction.h:
+ * Modules/mediastream/MediaDevices.cpp:
+ (WebCore::MediaDevices::virtualHasPendingActivity const):
+ * bindings/scripts/CodeGeneratorJS.pm:
+ (GenerateImplementation):
+ * bindings/scripts/test/JS/JSTestInterface.cpp:
+ (WebCore::JSTestInterfaceOwner::isReachableFromOpaqueRoots):
+ * bindings/scripts/test/JS/JSTestNamedConstructor.cpp:
+ (WebCore::JSTestNamedConstructorOwner::isReachableFromOpaqueRoots):
+ * css/FontFace.cpp:
+ (WebCore::FontFace::virtualHasPendingActivity const):
+ * dom/ActiveDOMObject.h:
+ * html/HTMLCanvasElement.cpp:
+ (WebCore::HTMLCanvasElement::virtualHasPendingActivity const):
+ * workers/service/ServiceWorkerRegistration.cpp:
+ (WebCore::ServiceWorkerRegistration::getOrCreate):
+ (WebCore::ServiceWorkerRegistration::update):
+ (WebCore::ServiceWorkerRegistration::unregister):
+ (WebCore::ServiceWorkerRegistration::queueTaskToFireUpdateFoundEvent):
+ (WebCore::ServiceWorkerRegistration::stop):
+ (WebCore::ServiceWorkerRegistration::virtualHasPendingActivity const):
+ * workers/service/ServiceWorkerRegistration.h:
+
2020-04-02 Simon Fraser <simon.fraser@apple.com>
Rename some wheel-event related functions
diff --git a/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp b/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp
index 3a7b229..38adb88 100644
--- a/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp
+++ b/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp
@@ -79,7 +79,7 @@
{
ASSERT(canCurrentThreadAccessThreadLocalData(originThread()) || Thread::mayBeGCThread());
- if (m_closedInServer || isContextStopped())
+ if (m_closedInServer)
return false;
if (!m_activeTransactions.isEmpty() || !m_committingTransactions.isEmpty() || !m_abortingTransactions.isEmpty())
diff --git a/Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp b/Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp
index dbaf43e4..63d11b3 100644
--- a/Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp
+++ b/Source/WebCore/Modules/indexeddb/IDBOpenDBRequest.cpp
@@ -192,7 +192,7 @@
// If an Open request was completed after the page has navigated, leaving this request
// with a stopped script execution context, we need to message back to the server so it
// doesn't hang waiting on a database connection or transaction that will never exist.
- if (m_contextStopped) {
+ if (isContextStopped()) {
switch (data.type()) {
case IDBResultType::OpenDatabaseSuccess:
connectionProxy().abortOpenAndUpgradeNeeded(data.databaseConnectionIdentifier(), IDBResourceIdentifier::emptyValue());
diff --git a/Source/WebCore/Modules/indexeddb/IDBRequest.cpp b/Source/WebCore/Modules/indexeddb/IDBRequest.cpp
index 5052433..8d20117 100644
--- a/Source/WebCore/Modules/indexeddb/IDBRequest.cpp
+++ b/Source/WebCore/Modules/indexeddb/IDBRequest.cpp
@@ -262,21 +262,18 @@
bool IDBRequest::virtualHasPendingActivity() const
{
ASSERT(canCurrentThreadAccessThreadLocalData(originThread()) || Thread::mayBeGCThread());
- return !m_contextStopped && m_hasPendingActivity;
+ return m_hasPendingActivity;
}
void IDBRequest::stop()
{
ASSERT(canCurrentThreadAccessThreadLocalData(originThread()));
- ASSERT(!m_contextStopped);
cancelForStop();
removeAllEventListeners();
clearWrappers();
-
- m_contextStopped = true;
}
void IDBRequest::cancelForStop()
@@ -287,7 +284,7 @@
void IDBRequest::enqueueEvent(Ref<Event>&& event)
{
ASSERT(canCurrentThreadAccessThreadLocalData(originThread()));
- if (m_contextStopped)
+ if (isContextStopped())
return;
queueTaskToDispatchEvent(*this, TaskSource::DatabaseAccess, WTFMove(event));
@@ -299,7 +296,7 @@
ASSERT(canCurrentThreadAccessThreadLocalData(originThread()));
ASSERT(m_hasPendingActivity);
- ASSERT(!m_contextStopped);
+ ASSERT(!isContextStopped());
auto protectedThis = makeRef(*this);
m_dispatchingEvent = true;
diff --git a/Source/WebCore/Modules/indexeddb/IDBRequest.h b/Source/WebCore/Modules/indexeddb/IDBRequest.h
index 2ceb5d3..9a4611b 100644
--- a/Source/WebCore/Modules/indexeddb/IDBRequest.h
+++ b/Source/WebCore/Modules/indexeddb/IDBRequest.h
@@ -144,7 +144,6 @@
bool m_shouldExposeTransactionToDOM { true };
RefPtr<DOMException> m_domError;
IndexedDB::RequestType m_requestType { IndexedDB::RequestType::Other };
- bool m_contextStopped { false };
Event* m_openDatabaseSuccessEvent { nullptr };
private:
diff --git a/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp b/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp
index 20d68e0..06a934a 100644
--- a/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp
+++ b/Source/WebCore/Modules/indexeddb/IDBTransaction.cpp
@@ -318,7 +318,7 @@
bool IDBTransaction::virtualHasPendingActivity() const
{
ASSERT(canCurrentThreadAccessThreadLocalData(m_database->originThread()) || Thread::mayBeGCThread());
- return !m_contextStopped && m_state != IndexedDB::TransactionState::Finished;
+ return m_state != IndexedDB::TransactionState::Finished;
}
void IDBTransaction::stop()
@@ -328,12 +328,12 @@
// IDBDatabase::stop() calls IDBTransaction::stop() for each of its active transactions.
// Since the order of calling ActiveDOMObject::stop() is random, we might already have been stopped.
- if (m_contextStopped)
+ if (m_isStopped)
return;
removeAllEventListeners();
- m_contextStopped = true;
+ m_isStopped = true;
if (isVersionChange())
m_openDBRequest = nullptr;
@@ -518,7 +518,7 @@
m_idbError = error;
fireOnAbort();
- if (isVersionChange() && !m_contextStopped) {
+ if (isVersionChange() && !isContextStopped()) {
ASSERT(m_openDBRequest);
m_openDBRequest->fireErrorAfterVersionChangeCompletion();
}
@@ -573,7 +573,7 @@
ASSERT(m_state != IndexedDB::TransactionState::Finished);
ASSERT(canCurrentThreadAccessThreadLocalData(m_database->originThread()));
- if (!scriptExecutionContext() || m_contextStopped)
+ if (!scriptExecutionContext() || isContextStopped())
return;
queueTaskToDispatchEvent(*this, TaskSource::DatabaseAccess, WTFMove(event));
@@ -585,7 +585,7 @@
ASSERT(canCurrentThreadAccessThreadLocalData(m_database->originThread()));
ASSERT(scriptExecutionContext());
- ASSERT(!m_contextStopped);
+ ASSERT(!isContextStopped());
ASSERT(event.type() == eventNames().completeEvent || event.type() == eventNames().abortEvent);
auto protectedThis = makeRef(*this);
diff --git a/Source/WebCore/Modules/indexeddb/IDBTransaction.h b/Source/WebCore/Modules/indexeddb/IDBTransaction.h
index 55b0d10..cdbee69 100644
--- a/Source/WebCore/Modules/indexeddb/IDBTransaction.h
+++ b/Source/WebCore/Modules/indexeddb/IDBTransaction.h
@@ -262,7 +262,7 @@
HashSet<RefPtr<IDBRequest>> m_openRequests;
RefPtr<IDBRequest> m_currentlyCompletingRequest;
- bool m_contextStopped { false };
+ bool m_isStopped { false };
bool m_didDispatchAbortOrCommit { false };
uint64_t m_lastWriteOperationID { 0 };
diff --git a/Source/WebCore/Modules/mediastream/MediaDevices.cpp b/Source/WebCore/Modules/mediastream/MediaDevices.cpp
index 3b64dec..bbfeabf 100644
--- a/Source/WebCore/Modules/mediastream/MediaDevices.cpp
+++ b/Source/WebCore/Modules/mediastream/MediaDevices.cpp
@@ -250,7 +250,7 @@
bool MediaDevices::virtualHasPendingActivity() const
{
- return !isContextStopped() && hasEventListeners(m_eventNames.devicechangeEvent);
+ return hasEventListeners(m_eventNames.devicechangeEvent);
}
const char* MediaDevices::activeDOMObjectName() const
diff --git a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
index 440d6f1..db9ac1b 100644
--- a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
+++ b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
@@ -4774,7 +4774,8 @@
if ($codeGenerator->InheritsExtendedAttribute($interface, "ActiveDOMObject")) {
push(@implContent, " auto* js${interfaceName} = jsCast<JS${interfaceName}*>(handle.slot()->asCell());\n");
$emittedJSCast = 1;
- push(@implContent, " if (js${interfaceName}->wrapped().hasPendingActivity()) {\n");
+ push(@implContent, " auto& wrapped = js${interfaceName}->wrapped();\n");
+ push(@implContent, " if (!wrapped.isContextStopped() && wrapped.hasPendingActivity()) {\n");
push(@implContent, " if (UNLIKELY(reason))\n");
push(@implContent, " *reason = \"ActiveDOMObject with pending activity\";\n");
push(@implContent, " return true;\n");
diff --git a/Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp b/Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp
index 6c3a063..3fdccec 100644
--- a/Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp
+++ b/Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp
@@ -1210,7 +1210,8 @@
bool JSTestInterfaceOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor, const char** reason)
{
auto* jsTestInterface = jsCast<JSTestInterface*>(handle.slot()->asCell());
- if (jsTestInterface->wrapped().hasPendingActivity()) {
+ auto& wrapped = jsTestInterface->wrapped();
+ if (!wrapped.isContextStopped() && wrapped.hasPendingActivity()) {
if (UNLIKELY(reason))
*reason = "ActiveDOMObject with pending activity";
return true;
diff --git a/Source/WebCore/bindings/scripts/test/JS/JSTestNamedConstructor.cpp b/Source/WebCore/bindings/scripts/test/JS/JSTestNamedConstructor.cpp
index 4199bbf..b10fddc 100644
--- a/Source/WebCore/bindings/scripts/test/JS/JSTestNamedConstructor.cpp
+++ b/Source/WebCore/bindings/scripts/test/JS/JSTestNamedConstructor.cpp
@@ -253,7 +253,8 @@
bool JSTestNamedConstructorOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor, const char** reason)
{
auto* jsTestNamedConstructor = jsCast<JSTestNamedConstructor*>(handle.slot()->asCell());
- if (jsTestNamedConstructor->wrapped().hasPendingActivity()) {
+ auto& wrapped = jsTestNamedConstructor->wrapped();
+ if (!wrapped.isContextStopped() && wrapped.hasPendingActivity()) {
if (UNLIKELY(reason))
*reason = "ActiveDOMObject with pending activity";
return true;
diff --git a/Source/WebCore/css/FontFace.cpp b/Source/WebCore/css/FontFace.cpp
index a525061..c06d1f3 100644
--- a/Source/WebCore/css/FontFace.cpp
+++ b/Source/WebCore/css/FontFace.cpp
@@ -488,7 +488,7 @@
bool FontFace::virtualHasPendingActivity() const
{
- return !isContextStopped() && m_mayLoadedPromiseBeScriptObservable && !m_loadedPromise->isFulfilled();
+ return m_mayLoadedPromiseBeScriptObservable && !m_loadedPromise->isFulfilled();
}
}
diff --git a/Source/WebCore/dom/ActiveDOMObject.h b/Source/WebCore/dom/ActiveDOMObject.h
index 7ad4da6..e6279a7 100644
--- a/Source/WebCore/dom/ActiveDOMObject.h
+++ b/Source/WebCore/dom/ActiveDOMObject.h
@@ -55,6 +55,7 @@
void suspendIfNeeded();
void assertSuspendIfNeededWasCalled() const;
+ // This function is used by JS bindings to determine if the JS wrapper should be kept alive or not.
bool hasPendingActivity() const { return m_pendingActivityInstanceCount || virtualHasPendingActivity(); }
// However, the suspend function will sometimes be called even if canSuspendForDocumentSuspension() returns false.
diff --git a/Source/WebCore/html/HTMLCanvasElement.cpp b/Source/WebCore/html/HTMLCanvasElement.cpp
index ef2b564..d18777e 100644
--- a/Source/WebCore/html/HTMLCanvasElement.cpp
+++ b/Source/WebCore/html/HTMLCanvasElement.cpp
@@ -957,9 +957,6 @@
bool HTMLCanvasElement::virtualHasPendingActivity() const
{
- if (isContextStopped())
- return false;
-
#if ENABLE(WEBGL)
if (is<WebGLRenderingContextBase>(m_context.get())) {
// WebGL rendering context may fire contextlost / contextchange / contextrestored events at any point.
diff --git a/Source/WebCore/workers/service/ServiceWorkerRegistration.cpp b/Source/WebCore/workers/service/ServiceWorkerRegistration.cpp
index d5fa327..bea32a5 100644
--- a/Source/WebCore/workers/service/ServiceWorkerRegistration.cpp
+++ b/Source/WebCore/workers/service/ServiceWorkerRegistration.cpp
@@ -49,7 +49,7 @@
Ref<ServiceWorkerRegistration> ServiceWorkerRegistration::getOrCreate(ScriptExecutionContext& context, Ref<ServiceWorkerContainer>&& container, ServiceWorkerRegistrationData&& data)
{
if (auto* registration = container->registration(data.identifier)) {
- ASSERT(!registration->m_isStopped);
+ ASSERT(!registration->isContextStopped());
return *registration;
}
@@ -137,7 +137,7 @@
void ServiceWorkerRegistration::update(Ref<DeferredPromise>&& promise)
{
- if (m_isStopped) {
+ if (isContextStopped()) {
promise->reject(Exception(InvalidStateError));
return;
}
@@ -154,7 +154,7 @@
void ServiceWorkerRegistration::unregister(Ref<DeferredPromise>&& promise)
{
- if (m_isStopped) {
+ if (isContextStopped()) {
promise->reject(Exception(InvalidStateError));
return;
}
@@ -182,7 +182,7 @@
void ServiceWorkerRegistration::queueTaskToFireUpdateFoundEvent()
{
- if (m_isStopped)
+ if (isContextStopped())
return;
REGISTRATION_RELEASE_LOG_IF_ALLOWED("fireUpdateFoundEvent: Firing updatefound event for registration %llu", identifier().toUInt64());
@@ -207,13 +207,12 @@
void ServiceWorkerRegistration::stop()
{
- m_isStopped = true;
removeAllEventListeners();
}
bool ServiceWorkerRegistration::virtualHasPendingActivity() const
{
- return !m_isStopped && getNewestWorker() && hasEventListeners();
+ return getNewestWorker() && hasEventListeners();
}
} // namespace WebCore
diff --git a/Source/WebCore/workers/service/ServiceWorkerRegistration.h b/Source/WebCore/workers/service/ServiceWorkerRegistration.h
index e00cae9..075fad7 100644
--- a/Source/WebCore/workers/service/ServiceWorkerRegistration.h
+++ b/Source/WebCore/workers/service/ServiceWorkerRegistration.h
@@ -95,8 +95,6 @@
RefPtr<ServiceWorker> m_installingWorker;
RefPtr<ServiceWorker> m_waitingWorker;
RefPtr<ServiceWorker> m_activeWorker;
-
- bool m_isStopped { false };
};
} // namespace WebCore
