Attachment 348937 Details for Bug 189018 – Patch

[patch] Patch

bug-189018-20180905103724.patch (text/plain), 7.89 KB, created by youenn fablet on 2018-09-05 10:37:25 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2018-09-05 10:37:25 PDT

Size: 7.89 KB

patch

obsolete

>Subversion Revision: 235670
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index f62aca15c9e40ad3d7ed381b84ae8f750880942b..14c2e5f6db3ae028e3ccbb808cfcbc2a3147c07a 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,27 @@
>+2018-09-05  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!).
>+
>+        Make ActiveDOMOjbect::hasPendingActivity() a protected method.
>+        Introduce ActiveDOMObject::isCollectable that is used by binding code to check whether an ActiveDOMObject is collectable.
>+        It will return true if either the object context is stopped or the object has no pending activity.
>+
>+        Covered by existing tests.
>+
>+        * Modules/encryptedmedia/legacy/WebKitMediaKeySession.h:
>+        * bindings/scripts/CodeGeneratorJS.pm:
>+        (GenerateImplementation):
>+        * bindings/scripts/test/JS/JSTestInterface.cpp:
>+        (WebCore::JSTestInterfaceOwner::isReachableFromOpaqueRoots):
>+        * bindings/scripts/test/JS/JSTestNamedConstructor.cpp:
>+        (WebCore::JSTestNamedConstructorOwner::isReachableFromOpaqueRoots):
>+        * dom/ActiveDOMObject.cpp:
>+        (WebCore::ActiveDOMObject::isCollectable const):
>+        * dom/ActiveDOMObject.h:
>+
> 2018-09-05  Eric Carlson  <eric.carlson@apple.com>
> 
>         [MediaStream] Simplify logic when changing RealtimeMediaSource settings
>diff --git a/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.h b/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.h
>index 9301f09381007d6a7684ec2ba3210d305d066a7c..1fe0c5875cea2605427e5a89aca572099ad7685f 100644
>--- a/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.h
>+++ b/Source/WebCore/Modules/encryptedmedia/legacy/WebKitMediaKeySession.h
>@@ -41,7 +41,7 @@ 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();
>diff --git a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
>index 7f4ac10c9e8af8bdc6dbc865cc7bbfe4a642f3dc..f9b3cf3822428fdff042599db3b979bb83cbbde6 100644
>--- a/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
>+++ b/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm
>@@ -4550,18 +4550,12 @@ 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 isCollectable 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, "    if (!js${interfaceName}->wrapped().isCollectable()) {\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 4c64c19eb85826029f60b925b319e11a703d94f7..f1e44718b07922163985a60dd774b462bbfefab2 100644
>--- a/Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp
>+++ b/Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp
>@@ -999,7 +999,7 @@ void JSTestInterface::heapSnapshot(JSCell* cell, HeapSnapshotBuilder& builder)
> 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()) {
>+    if (!jsTestInterface->wrapped().isCollectable()) {
>         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 9298a1760c0835266c42eb664864bf64482c7b30..5a2a95f08c9c284c2711c1640af500ddf1f4e715 100644
>--- a/Source/WebCore/bindings/scripts/test/JS/JSTestNamedConstructor.cpp
>+++ b/Source/WebCore/bindings/scripts/test/JS/JSTestNamedConstructor.cpp
>@@ -212,7 +212,7 @@ void JSTestNamedConstructor::heapSnapshot(JSCell* cell, HeapSnapshotBuilder& bui
> 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()) {
>+    if (!jsTestNamedConstructor->wrapped().isCollectable()) {
>         if (UNLIKELY(reason))
>             *reason = "ActiveDOMObject with pending activity";
>         return true;
>diff --git a/Source/WebCore/dom/ActiveDOMObject.cpp b/Source/WebCore/dom/ActiveDOMObject.cpp
>index a4392f077da3f603006fe85836ba75b6a065dd60..61c2c817ffc70672eb60c49c300612ddbf8538ab 100644
>--- a/Source/WebCore/dom/ActiveDOMObject.cpp
>+++ b/Source/WebCore/dom/ActiveDOMObject.cpp
>@@ -111,4 +111,9 @@ bool ActiveDOMObject::isContextStopped() const
>     return !scriptExecutionContext() || scriptExecutionContext()->activeDOMObjectsAreStopped();
> }
> 
>+bool ActiveDOMObject::isCollectable() const
>+{
>+    return !hasPendingActivity() || isContextStopped();
>+}
>+
> } // namespace WebCore
>diff --git a/Source/WebCore/dom/ActiveDOMObject.h b/Source/WebCore/dom/ActiveDOMObject.h
>index 360ad21489adbd5fecbb817680df4b9e8f789e74..69dc617b9972b39df2837fd61a5eebba89aee542 100644
>--- a/Source/WebCore/dom/ActiveDOMObject.h
>+++ b/Source/WebCore/dom/ActiveDOMObject.h
>@@ -48,7 +48,7 @@ public:
>     void suspendIfNeeded();
>     void assertSuspendIfNeededWasCalled() const;
> 
>-    virtual bool hasPendingActivity() const;
>+    bool isCollectable() const;
> 
>     // The canSuspendForDocumentSuspension() function is used by the caller if there is a choice between suspending
>     // and stopping. For example, a page won't be suspended and placed in the back/forward
>@@ -116,6 +116,9 @@ protected:
>     explicit ActiveDOMObject(ScriptExecutionContext*);
>     virtual ~ActiveDOMObject();
> 
>+    friend class ScriptExecutionContext;
>+    virtual bool hasPendingActivity() const;
>+
> private:
>     unsigned m_pendingActivityCount;
> #if !ASSERT_DISABLED

Actions: View | Formatted Diff | Diff

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