⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 236376 in webkit


Ignore:
Timestamp:
Sep 21, 2018, 5:17:17 PM (8 years ago)
Author:
rniwa@webkit.org
Message:

Custom elements in a reaction queue can lose its JS wrapper and become HTMLUnknownElement
​https://bugs.webkit.org/show_bug.cgi?id=184307

Reviewed by Keith Miller.

Source/WebCore:

The bug was caused by the custom elements reaction queue not reporting its content to GC during marking.

When there is no JS reference to the JS wrappers of those custom element, and if those custom elements
are disconnected, GC would happily collect those the wrappers. Unfortunately, the same bug exists for
any asynchronous events and other WebCore code which keeps elements alive for a later use but doesn't
report them to GC (e.g. during visitChildren).

This patch, therefore, introduces a generic mechanism to keep these elements' wrappers alive. Namely,
we introduce GCReachableRef, a new smart pointer type for Node's subclasses, which keeps element as well
as its wrappers alive. GCReachableRef works by adding its Node to a global hash counted set when it's
created and making JSNodeOwner::isReachableFromOpaqueRoots return true when the node is in the set.

Test: fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html

  • Sources.txt:
  • WebCore.xcodeproj/project.pbxproj:
  • bindings/js/JSNodeCustom.cpp:

(WebCore::isReachableFromDOM):

  • dom/CustomElementReactionQueue.cpp:

(WebCore::CustomElementReactionQueue::ElementQueue::invokeAll): Don't swap the vector of elements in
in the queue. Assuming each DOM API has an appropriate CustomElementsReactionStack, we should never
append a new element to this queue while invoking custom element reactions.

  • dom/CustomElementReactionQueue.h:
  • dom/GCReachableRef.cpp: Added.
  • dom/GCReachableRef.h: Added.

(WebCore::GCReachableRefMap::contains): Added.
(WebCore::GCReachableRefMap::add): Added.
(WebCore::GCReachableRefMap::remove): Added.
(WebCore::GCReachableRef::GCReachableRef): Added. We need isNull() check since WTFMove may have been
called on the source GCReachableRef.
(WebCore::GCReachableRef::~GCReachableRef): Ditto.
(WebCore::GCReachableRef::operator-> const): Added.
(WebCore::GCReachableRef::get const): Added.
(WebCore::GCReachableRef::operator T& const): Added.
(WebCore::GCReachableRef::operator! const): Added.
(WebCore::GCReachableRef::isNull const): Added. Returns true if WTFMove had been called on Ref.

LayoutTests:

Added a test for enqueuing a lot of custom elements into the reaction queue via innerHTML setter.
WebKit should retain the JS wrappers of all custom elements.

  • fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper-expected.txt: Added.
  • fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html: Added.
Location:
trunk
Files:
4 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r236370 r236376  
     12018-09-21  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Custom elements in a reaction queue can lose its JS wrapper and become HTMLUnknownElement
     4        https://bugs.webkit.org/show_bug.cgi?id=184307
     5
     6        Reviewed by Keith Miller.
     7
     8        Added a test for enqueuing a lot of custom elements into the reaction queue via innerHTML setter.
     9        WebKit should retain the JS wrappers of all custom elements.
     10
     11        * fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper-expected.txt: Added.
     12        * fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html: Added.
     13
    1142018-09-21  Chris Dumez  <cdumez@apple.com>
    215

  • trunk/Source/WebCore/ChangeLog

    r236365 r236376  
     12018-09-21  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Custom elements in a reaction queue can lose its JS wrapper and become HTMLUnknownElement
     4        https://bugs.webkit.org/show_bug.cgi?id=184307
     5
     6        Reviewed by Keith Miller.
     7
     8        The bug was caused by the custom elements reaction queue not reporting its content to GC during marking.
     9
     10        When there is no JS reference to the JS wrappers of those custom element, and if those custom elements
     11        are disconnected, GC would happily collect those the wrappers. Unfortunately, the same bug exists for
     12        any asynchronous events and other WebCore code which keeps elements alive for a later use but doesn't
     13        report them to GC (e.g. during visitChildren).
     14
     15        This patch, therefore, introduces a generic mechanism to keep these elements' wrappers alive. Namely,
     16        we introduce GCReachableRef, a new smart pointer type for Node's subclasses, which keeps element as well
     17        as its wrappers alive. GCReachableRef works by adding its Node to a global hash counted set when it's
     18        created and making JSNodeOwner::isReachableFromOpaqueRoots return true when the node is in the set.
     19
     20        Test: fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html
     21
     22        * Sources.txt:
     23        * WebCore.xcodeproj/project.pbxproj:
     24        * bindings/js/JSNodeCustom.cpp:
     25        (WebCore::isReachableFromDOM):
     26        * dom/CustomElementReactionQueue.cpp:
     27        (WebCore::CustomElementReactionQueue::ElementQueue::invokeAll): Don't swap the vector of elements in
     28        in the queue. Assuming each DOM API has an appropriate CustomElementsReactionStack, we should never
     29        append a new element to this queue while invoking custom element reactions.
     30        * dom/CustomElementReactionQueue.h:
     31        * dom/GCReachableRef.cpp: Added.
     32        * dom/GCReachableRef.h: Added.
     33        (WebCore::GCReachableRefMap::contains): Added.
     34        (WebCore::GCReachableRefMap::add): Added.
     35        (WebCore::GCReachableRefMap::remove): Added.
     36        (WebCore::GCReachableRef::GCReachableRef): Added. We need isNull() check since WTFMove may have been
     37        called on the source GCReachableRef.
     38        (WebCore::GCReachableRef::~GCReachableRef): Ditto.
     39        (WebCore::GCReachableRef::operator-> const): Added.
     40        (WebCore::GCReachableRef::get const): Added.
     41        (WebCore::GCReachableRef::operator T& const): Added.
     42        (WebCore::GCReachableRef::operator! const): Added.
     43        (WebCore::GCReachableRef::isNull const): Added. Returns true if WTFMove had been called on Ref.
     44
    1452018-09-21  Alex Christensen  <achristensen@webkit.org>
    246

  • trunk/Source/WebCore/Sources.txt

    r236273 r236376  
    816816dom/StaticRange.cpp
    817817dom/StringCallback.cpp
     818dom/GCReachableRef.cpp
    818819dom/StyledElement.cpp
    819820dom/TagCollection.cpp

  • trunk/Source/WebCore/WebCore.xcodeproj/project.pbxproj

    r236339 r236376  
    28432843                9B714E211C91166900AC0E92 /* EventPath.h in Headers */ = {isa = PBXBuildFile; fileRef = 9B714E1F1C91166900AC0E92 /* EventPath.h */; };
    28442844                9BA273F4172206BB0097CE47 /* LogicalSelectionOffsetCaches.h in Headers */ = {isa = PBXBuildFile; fileRef = 9BA273F3172206BB0097CE47 /* LogicalSelectionOffsetCaches.h */; };
     2845                9BAAC45C21520128003D4A98 /* GCReachableRef.h in Headers */ = {isa = PBXBuildFile; fileRef = 9BAAC4562151E39E003D4A98 /* GCReachableRef.h */; settings = {ATTRIBUTES = (Private, ); }; };
    28452846                9BAB6C6C12550631001626D4 /* EditingStyle.h in Headers */ = {isa = PBXBuildFile; fileRef = 9BAB6C6A12550631001626D4 /* EditingStyle.h */; settings = {ATTRIBUTES = (Private, ); }; };
    28462847                9BAF3B2412C1A39800014BF1 /* WritingDirection.h in Headers */ = {isa = PBXBuildFile; fileRef = 9BAF3B2312C1A39800014BF1 /* WritingDirection.h */; settings = {ATTRIBUTES = (Private, ); }; };
    … …  
    1087210873                9BA273F3172206BB0097CE47 /* LogicalSelectionOffsetCaches.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = LogicalSelectionOffsetCaches.h; sourceTree = "<group>"; };
    1087310874                9BA827781F06156500F71E75 /* NavigationDisabler.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = NavigationDisabler.h; sourceTree = "<group>"; };
     10875                9BAAC4562151E39E003D4A98 /* GCReachableRef.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = GCReachableRef.h; sourceTree = "<group>"; };
     10876                9BAAC4582151E77A003D4A98 /* GCReachableRef.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = GCReachableRef.cpp; sourceTree = "<group>"; };
    1087410877                9BAB6C6A12550631001626D4 /* EditingStyle.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = EditingStyle.h; sourceTree = "<group>"; };
    1087510878                9BAB6C6B12550631001626D4 /* EditingStyle.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = EditingStyle.cpp; sourceTree = "<group>"; };
    … …  
    2659126594                                81AC6C35131C57D30009A7E0 /* StringCallback.h */,
    2659226595                                81AC6C34131C57C20009A7E0 /* StringCallback.idl */,
     26596                                9BAAC4582151E77A003D4A98 /* GCReachableRef.cpp */,
     26597                                9BAAC4562151E39E003D4A98 /* GCReachableRef.h */,
    2659326598                                A8C4A7EC09D563270003AC8D /* StyledElement.cpp */,
    2659426599                                A8C4A7EB09D563270003AC8D /* StyledElement.h */,
    … …  
    3042630431                                9B6C41531344949000085B62 /* StringWithDirection.h in Headers */,
    3042730432                                849F77760EFEC6200090849D /* StrokeStyleApplier.h in Headers */,
     30433                                9BAAC45C21520128003D4A98 /* GCReachableRef.h in Headers */,
    3042830434                                414B82051D6DF0E50077EBE3 /* StructuredClone.h in Headers */,
    3042930435                                BC5EB6A30E81DC4F00B25965 /* StyleBackgroundData.h in Headers */,

  • trunk/Source/WebCore/bindings/js/JSNodeCustom.cpp

    r235271 r236376  
    6262#include "ScriptState.h"
    6363#include "ShadowRoot.h"
     64#include "GCReachableRef.h"
    6465#include "StyleSheet.h"
    6566#include "StyledElement.h"
    … …  
    108109            return true;
    109110        }
     111        if (GCReachableRefMap::contains(*node)) {
     112            if (UNLIKELY(reason))
     113                *reason = "Node is scheduled to be used in an async script invocation)";
     114            return true;
     115        }
    110116    }
    111117

  • trunk/Source/WebCore/dom/CustomElementReactionQueue.cpp

    r234636 r236376  
    235235    RELEASE_ASSERT(!m_invoking);
    236236    SetForScope<bool> invoking(m_invoking, true);
    237     Vector<Ref<Element>> elements;
    238     elements.swap(m_elements);
    239     RELEASE_ASSERT(m_elements.isEmpty());
    240     for (auto& element : elements) {
     237    auto originalSize = m_elements.size();
     238    for (auto& element : m_elements) {
    241239        auto* queue = element->reactionQueue();
    242240        ASSERT(queue);
    243241        queue->invokeAll(element.get());
    244242    }
    245     RELEASE_ASSERT(m_elements.isEmpty());
     243    RELEASE_ASSERT(m_elements.size() == originalSize);
     244    m_elements.clear();
    246245}
    247246

  • trunk/Source/WebCore/dom/CustomElementReactionQueue.h

    r234636 r236376  
    2626#pragma once
    2727
     28#include "GCReachableRef.h"
    2829#include <wtf/Forward.h>
    2930#include <wtf/Noncopyable.h>
    … …  
    7273        void invokeAll();
    7374
    74         Vector<Ref<Element>> m_elements;
     75        Vector<GCReachableRef<Element>> m_elements;
    7576        bool m_invoking { false };
    7677    };
Note: See TracChangeset for help on using the changeset viewer.