Changeset 237880 in webkit


Ignore:
Timestamp:
Nov 6, 2018 12:52:51 PM (5 years ago)
Author:
ajuma@chromium.org
Message:

IntersectionObserver doesn't keep target's JS wrapper alive
​https://bugs.webkit.org/show_bug.cgi?id=190235

Reviewed by Ryosuke Niwa.

Source/WebCore:

Retain JS wrappers of targets in queued entries using a vector of GCReachableRef owned by
IntersectionObserver, which gets cleared after the entries have been delivered.

Make IntersectionObserver::takeRecords return a struct which has both the vector of GCReachableRefs
for targets and the vector of intersection observer entries, so that the GCReachableRefs survive
until the creation of JS wrappers for the entries.

Modify IntersectionObserver::hasPendingActivity to keep the observer alive while it has
entries to deliver.

Tests: intersection-observer/intersection-observer-entry-keeps-js-wrapper-of-target-alive.html

intersection-observer/intersection-observer-keeps-js-wrapper-of-target-alive.html
intersection-observer/target-deleted.html

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

(WebCore::JSIntersectionObserverEntry::visitAdditionalChildren): Keep the target element's wrapper alive while the
IntersectionObserverEntry's wrapper is alive.

  • page/IntersectionObserver.cpp:

(WebCore::IntersectionObserver::takeRecords): Change return type to include GCReachableRefs for each record's target, so that
each target can be kept until a JS wrapper is constructed for its IntersectionObserverEntry.
(WebCore::IntersectionObserver::appendQueuedEntry):
(WebCore::IntersectionObserver::notify): Erase GCReachableRefs for targets after delivering the corresponding records.
(WebCore::IntersectionObserver::hasPendingActivity const): Keep the IntersectionObserver alive until queued entries are delivered.
(WebCore::IntersectionObserver::stop):

  • page/IntersectionObserver.h:
  • page/IntersectionObserver.idl:
  • page/IntersectionObserverEntry.h:

(WebCore::IntersectionObserverEntry::target const): Make this return a raw pointer instead of a RefPtr so that it
can be called in JSIntersectionObserverEntry::visitAdditionalChildren, which runs on the GC thread (it's illegal to ref a Node
on a non-main thread).

  • page/IntersectionObserverEntry.idl:

LayoutTests:

Add test coverage.

Update test that depends on a target getting GC'd to call takeRecords() since
targets with pending entries are no logner GC'd.

  • intersection-observer/intersection-observer-entry-keeps-js-wrapper-of-target-alive-expected.txt: Added.
  • intersection-observer/intersection-observer-entry-keeps-js-wrapper-of-target-alive.html: Added.
  • intersection-observer/intersection-observer-keeps-js-wrapper-of-target-alive-expected.txt: Added.
  • intersection-observer/intersection-observer-keeps-js-wrapper-of-target-alive.html: Added.
  • intersection-observer/root-element-deleted.html:
  • intersection-observer/target-deleted-expected.txt: Added.
  • intersection-observer/target-deleted.html: Added.
Location:
trunk
Files:
6 added
10 edited
1 copied

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r237879 r237880  
     12018-11-06  Ali Juma  <ajuma@chromium.org>
     2
     3        IntersectionObserver doesn't keep target's JS wrapper alive
     4        https://bugs.webkit.org/show_bug.cgi?id=190235
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        Add test coverage.
     9
     10        Update test that depends on a target getting GC'd to call takeRecords() since
     11        targets with pending entries are no logner GC'd.
     12
     13        * intersection-observer/intersection-observer-entry-keeps-js-wrapper-of-target-alive-expected.txt: Added.
     14        * intersection-observer/intersection-observer-entry-keeps-js-wrapper-of-target-alive.html: Added.
     15        * intersection-observer/intersection-observer-keeps-js-wrapper-of-target-alive-expected.txt: Added.
     16        * intersection-observer/intersection-observer-keeps-js-wrapper-of-target-alive.html: Added.
     17        * intersection-observer/root-element-deleted.html:
     18        * intersection-observer/target-deleted-expected.txt: Added.
     19        * intersection-observer/target-deleted.html: Added.
     20
    1212018-11-06  Eric Carlson  <eric.carlson@apple.com>
    222

  • trunk/LayoutTests/intersection-observer/root-element-deleted.html

    r235014 r237880  
    2121        target = null;
    2222        requestAnimationFrame(t.step_func_done(function() {
     23            observer.takeRecords();
    2324            GCController.collect();
    2425            assert_equals(observer.root, null, 'Observer has null root after root element is destroyed');

  • trunk/Source/WebCore/ChangeLog

    r237878 r237880  
     12018-11-06  Ali Juma  <ajuma@chromium.org>
     2
     3        IntersectionObserver doesn't keep target's JS wrapper alive
     4        https://bugs.webkit.org/show_bug.cgi?id=190235
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        Retain JS wrappers of targets in queued entries using a vector of GCReachableRef owned by
     9        IntersectionObserver, which gets cleared after the entries have been delivered.
     10
     11        Make IntersectionObserver::takeRecords return a struct which has both the vector of GCReachableRefs
     12        for targets and the vector of intersection observer entries, so that the GCReachableRefs survive
     13        until the creation of JS wrappers for the entries.
     14
     15        Modify IntersectionObserver::hasPendingActivity to keep the observer alive while it has
     16        entries to deliver.
     17
     18        Tests: intersection-observer/intersection-observer-entry-keeps-js-wrapper-of-target-alive.html
     19               intersection-observer/intersection-observer-keeps-js-wrapper-of-target-alive.html
     20               intersection-observer/target-deleted.html
     21
     22        * Sources.txt:
     23        * WebCore.xcodeproj/project.pbxproj:
     24        * bindings/js/JSIntersectionObserverEntryCustom.cpp:
     25        (WebCore::JSIntersectionObserverEntry::visitAdditionalChildren): Keep the target element's wrapper alive while the
     26        IntersectionObserverEntry's wrapper is alive.
     27        * page/IntersectionObserver.cpp:
     28        (WebCore::IntersectionObserver::takeRecords): Change return type to include GCReachableRefs for each record's target, so that
     29        each target can be kept until a JS wrapper is constructed for its IntersectionObserverEntry.
     30        (WebCore::IntersectionObserver::appendQueuedEntry):
     31        (WebCore::IntersectionObserver::notify): Erase GCReachableRefs for targets after delivering the corresponding records.
     32        (WebCore::IntersectionObserver::hasPendingActivity const): Keep the IntersectionObserver alive until queued entries are delivered.
     33        (WebCore::IntersectionObserver::stop):
     34        * page/IntersectionObserver.h:
     35        * page/IntersectionObserver.idl:
     36        * page/IntersectionObserverEntry.h:
     37        (WebCore::IntersectionObserverEntry::target const): Make this return a raw pointer instead of a RefPtr so that it
     38        can be called in JSIntersectionObserverEntry::visitAdditionalChildren, which runs on the GC thread (it's illegal to ref a Node
     39        on a non-main thread).
     40        * page/IntersectionObserverEntry.idl:
     41
    1422018-11-06  Timothy Hatcher  <timothy@apple.com>
    243

  • trunk/Source/WebCore/Sources.txt

    r237855 r237880  
    431431bindings/js/JSIDBTransactionCustom.cpp
    432432bindings/js/JSImageDataCustom.cpp
     433bindings/js/JSIntersectionObserverEntryCustom.cpp
    433434bindings/js/JSLazyEventListener.cpp
    434435bindings/js/JSLocationCustom.cpp

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

    r237855 r237880  
    94039403                77AAD6831ECFB66200BFA2D1 /* CredentialCreationOptions.idl */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = CredentialCreationOptions.idl; sourceTree = "<group>"; };
    94049404                77AAD6851ECFBD3900BFA2D1 /* CredentialCreationOptions.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CredentialCreationOptions.h; sourceTree = "<group>"; };
     9405                77C13F042165658A002D9C5F /* JSIntersectionObserverEntryCustom.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = JSIntersectionObserverEntryCustom.cpp; sourceTree = "<group>"; };
    94059406                77CAAAEF1F2FC35000CB5C8D /* VisualViewport.idl */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = VisualViewport.idl; sourceTree = "<group>"; };
    94069407                77D50FFA1ED4EC7800DA4C87 /* CredentialRequestOptions.idl */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text; path = CredentialRequestOptions.idl; sourceTree = "<group>"; };
    … …  
    1613016131                        isa = PBXGroup;
    1613116132                        children = (
    16132                                 71495DD8202B06DC00ADFD34 /* JSEffectTiming.cpp */,
    16133                                 71495DDA202B06DE00ADFD34 /* JSEffectTiming.h */,
    1613416133                                714C7C691FDADAD500F2BEE1 /* JSAnimationPlaybackEvent.cpp */,
    1613516134                                714C7C6A1FDADAD500F2BEE1 /* JSAnimationPlaybackEvent.h */,
    … …  
    1615216151                                71729F7D20F3BABA00801CE6 /* JSDocumentTimelineOptions.cpp */,
    1615316152                                71729F7C20F3BAB900801CE6 /* JSDocumentTimelineOptions.h */,
     16153                                71495DD8202B06DC00ADFD34 /* JSEffectTiming.cpp */,
     16154                                71495DDA202B06DE00ADFD34 /* JSEffectTiming.h */,
    1615416155                                712BE4841FE867C2002031CC /* JSFillMode.cpp */,
    1615516156                                712BE4851FE86818002031CC /* JSFillMode.h */,
    … …  
    2001420015                                71EFCED7202B388D00D7C411 /* AnimationEffect.h */,
    2001520016                                71EFCED6202B388C00D7C411 /* AnimationEffect.idl */,
    20016                                 712BE47C1FE86448002031CC /* EffectTiming.h */,
    20017                                 712BE47A1FE86447002031CC /* EffectTiming.idl */,
    2001820017                                714C7C601FDAD27900F2BEE1 /* AnimationPlaybackEvent.cpp */,
    2001920018                                714C7C651FDAD27B00F2BEE1 /* AnimationPlaybackEvent.h */,
    … …  
    2004620045                                71729F7A20F3BA3A00801CE6 /* DocumentTimelineOptions.h */,
    2004720046                                71729F7920F3BA3900801CE6 /* DocumentTimelineOptions.idl */,
     20047                                712BE47C1FE86448002031CC /* EffectTiming.h */,
     20048                                712BE47A1FE86447002031CC /* EffectTiming.idl */,
    2004820049                                712BE4811FE865D4002031CC /* FillMode.h */,
    2004920050                                712BE4821FE865D5002031CC /* FillMode.idl */,
    … …  
    2026020261                                51E269321DD3BC43006B6A58 /* JSIDBTransactionCustom.cpp */,
    2026120262                                A7D0318D0E93540300E24ACD /* JSImageDataCustom.cpp */,
     20263                                77C13F042165658A002D9C5F /* JSIntersectionObserverEntryCustom.cpp */,
    2026220264                                AD726FE716D9F204003A4E6D /* JSMediaListCustom.h */,
    2026320265                                415CDAF61E6CE0D3004F11EE /* JSMediaStreamTrackCustom.cpp */,
    … …  
    2763027632                                316FE1120E6E1DA700BF6088 /* AnimationBase.h in Headers */,
    2763127633                                71EFCEDC202B38A900D7C411 /* AnimationEffect.h in Headers */,
    27632                                 712BE47D1FE86458002031CC /* EffectTiming.h in Headers */,
    2763327634                                319848011A1D817B00A13318 /* AnimationEvent.h in Headers */,
    2763427635                                49E912AD0EFAC906009D0CAF /* AnimationList.h in Headers */,
    … …  
    2794627947                                93309DDD099E64920056E581 /* CompositeEditCommand.h in Headers */,
    2794727948                                71247E371FEA5F86008C08CE /* CompositeOperation.h in Headers */,
     27949                                7111243E216FA71100EB7B67 /* CompositeOperationOrAuto.h in Headers */,
    2794827950                                79F2F5A21091939A000D87CB /* CompositionEvent.h in Headers */,
    2794927951                                2DD5A7271EBEE47D009BA597 /* CompositionUnderline.h in Headers */,
    … …  
    2795427956                                A818721C0977D3C0005826D9 /* ContainerNode.h in Headers */,
    2795527957                                E1A1470811102B1500EEC0F3 /* ContainerNodeAlgorithms.h in Headers */,
    27956                                 71112441216FA7CD00EB7B67 /* JSCompositeOperationOrAuto.h in Headers */,
    2795727958                                BC5EB9810E82072500B25965 /* ContentData.h in Headers */,
    2795827959                                51B45D211AB8D1E200117CD2 /* ContentExtension.h in Headers */,
    … …  
    2839228393                                4BAE95B10B2FA9CE00AED8A0 /* EditorDeleteAction.h in Headers */,
    2839328394                                93FDAFCA0B11307400E2746F /* EditorInsertAction.h in Headers */,
     28395                                712BE47D1FE86458002031CC /* EffectTiming.h in Headers */,
    2839428396                                A8C4A80709D563270003AC8D /* Element.h in Headers */,
    2839528397                                E4AE7C1A17D232350009FB31 /* ElementAncestorIterator.h in Headers */,
    … …  
    2900329005                                576814451E70CB1F00E77754 /* JSAesKeyParams.h in Headers */,
    2900429006                                FDA15ECA12B03F50003A583A /* JSAnalyserNode.h in Headers */,
    29005                                 71495DDB202B06F100ADFD34 /* JSEffectTiming.h in Headers */,
    2900629007                                3198480C1A1E6CE800A13318 /* JSAnimationEvent.h in Headers */,
    2900729008                                714C7C6C1FDADAF300F2BEE1 /* JSAnimationPlaybackEvent.h in Headers */,
    … …  
    2907929080                                93F9B6E10BA0FB7200854064 /* JSComment.h in Headers */,
    2908029081                                71247E3B1FEA5F90008C08CE /* JSCompositeOperation.h in Headers */,
     29082                                71112441216FA7CD00EB7B67 /* JSCompositeOperationOrAuto.h in Headers */,
    2908129083                                79AC9219109945C80021266E /* JSCompositionEvent.h in Headers */,
    2908229084                                7116E2CF1FED765B00C06FDE /* JSComputedEffectTiming.h in Headers */,
    … …  
    2916729169                                E323CFFA1E5AF6AF00F0B4A0 /* JSDOMConvertPromise.h in Headers */,
    2916829170                                7C8E34BF1E4A33B00054CE23 /* JSDOMConvertRecord.h in Headers */,
    29169                                 7111243E216FA71100EB7B67 /* CompositeOperationOrAuto.h in Headers */,
    2917029171                                7C1F5D591F22FF7300A8874F /* JSDOMConvertScheduledAction.h in Headers */,
    2917129172                                7C8E34C01E4A33B00054CE23 /* JSDOMConvertSequences.h in Headers */,
    … …  
    2922429225                                57EEAA551EA001B100701124 /* JSEcdsaParams.h in Headers */,
    2922529226                                5750A9821E6A150800705C4A /* JSEcKeyParams.h in Headers */,
     29227                                71495DDB202B06F100ADFD34 /* JSEffectTiming.h in Headers */,
    2922629228                                65DF31FA09D1CC60000BE325 /* JSElement.h in Headers */,
    2922729229                                ADEC78F818EE5308001315C2 /* JSElementCustom.h in Headers */,
    … …  
    2945029452                                3140C5271FDF558200D2A873 /* JSOffscreenCanvasRenderingContext2D.h in Headers */,
    2945129453                                57E233651DC7DB1F00F28D01 /* JsonWebKey.h in Headers */,
     29454                                71207343216DFB4100C78329 /* JSOptionalEffectTiming.h in Headers */,
    2945229455                                FDEA6243152102E200479DF0 /* JSOscillatorNode.h in Headers */,
    2945329456                                0704A40C1D6DFC690086DCDB /* JSOverconstrainedError.h in Headers */,
    … …  
    2958829591                                24D9129613CA956100D21915 /* JSSVGAltGlyphItemElement.h in Headers */,
    2958929592                                B222F6990AB771950022EFAD /* JSSVGAngle.h in Headers */,
    29590                                 71207340216DFB0000C78329 /* OptionalEffectTiming.h in Headers */,
    2959129593                                B2FA3D370AB75A6F000E5AC4 /* JSSVGAnimateColorElement.h in Headers */,
    2959229594                                B2FA3D390AB75A6F000E5AC4 /* JSSVGAnimatedAngle.h in Headers */,
    … …  
    3018130183                                B2D3DA650D006CD600EF6F27 /* OpenTypeMathData.h in Headers */,
    3018230184                                B2D3EA650D006CD600EF6F28 /* OpenTypeTypes.h in Headers */,
     30185                                71207340216DFB0000C78329 /* OptionalEffectTiming.h in Headers */,
    3018330186                                CDE7FC45181904B1002BBB77 /* OrderIterator.h in Headers */,
    3018430187                                4184F5161EAF05A800F18BF0 /* OrientationNotifier.h in Headers */,
    … …  
    3060030603                                514C767A0CE923A1007EF3CD /* ResourceHandleClient.h in Headers */,
    3060130604                                514C767B0CE923A1007EF3CD /* ResourceHandleInternal.h in Headers */,
    30602                                 71207343216DFB4100C78329 /* JSOptionalEffectTiming.h in Headers */,
    3060330605                                656D373F0ADBA5DE00A4554D /* ResourceLoader.h in Headers */,
    3060430606                                D0A3A7311405A39800FB8ED3 /* ResourceLoaderOptions.h in Headers */,

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

    r237879 r237880  
    11/*
    2  * Copyright (C) 2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2018 Google LLC. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    2424 */
    2525
    26 // https://wicg.github.io/IntersectionObserver/
     26#include "config.h"
     27#include "JSIntersectionObserverEntry.h"
    2728
    28 [
    29     ActiveDOMObject,
    30     Conditional=INTERSECTION_OBSERVER,
    31     ConstructorCallWith=Document,
    32     ConstructorMayThrowException,
    33     Constructor(IntersectionObserverCallback callback, optional IntersectionObserverInit options),
    34     EnabledAtRuntime=IntersectionObserver
    35 ] interface IntersectionObserver {
    36     readonly attribute Element? root;
    37     readonly attribute DOMString rootMargin;
    38     readonly attribute sequence<double> thresholds;
     29#include "JSNodeCustom.h"
    3930
    40     void observe(Element target);
    41     void unobserve(Element target);
    42     void disconnect();
    43     sequence<IntersectionObserverEntry> takeRecords();
    44 };
     31namespace WebCore {
    4532
    46 [
    47     Conditional=INTERSECTION_OBSERVER,
    48     EnabledBySetting=IntersectionObserver
    49 ]
    50 dictionary IntersectionObserverInit {
    51     Element? root = null;
    52     DOMString rootMargin = "0px";
    53     (double or sequence<double>) threshold = 0.0;
    54 };
     33void JSIntersectionObserverEntry::visitAdditionalChildren(JSC::SlotVisitor& visitor)
     34{
     35    visitor.addOpaqueRoot(root(wrapped().target()));
     36}
     37
     38}

  • trunk/Source/WebCore/page/IntersectionObserver.cpp

    r237228 r237880  
    184184}
    185185
    186 Vector<Ref<IntersectionObserverEntry>> IntersectionObserver::takeRecords()
    187 {
    188     return WTFMove(m_queuedEntries);
     186auto IntersectionObserver::takeRecords() -> TakenRecords
     187{
     188    return { WTFMove(m_queuedEntries), WTFMove(m_pendingTargets) };
    189189}
    190190
    … …  
    245245void IntersectionObserver::appendQueuedEntry(Ref<IntersectionObserverEntry>&& entry)
    246246{
     247    ASSERT(entry->target());
     248    m_pendingTargets.append(*entry->target());
    247249    m_queuedEntries.append(WTFMove(entry));
    248250}
    … …  
    250252void IntersectionObserver::notify()
    251253{
    252     if (m_queuedEntries.isEmpty() || !m_callback || !m_callback->canInvokeCallback())
    253         return;
    254 
    255     m_callback->handleEvent(takeRecords(), *this);
     254    if (m_queuedEntries.isEmpty()) {
     255        ASSERT(m_pendingTargets.isEmpty());
     256        return;
     257    }
     258
     259    auto takenRecords = takeRecords();
     260    m_callback->handleEvent(WTFMove(takenRecords.records), *this);
    256261}
    257262
    258263bool IntersectionObserver::hasPendingActivity() const
    259264{
    260     return hasObservationTargets() && trackingDocument();
     265    return (hasObservationTargets() && trackingDocument()) || !m_queuedEntries.isEmpty();
    261266}
    262267
    … …  
    275280    disconnect();
    276281    m_callback = nullptr;
     282    m_queuedEntries.clear();
     283    m_pendingTargets.clear();
    277284}
    278285

  • trunk/Source/WebCore/page/IntersectionObserver.h

    r235943 r237880  
    2929
    3030#include "ActiveDOMObject.h"
     31#include "GCReachableRef.h"
    3132#include "IntersectionObserverCallback.h"
    3233#include "IntersectionObserverEntry.h"
    … …  
    8182    void disconnect();
    8283
    83     Vector<Ref<IntersectionObserverEntry>> takeRecords();
     84    struct TakenRecords {
     85        Vector<Ref<IntersectionObserverEntry>> records;
     86        Vector<GCReachableRef<Element>> pendingTargets;
     87    };
     88    TakenRecords takeRecords();
    8489
    8590    void targetDestroyed(Element&);
    … …  
    110115    RefPtr<IntersectionObserverCallback> m_callback;
    111116    Vector<Element*> m_observationTargets;
     117    Vector<GCReachableRef<Element>> m_pendingTargets;
    112118    Vector<Ref<IntersectionObserverEntry>> m_queuedEntries;
    113119};

  • trunk/Source/WebCore/page/IntersectionObserver.idl

    r235736 r237880  
    4141    void unobserve(Element target);
    4242    void disconnect();
    43     sequence<IntersectionObserverEntry> takeRecords();
     43    [ResultField=records] sequence<IntersectionObserverEntry> takeRecords();
    4444};
    4545

  • trunk/Source/WebCore/page/IntersectionObserverEntry.h

    r234732 r237880  
    6161    RefPtr<DOMRectReadOnly> boundingClientRect() const { return m_boundingClientRect; }
    6262    RefPtr<DOMRectReadOnly> intersectionRect() const { return m_intersectionRect; }
    63     RefPtr<Element> target() const { return m_target; }
     63    Element* target() const { return m_target.get(); }
    6464
    6565    bool isIntersecting() const { return m_isIntersecting; }

  • trunk/Source/WebCore/page/IntersectionObserverEntry.idl

    r234732 r237880  
    3232    Constructor(IntersectionObserverEntryInit intersectionObserverEntryInit),
    3333    ImplementationLacksVTable,
    34     EnabledAtRuntime=IntersectionObserver
     34    EnabledAtRuntime=IntersectionObserver,
     35    JSCustomMarkFunction,
    3536] interface IntersectionObserverEntry {
    3637    readonly attribute DOMHighResTimeStamp time;
Note: See TracChangeset for help on using the changeset viewer.