WebKit Bugzilla
Attachment 348217 Details for
Bug 189018
: ActiveDOMObjects should be GC collectable as soon as their document is being stopped
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
WIP
bug-189018-20180827153629.patch (text/plain), 16.40 KB, created by
youenn fablet
on 2018-08-27 15:36:30 PDT
(
hide
)
Description:
WIP
Filename:
MIME Type:
Creator:
youenn fablet
Created:
2018-08-27 15:36:30 PDT
Size:
16.40 KB
patch
obsolete
>Subversion Revision: 235368 >diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog >index 376135ca411951da75a24ce51d4fcbf536844813..fb42908186bd9dae861cd1406bb080ea00c3edaf 100644 >--- a/Source/WebCore/ChangeLog >+++ b/Source/WebCore/ChangeLog >@@ -1,3 +1,40 @@ >+2018-08-27 Youenn Fablet <youenn@apple.com> >+ >+ ActiveDOMObjects should be GC collectable as soon as their document is being stopped >+ https://bugs.webkit.org/show_bug.cgi?id=189018 >+ >+ Reviewed by NOBODY (OOPS!). >+ >+ WIP. >+ >+ * Modules/encryptedmedia/legacy/WebKitMediaKeySession.cpp: >+ (WebCore::WebKitMediaKeySession::WebKitMediaKeySession): >+ (WebCore::WebKitMediaKeySession::close): >+ * Modules/encryptedmedia/legacy/WebKitMediaKeySession.h: >+ * Modules/fetch/FetchResponse.cpp: >+ (WebCore::FetchResponse::BodyLoader::didSucceed): >+ (WebCore::FetchResponse::BodyLoader::didFail): >+ * Modules/indexeddb/IDBDatabase.cpp: >+ (WebCore::IDBDatabase::hasPendingActivity const): >+ * Modules/indexeddb/IDBIndex.h: >+ * Modules/mediastream/MediaDevices.cpp: >+ (WebCore::MediaDevices::hasPendingActivity const): >+ * Modules/webdatabase/DatabaseContext.h: >+ * bindings/scripts/CodeGeneratorJS.pm: >+ (GenerateImplementation): >+ * css/FontFaceSet.h: >+ * dom/ActiveDOMObject.cpp: >+ (WebCore::ActiveDOMObject::hasPendingActivity const): >+ (WebCore::ActiveDOMObject::isContextStopped const): >+ * dom/ActiveDOMObject.h: >+ * dom/Document.h: >+ * dom/ScriptExecutionContext.h: >+ * html/HTMLMarqueeElement.h: >+ * html/HTMLSourceElement.h: >+ * workers/WorkerGlobalScope.cpp: >+ (WebCore::WorkerGlobalScope::prepareForTermination): >+ * workers/WorkerGlobalScope.h: >+ > 2018-08-27 Youenn Fablet <youenn@apple.com> > > Various IndexDB tests abandon documents >diff --git a/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.cpp b/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.cpp >index b6aa5d85684b56b6005360598f7798cf4f6a3274..e666ce97936745eda0106607978f6e9809ff0b39 100644 >--- a/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.cpp >+++ b/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.cpp >@@ -56,6 +56,8 @@ WebKitMediaKeySession::WebKitMediaKeySession(ScriptExecutionContext& context, We > , m_keyRequestTimer(*this, &WebKitMediaKeySession::keyRequestTimerFired) > , m_addKeyTimer(*this, &WebKitMediaKeySession::addKeyTimerFired) > { >+ if (m_session) >+ m_sessionId = m_session->sessionId(); > } > > WebKitMediaKeySession::~WebKitMediaKeySession() >@@ -68,8 +70,10 @@ WebKitMediaKeySession::~WebKitMediaKeySession() > > void WebKitMediaKeySession::close() > { >- if (m_session) >+ if (m_session) { > m_session->releaseKeys(); >+ m_session = nullptr; >+ } > } > > RefPtr<ArrayBuffer> WebKitMediaKeySession::cachedKeyForKeyId(const String& keyId) const >@@ -77,11 +81,6 @@ RefPtr<ArrayBuffer> WebKitMediaKeySession::cachedKeyForKeyId(const String& keyId > return m_session ? m_session->cachedKeyForKeyID(keyId) : nullptr; > } > >-const String& WebKitMediaKeySession::sessionId() const >-{ >- return m_session->sessionId(); >-} >- > void WebKitMediaKeySession::generateKeyRequest(const String& mimeType, Ref<Uint8Array>&& initData) > { > m_pendingKeyRequests.append({ mimeType, WTFMove(initData) }); >diff --git a/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.h b/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.h >index 1d0dc7c4aab0a8b88c957928ec6de47a653bd889..1fe0c5875cea2605427e5a89aca572099ad7685f 100644 >--- a/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.h >+++ b/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.h >@@ -41,14 +41,14 @@ namespace WebCore { > class WebKitMediaKeyError; > class WebKitMediaKeys; > >-class WebKitMediaKeySession final : public RefCounted<WebKitMediaKeySession>, public EventTargetWithInlineData, private ActiveDOMObject, private LegacyCDMSessionClient { >+class WebKitMediaKeySession final : public RefCounted<WebKitMediaKeySession>, public EventTargetWithInlineData, public ActiveDOMObject, private LegacyCDMSessionClient { > public: > static Ref<WebKitMediaKeySession> create(ScriptExecutionContext&, WebKitMediaKeys&, const String& keySystem); > ~WebKitMediaKeySession(); > > WebKitMediaKeyError* error() { return m_error.get(); } > const String& keySystem() const { return m_keySystem; } >- const String& sessionId() const; >+ const String& sessionId() const { return m_sessionId; } > ExceptionOr<void> update(Ref<Uint8Array>&& key); > void close(); > >diff --git a/Source/WebCore/Modules/fetch/FetchResponse.cpp b/Source/WebCore/Modules/fetch/FetchResponse.cpp >index ff8c3a24a041f20055ab3de81936c8eed9230e41..7af865426ca58f6a0b187eee4ee42603c4172849 100644 >--- a/Source/WebCore/Modules/fetch/FetchResponse.cpp >+++ b/Source/WebCore/Modules/fetch/FetchResponse.cpp >@@ -221,7 +221,7 @@ const ResourceResponse& FetchResponse::filteredResponse() const > > void FetchResponse::BodyLoader::didSucceed() > { >- ASSERT(m_response.hasPendingActivity()); >+ ASSERT(m_response.hasPendingActivity() || m_response.isContextStopped()); > m_response.m_body->loadingSucceeded(); > > #if ENABLE(STREAMS_API) >@@ -243,7 +243,7 @@ void FetchResponse::BodyLoader::didSucceed() > > void FetchResponse::BodyLoader::didFail(const ResourceError& error) > { >- ASSERT(m_response.hasPendingActivity()); >+ ASSERT(m_response.hasPendingActivity() || m_response.isContextStopped()); > > m_response.m_loadingError = error; > >diff --git a/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp b/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp >index 3391855baee55d059784c920e9c2428b271b9245..258d89a5adabca588a19a48d978f6ee221f8de7e 100644 >--- a/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp >+++ b/Source/WebCore/Modules/indexeddb/IDBDatabase.cpp >@@ -76,7 +76,7 @@ bool IDBDatabase::hasPendingActivity() const > { > ASSERT(&originThread() == &Thread::current() || mayBeGCThread()); > >- if (m_closedInServer) >+ if (m_closedInServer || isContextStopped()) > return false; > > if (!m_activeTransactions.isEmpty() || !m_committingTransactions.isEmpty() || !m_abortingTransactions.isEmpty()) >diff --git a/Source/WebCore/Modules/indexeddb/IDBIndex.h b/Source/WebCore/Modules/indexeddb/IDBIndex.h >index d6488d591cac3edf0d123dd8404090bf1507f325..8ab5c1c11938a0017cdc19eb38cd47526faa6dfd 100644 >--- a/Source/WebCore/Modules/indexeddb/IDBIndex.h >+++ b/Source/WebCore/Modules/indexeddb/IDBIndex.h >@@ -41,7 +41,7 @@ class IDBKeyRange; > > struct IDBKeyRangeData; > >-class IDBIndex final : private ActiveDOMObject { >+class IDBIndex final : public ActiveDOMObject { > WTF_MAKE_NONCOPYABLE(IDBIndex); > WTF_MAKE_FAST_ALLOCATED; > public: >diff --git a/Source/WebCore/Modules/mediastream/MediaDevices.cpp b/Source/WebCore/Modules/mediastream/MediaDevices.cpp >index 5c0f538e62dbf155b21ce9615109934dc04e006d..d9b0bb775ae877c9b2415aff0850953ef9d3d146 100644 >--- a/Source/WebCore/Modules/mediastream/MediaDevices.cpp >+++ b/Source/WebCore/Modules/mediastream/MediaDevices.cpp >@@ -162,7 +162,7 @@ void MediaDevices::scheduledEventTimerFired() > > bool MediaDevices::hasPendingActivity() const > { >- return scriptExecutionContext() && hasEventListeners(m_eventNames.devicechangeEvent); >+ return !isContextStopped() && hasEventListeners(m_eventNames.devicechangeEvent); > } > > const char* MediaDevices::activeDOMObjectName() const >diff --git a/Source/WebCore/Modules/webdatabase/DatabaseContext.h b/Source/WebCore/Modules/webdatabase/DatabaseContext.h >index d18644888ffaee089a8de8d123539e472910923d..724fa30a4c0a4f909f7f182d89f72a87df642388 100644 >--- a/Source/WebCore/Modules/webdatabase/DatabaseContext.h >+++ b/Source/WebCore/Modules/webdatabase/DatabaseContext.h >@@ -44,7 +44,7 @@ class DatabaseThread; > class SecurityOrigin; > struct SecurityOriginData; > >-class DatabaseContext final : public ThreadSafeRefCounted<DatabaseContext>, private ActiveDOMObject { >+class DatabaseContext final : public ThreadSafeRefCounted<DatabaseContext>, public ActiveDOMObject { > public: > virtual ~DatabaseContext(); > >diff --git a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm >index 7f4ac10c9e8af8bdc6dbc865cc7bbfe4a642f3dc..05c4bbc92106e28878c03b83dab1c578fd8b5db8 100644 >--- a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm >+++ b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm >@@ -4550,18 +4550,13 @@ sub GenerateImplementation > if (ShouldGenerateWrapperOwnerCode($hasParent, $interface) && !GetCustomIsReachable($interface)) { > push(@implContent, "bool JS${interfaceName}Owner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor, const char** reason)\n"); > push(@implContent, "{\n"); >- # All ActiveDOMObjects implement hasPendingActivity(), but not all of them >- # increment their C++ reference counts when hasPendingActivity() becomes >- # true. As a result, ActiveDOMObjects can be prematurely destroyed before >- # their pending activities complete. To wallpaper over this bug, JavaScript >- # wrappers unconditionally keep ActiveDOMObjects with pending activity alive. >- # FIXME: Fix this lifetime issue in the DOM, and move this hasPendingActivity >- # check just above the (GetGenerateIsReachable($interface) eq "Impl") check below. >+ # FIXME: Move this hasPendingActviity check just above the (GetGenerateIsReachable($interface) eq "Impl") check below. > my $emittedJSCast = 0; > 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, " ASSERT(!js${interfaceName}->wrapped().isContextStopped());\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/css/FontFaceSet.h b/Source/WebCore/css/FontFaceSet.h >index 1c89172a8e71dd838d43ba6dd843838864ed6ec7..904883242570ddab2c5d33c26ee3c180f977e4b9 100644 >--- a/Source/WebCore/css/FontFaceSet.h >+++ b/Source/WebCore/css/FontFaceSet.h >@@ -35,7 +35,7 @@ namespace WebCore { > > class DOMException; > >-class FontFaceSet final : public RefCounted<FontFaceSet>, private CSSFontFaceSetClient, public EventTargetWithInlineData, private ActiveDOMObject { >+class FontFaceSet final : public RefCounted<FontFaceSet>, private CSSFontFaceSetClient, public EventTargetWithInlineData, public ActiveDOMObject { > public: > static Ref<FontFaceSet> create(Document&, const Vector<RefPtr<FontFace>>& initialFaces); > static Ref<FontFaceSet> create(Document&, CSSFontFaceSet& backing); >diff --git a/Source/WebCore/dom/ActiveDOMObject.cpp b/Source/WebCore/dom/ActiveDOMObject.cpp >index b81a884c40ffa26c9d55726b040f900c38e5e885..67141f70e2b2ab78a63047ce13bf89e98fff3be4 100644 >--- a/Source/WebCore/dom/ActiveDOMObject.cpp >+++ b/Source/WebCore/dom/ActiveDOMObject.cpp >@@ -86,7 +86,11 @@ void ActiveDOMObject::assertSuspendIfNeededWasCalled() const > > bool ActiveDOMObject::hasPendingActivity() const > { >- return m_pendingActivityCount; >+ if (!m_pendingActivityCount) >+ return false; >+ >+ auto* context = scriptExecutionContext(); >+ return context && !context->isStopped(); > } > > bool ActiveDOMObject::canSuspendForDocumentSuspension() const >@@ -106,4 +110,9 @@ void ActiveDOMObject::stop() > { > } > >+bool ActiveDOMObject::isContextStopped() const >+{ >+ return !scriptExecutionContext() || scriptExecutionContext()->isStopped(); >+} >+ > } // namespace WebCore >diff --git a/Source/WebCore/dom/ActiveDOMObject.h b/Source/WebCore/dom/ActiveDOMObject.h >index ce1ec5726f6d9f67aa152d67c32a6c7f58864f9c..360ad21489adbd5fecbb817680df4b9e8f789e74 100644 >--- a/Source/WebCore/dom/ActiveDOMObject.h >+++ b/Source/WebCore/dom/ActiveDOMObject.h >@@ -110,6 +110,8 @@ public: > return adoptRef(*new PendingActivity<T>(thisObject)); > } > >+ bool isContextStopped() const; >+ > protected: > explicit ActiveDOMObject(ScriptExecutionContext*); > virtual ~ActiveDOMObject(); >diff --git a/Source/WebCore/dom/Document.h b/Source/WebCore/dom/Document.h >index 0a1d903417d4ca9dc0e30bbffac41d188f31bdbc..d60fe36b0403fbf305f99b1b71772238d15449be 100644 >--- a/Source/WebCore/dom/Document.h >+++ b/Source/WebCore/dom/Document.h >@@ -1597,6 +1597,8 @@ private: > > bool shouldEnforceHTTP09Sandbox() const; > >+ bool isStopped() final { return m_hasPreparedForDestruction; } >+ > void platformSuspendOrStopActiveDOMObjects(); > > bool domainIsRegisterable(const String&) const; >diff --git a/Source/WebCore/dom/ScriptExecutionContext.h b/Source/WebCore/dom/ScriptExecutionContext.h >index 3bdfcc80dbe6e6adad5877d1fee301127376d563..89a56d6fe607dcdae0bbd67763d6b1641c64b149 100644 >--- a/Source/WebCore/dom/ScriptExecutionContext.h >+++ b/Source/WebCore/dom/ScriptExecutionContext.h >@@ -130,6 +130,8 @@ public: > // Active objects are not garbage collected even if inaccessible, e.g. because their activity may result in callbacks being invoked. > WEBCORE_EXPORT bool canSuspendActiveDOMObjectsForDocumentSuspension(Vector<ActiveDOMObject*>* unsuspendableObjects = nullptr); > >+ virtual bool isStopped() = 0; >+ > // Active objects can be asked to suspend even if canSuspendActiveDOMObjectsForDocumentSuspension() returns 'false' - > // step-by-step JS debugging is one example. > virtual void suspendActiveDOMObjects(ReasonForSuspension); >diff --git a/Source/WebCore/html/HTMLMarqueeElement.h b/Source/WebCore/html/HTMLMarqueeElement.h >index 68490fd4526295830a29fb6a9b640e6682fd3c5d..4b85bc17013d147ac81d6b4639e6336fe6a4ae87 100644 >--- a/Source/WebCore/html/HTMLMarqueeElement.h >+++ b/Source/WebCore/html/HTMLMarqueeElement.h >@@ -29,7 +29,7 @@ namespace WebCore { > > class RenderMarquee; > >-class HTMLMarqueeElement final : public HTMLElement, private ActiveDOMObject { >+class HTMLMarqueeElement final : public HTMLElement, public ActiveDOMObject { > WTF_MAKE_ISO_ALLOCATED(HTMLMarqueeElement); > public: > static Ref<HTMLMarqueeElement> create(const QualifiedName&, Document&); >diff --git a/Source/WebCore/html/HTMLSourceElement.h b/Source/WebCore/html/HTMLSourceElement.h >index 3204f54a1029f24a33f93a60e6d6c334b3bff578..6ed8235d9cd2fe7a190b63aff4937cfa296b2fcf 100644 >--- a/Source/WebCore/html/HTMLSourceElement.h >+++ b/Source/WebCore/html/HTMLSourceElement.h >@@ -32,7 +32,7 @@ namespace WebCore { > > class MediaQuerySet; > >-class HTMLSourceElement final : public HTMLElement, private ActiveDOMObject { >+class HTMLSourceElement final : public HTMLElement, public ActiveDOMObject { > WTF_MAKE_ISO_ALLOCATED(HTMLSourceElement); > public: > static Ref<HTMLSourceElement> create(Document&); >diff --git a/Source/WebCore/workers/WorkerGlobalScope.cpp b/Source/WebCore/workers/WorkerGlobalScope.cpp >index 2a2631386beca42cb874084794cbab0f2ea688ee..a6945edea93d82387baf0f7278c3f4c9a7f4f3b5 100644 >--- a/Source/WebCore/workers/WorkerGlobalScope.cpp >+++ b/Source/WebCore/workers/WorkerGlobalScope.cpp >@@ -107,6 +107,9 @@ String WorkerGlobalScope::origin() const > > void WorkerGlobalScope::prepareForTermination() > { >+ ASSERT(!m_isPreparingForTermination); >+ m_isPreparingForTermination = true; >+ > #if ENABLE(INDEXED_DATABASE) > stopIndexedDatabase(); > #endif >diff --git a/Source/WebCore/workers/WorkerGlobalScope.h b/Source/WebCore/workers/WorkerGlobalScope.h >index 5b9a0be369c299c0fb7541078fc7656d6de79ec1..ecae0f400d0747702cbdfc63802378890bf77837 100644 >--- a/Source/WebCore/workers/WorkerGlobalScope.h >+++ b/Source/WebCore/workers/WorkerGlobalScope.h >@@ -156,6 +156,7 @@ private: > WorkerEventQueue& eventQueue() const final; > String resourceRequestIdentifier() const final { return m_identifier; } > SocketProvider* socketProvider() final; >+ bool isStopped() final { return m_isPreparingForTermination; } > > bool shouldBypassMainWorldContentSecurityPolicy() const final { return m_shouldBypassMainWorldContentSecurityPolicy; } > bool isJSExecutionForbidden() const final; >@@ -190,6 +191,7 @@ private: > std::unique_ptr<MicrotaskQueue> m_microtaskQueue; > > bool m_closing { false }; >+ bool m_isPreparingForTermination { false }; > bool m_isOnline; > bool m_shouldBypassMainWorldContentSecurityPolicy; >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 189018
:
348217
|
348243
|
348249
|
348937
|
348960
|
348963