Attachment 348217 Details for Bug 189018 – WIP

[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:

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

Actions: View | Formatted Diff | Diff

Attachments on bug 189018: 348217 | 348243 | 348249 | 348937 | 348960 | 348963