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

Changeset 284802 in webkit


Ignore:
Timestamp:
Oct 25, 2021, 12:09:38 PM (5 years ago)
Author:
Alan Coon
Message:

Cherry-pick r283590. rdar://problem/80837616

ASSERT(m_callback->hasCallback()) under IntersectionObserver::notify()
​https://bugs.webkit.org/show_bug.cgi?id=231235
<rdar://80837616>

Reviewed by Ryosuke Niwa.

Source/WebCore:

IntersectionObserver's JS callback stays alive as long as its JS wrapper and
its JS wrapper's lifetime relies on the IntersectionObserver::isReachableFromOpaqueRoots()
implementation. isReachableFromOpaqueRoots() keeps the wrapper alive as long
as the JS wrappers of observation / pending targets are alive. However, as per specification,
we always need to dispatch an observation for an observation target, even if that target
is not connected. Our code was already taking care of dispatching such observation. However,
there was nothing keeping the observation target alive in this case and thus nothing keeping
the JS callback alive either.

To address the issue, I am introducing a new m_targetsWaitingForFirstObservation data member
which holds a strong ref to the observation target until the next time we call notify().
This makes sure that the observation target (and its JS wrapper) stays alive long enough for
us to dispatch the first observation for it. I also updated isReachableFromOpaqueRoots() to
return true as long as m_targetsWaitingForFirstObservation is non-empty so that the
IntersectionObserver's JS wrapper (and thus the JS callback) stay alive long enough too.

Tests: intersection-observer/observe-disconnected-target-crash.html

intersection-observer/observe-disconnected-target.html

  • page/IntersectionObserver.cpp: (WebCore::IntersectionObserver::observe): (WebCore::IntersectionObserver::unobserve): (WebCore::IntersectionObserver::removeAllTargets): (WebCore::IntersectionObserver::notify): (WebCore::IntersectionObserver::isReachableFromOpaqueRoots const):
  • page/IntersectionObserver.h:

LayoutTests:

Add layout test coverage both for the crash and the Web facing behavior.

  • intersection-observer/observe-disconnected-target-crash-expected.txt: Added.
  • intersection-observer/observe-disconnected-target-crash.html: Added.
  • intersection-observer/observe-disconnected-target-expected.txt: Added.
  • intersection-observer/observe-disconnected-target.html: Added.

git-svn-id: ​https://svn.webkit.org/repository/webkit/trunk@283590 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Location:
branches/safari-612-branch
Files:
6 added
4 edited

Legend:

Unmodified
Added
Removed

  • branches/safari-612-branch/LayoutTests/ChangeLog

    r284801 r284802  
     12021-10-25  Null  <null@apple.com>
     2
     3        Cherry-pick r283590. rdar://problem/80837616
     4
     5    ASSERT(m_callback->hasCallback()) under IntersectionObserver::notify()
     6    https://bugs.webkit.org/show_bug.cgi?id=231235
     7    <rdar://80837616>
     8   
     9    Reviewed by Ryosuke Niwa.
     10   
     11    Source/WebCore:
     12   
     13    IntersectionObserver's JS callback stays alive as long as its JS wrapper and
     14    its JS wrapper's lifetime relies on the IntersectionObserver::isReachableFromOpaqueRoots()
     15    implementation. isReachableFromOpaqueRoots() keeps the wrapper alive as long
     16    as the JS wrappers of observation / pending targets are alive. However, as per specification,
     17    we always need to dispatch an observation for an observation target, even if that target
     18    is not connected. Our code was already taking care of dispatching such observation. However,
     19    there was nothing keeping the observation target alive in this case and thus nothing keeping
     20    the JS callback alive either.
     21   
     22    To address the issue, I am introducing a new m_targetsWaitingForFirstObservation data member
     23    which holds a strong ref to the observation target until the next time we call notify().
     24    This makes sure that the observation target (and its JS wrapper) stays alive long enough for
     25    us to dispatch the first observation for it. I also updated isReachableFromOpaqueRoots() to
     26    return true as long as m_targetsWaitingForFirstObservation is non-empty so that the
     27    IntersectionObserver's JS wrapper (and thus the JS callback) stay alive long enough too.
     28   
     29    Tests: intersection-observer/observe-disconnected-target-crash.html
     30           intersection-observer/observe-disconnected-target.html
     31   
     32    * page/IntersectionObserver.cpp:
     33    (WebCore::IntersectionObserver::observe):
     34    (WebCore::IntersectionObserver::unobserve):
     35    (WebCore::IntersectionObserver::removeAllTargets):
     36    (WebCore::IntersectionObserver::notify):
     37    (WebCore::IntersectionObserver::isReachableFromOpaqueRoots const):
     38    * page/IntersectionObserver.h:
     39   
     40    LayoutTests:
     41   
     42    Add layout test coverage both for the crash and the Web facing behavior.
     43   
     44    * intersection-observer/observe-disconnected-target-crash-expected.txt: Added.
     45    * intersection-observer/observe-disconnected-target-crash.html: Added.
     46    * intersection-observer/observe-disconnected-target-expected.txt: Added.
     47    * intersection-observer/observe-disconnected-target.html: Added.
     48   
     49   
     50    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@283590 268f45cc-cd09-0410-ab3c-d52691b4dbfc
     51
     52    2021-10-05  Chris Dumez  <cdumez@apple.com>
     53
     54            ASSERT(m_callback->hasCallback()) under IntersectionObserver::notify()
     55            https://bugs.webkit.org/show_bug.cgi?id=231235
     56            <rdar://80837616>
     57
     58            Reviewed by Ryosuke Niwa.
     59
     60            Add layout test coverage both for the crash and the Web facing behavior.
     61
     62            * intersection-observer/observe-disconnected-target-crash-expected.txt: Added.
     63            * intersection-observer/observe-disconnected-target-crash.html: Added.
     64            * intersection-observer/observe-disconnected-target-expected.txt: Added.
     65            * intersection-observer/observe-disconnected-target.html: Added.
     66
    1672021-10-25  Null  <null@apple.com>
    268

  • branches/safari-612-branch/Source/WebCore/ChangeLog

    r284801 r284802  
     12021-10-25  Null  <null@apple.com>
     2
     3        Cherry-pick r283590. rdar://problem/80837616
     4
     5    ASSERT(m_callback->hasCallback()) under IntersectionObserver::notify()
     6    https://bugs.webkit.org/show_bug.cgi?id=231235
     7    <rdar://80837616>
     8   
     9    Reviewed by Ryosuke Niwa.
     10   
     11    Source/WebCore:
     12   
     13    IntersectionObserver's JS callback stays alive as long as its JS wrapper and
     14    its JS wrapper's lifetime relies on the IntersectionObserver::isReachableFromOpaqueRoots()
     15    implementation. isReachableFromOpaqueRoots() keeps the wrapper alive as long
     16    as the JS wrappers of observation / pending targets are alive. However, as per specification,
     17    we always need to dispatch an observation for an observation target, even if that target
     18    is not connected. Our code was already taking care of dispatching such observation. However,
     19    there was nothing keeping the observation target alive in this case and thus nothing keeping
     20    the JS callback alive either.
     21   
     22    To address the issue, I am introducing a new m_targetsWaitingForFirstObservation data member
     23    which holds a strong ref to the observation target until the next time we call notify().
     24    This makes sure that the observation target (and its JS wrapper) stays alive long enough for
     25    us to dispatch the first observation for it. I also updated isReachableFromOpaqueRoots() to
     26    return true as long as m_targetsWaitingForFirstObservation is non-empty so that the
     27    IntersectionObserver's JS wrapper (and thus the JS callback) stay alive long enough too.
     28   
     29    Tests: intersection-observer/observe-disconnected-target-crash.html
     30           intersection-observer/observe-disconnected-target.html
     31   
     32    * page/IntersectionObserver.cpp:
     33    (WebCore::IntersectionObserver::observe):
     34    (WebCore::IntersectionObserver::unobserve):
     35    (WebCore::IntersectionObserver::removeAllTargets):
     36    (WebCore::IntersectionObserver::notify):
     37    (WebCore::IntersectionObserver::isReachableFromOpaqueRoots const):
     38    * page/IntersectionObserver.h:
     39   
     40    LayoutTests:
     41   
     42    Add layout test coverage both for the crash and the Web facing behavior.
     43   
     44    * intersection-observer/observe-disconnected-target-crash-expected.txt: Added.
     45    * intersection-observer/observe-disconnected-target-crash.html: Added.
     46    * intersection-observer/observe-disconnected-target-expected.txt: Added.
     47    * intersection-observer/observe-disconnected-target.html: Added.
     48   
     49   
     50    git-svn-id: https://svn.webkit.org/repository/webkit/trunk@283590 268f45cc-cd09-0410-ab3c-d52691b4dbfc
     51
     52    2021-10-05  Chris Dumez  <cdumez@apple.com>
     53
     54            ASSERT(m_callback->hasCallback()) under IntersectionObserver::notify()
     55            https://bugs.webkit.org/show_bug.cgi?id=231235
     56            <rdar://80837616>
     57
     58            Reviewed by Ryosuke Niwa.
     59
     60            IntersectionObserver's JS callback stays alive as long as its JS wrapper and
     61            its JS wrapper's lifetime relies on the IntersectionObserver::isReachableFromOpaqueRoots()
     62            implementation. isReachableFromOpaqueRoots() keeps the wrapper alive as long
     63            as the JS wrappers of observation / pending targets are alive. However, as per specification,
     64            we always need to dispatch an observation for an observation target, even if that target
     65            is not connected. Our code was already taking care of dispatching such observation. However,
     66            there was nothing keeping the observation target alive in this case and thus nothing keeping
     67            the JS callback alive either.
     68
     69            To address the issue, I am introducing a new m_targetsWaitingForFirstObservation data member
     70            which holds a strong ref to the observation target until the next time we call notify().
     71            This makes sure that the observation target (and its JS wrapper) stays alive long enough for
     72            us to dispatch the first observation for it. I also updated isReachableFromOpaqueRoots() to
     73            return true as long as m_targetsWaitingForFirstObservation is non-empty so that the
     74            IntersectionObserver's JS wrapper (and thus the JS callback) stay alive long enough too.
     75
     76            Tests: intersection-observer/observe-disconnected-target-crash.html
     77                   intersection-observer/observe-disconnected-target.html
     78
     79            * page/IntersectionObserver.cpp:
     80            (WebCore::IntersectionObserver::observe):
     81            (WebCore::IntersectionObserver::unobserve):
     82            (WebCore::IntersectionObserver::removeAllTargets):
     83            (WebCore::IntersectionObserver::notify):
     84            (WebCore::IntersectionObserver::isReachableFromOpaqueRoots const):
     85            * page/IntersectionObserver.h:
     86
    1872021-10-25  Null  <null@apple.com>
    288

  • branches/safari-612-branch/Source/WebCore/page/IntersectionObserver.cpp

    r281188 r284802  
    171171    m_observationTargets.append(makeWeakPtr(target));
    172172
     173    // Per the specification, we should dispatch at least one observation for the target. For this reason, we make sure to keep the
     174    // target alive until this first observation. This, in turn, will keep the IntersectionObserver's JS wrapper alive via
     175    // isReachableFromOpaqueRoots(), so the callback stays alive.
     176    m_targetsWaitingForFirstObservation.append(target);
     177
    173178    auto* document = trackingDocument();
    174179    if (!hadObservationTargets)
    … …  
    184189    bool removed = m_observationTargets.removeFirst(&target);
    185190    ASSERT_UNUSED(removed, removed);
     191    m_targetsWaitingForFirstObservation.removeFirstMatching([&](auto& pendingTarget) { return pendingTarget.ptr() == &target; });
    186192
    187193    if (!hasObservationTargets()) {
    … …  
    209215{
    210216    m_observationTargets.removeFirst(&target);
     217    m_targetsWaitingForFirstObservation.removeFirstMatching([&](auto& pendingTarget) { return pendingTarget.ptr() == &target; });
    211218    if (!hasObservationTargets()) {
    212219        if (auto* document = trackingDocument())
    … …  
    234241    }
    235242    m_observationTargets.clear();
     243    m_targetsWaitingForFirstObservation.clear();
    236244}
    237245
    … …  
    275283
    276284    auto takenRecords = takeRecords();
     285    auto targetsWaitingForFirstObservation = std::exchange(m_targetsWaitingForFirstObservation, { });
    277286
    278287    // FIXME: The JSIntersectionObserver wrapper should be kept alive as long as the intersection observer can fire events.
    … …  
    300309            return true;
    301310    }
    302     return false;
     311    return !m_targetsWaitingForFirstObservation.isEmpty();
    303312}
    304313

  • branches/safari-612-branch/Source/WebCore/page/IntersectionObserver.h

    r279800 r284802  
    123123    Vector<GCReachableRef<Element>> m_pendingTargets;
    124124    Vector<Ref<IntersectionObserverEntry>> m_queuedEntries;
     125    Vector<GCReachableRef<Element>> m_targetsWaitingForFirstObservation;
    125126};
    126127
Note: See TracChangeset for help on using the changeset viewer.