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

Changeset 283590 in webkit


Ignore:
Timestamp:
Oct 5, 2021, 5:16:33 PM (5 years ago)
Author:
Chris Dumez
Message:

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.
Location:
trunk
Files:
6 added
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r283587 r283590  
     12021-10-05  Chris Dumez  <cdumez@apple.com>
     2
     3        ASSERT(m_callback->hasCallback()) under IntersectionObserver::notify()
     4        https://bugs.webkit.org/show_bug.cgi?id=231235
     5        <rdar://80837616>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        Add layout test coverage both for the crash and the Web facing behavior.
     10
     11        * intersection-observer/observe-disconnected-target-crash-expected.txt: Added.
     12        * intersection-observer/observe-disconnected-target-crash.html: Added.
     13        * intersection-observer/observe-disconnected-target-expected.txt: Added.
     14        * intersection-observer/observe-disconnected-target.html: Added.
     15
    1162021-10-05  Ayumi Kojima  <ayumi_kojima@apple.com>
    217

  • trunk/Source/WebCore/ChangeLog

    r283585 r283590  
     12021-10-05  Chris Dumez  <cdumez@apple.com>
     2
     3        ASSERT(m_callback->hasCallback()) under IntersectionObserver::notify()
     4        https://bugs.webkit.org/show_bug.cgi?id=231235
     5        <rdar://80837616>
     6
     7        Reviewed by Ryosuke Niwa.
     8
     9        IntersectionObserver's JS callback stays alive as long as its JS wrapper and
     10        its JS wrapper's lifetime relies on the IntersectionObserver::isReachableFromOpaqueRoots()
     11        implementation. isReachableFromOpaqueRoots() keeps the wrapper alive as long
     12        as the JS wrappers of observation / pending targets are alive. However, as per specification,
     13        we always need to dispatch an observation for an observation target, even if that target
     14        is not connected. Our code was already taking care of dispatching such observation. However,
     15        there was nothing keeping the observation target alive in this case and thus nothing keeping
     16        the JS callback alive either.
     17
     18        To address the issue, I am introducing a new m_targetsWaitingForFirstObservation data member
     19        which holds a strong ref to the observation target until the next time we call notify().
     20        This makes sure that the observation target (and its JS wrapper) stays alive long enough for
     21        us to dispatch the first observation for it. I also updated isReachableFromOpaqueRoots() to
     22        return true as long as m_targetsWaitingForFirstObservation is non-empty so that the
     23        IntersectionObserver's JS wrapper (and thus the JS callback) stay alive long enough too.
     24
     25        Tests: intersection-observer/observe-disconnected-target-crash.html
     26               intersection-observer/observe-disconnected-target.html
     27
     28        * page/IntersectionObserver.cpp:
     29        (WebCore::IntersectionObserver::observe):
     30        (WebCore::IntersectionObserver::unobserve):
     31        (WebCore::IntersectionObserver::removeAllTargets):
     32        (WebCore::IntersectionObserver::notify):
     33        (WebCore::IntersectionObserver::isReachableFromOpaqueRoots const):
     34        * page/IntersectionObserver.h:
     35
    1362021-10-05  Jean-Yves Avenard  <jya@apple.com>
    237

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

    r282487 r283590  
    170170    m_observationTargets.append(makeWeakPtr(target));
    171171
     172    // Per the specification, we should dispatch at least one observation for the target. For this reason, we make sure to keep the
     173    // target alive until this first observation. This, in turn, will keep the IntersectionObserver's JS wrapper alive via
     174    // isReachableFromOpaqueRoots(), so the callback stays alive.
     175    m_targetsWaitingForFirstObservation.append(target);
     176
    172177    auto* document = trackingDocument();
    173178    if (!hadObservationTargets)
    … …  
    183188    bool removed = m_observationTargets.removeFirst(&target);
    184189    ASSERT_UNUSED(removed, removed);
     190    m_targetsWaitingForFirstObservation.removeFirstMatching([&](auto& pendingTarget) { return pendingTarget.ptr() == &target; });
    185191
    186192    if (!hasObservationTargets()) {
    … …  
    208214{
    209215    m_observationTargets.removeFirst(&target);
     216    m_targetsWaitingForFirstObservation.removeFirstMatching([&](auto& pendingTarget) { return pendingTarget.ptr() == &target; });
    210217    if (!hasObservationTargets()) {
    211218        if (auto* document = trackingDocument())
    … …  
    233240    }
    234241    m_observationTargets.clear();
     242    m_targetsWaitingForFirstObservation.clear();
    235243}
    236244
    … …  
    274282
    275283    auto takenRecords = takeRecords();
     284    auto targetsWaitingForFirstObservation = std::exchange(m_targetsWaitingForFirstObservation, { });
    276285
    277286    // FIXME: The JSIntersectionObserver wrapper should be kept alive as long as the intersection observer can fire events.
    … …  
    299308            return true;
    300309    }
    301     return false;
     310    return !m_targetsWaitingForFirstObservation.isEmpty();
    302311}
    303312

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

    r282487 r283590  
    121121    Vector<GCReachableRef<Element>> m_pendingTargets;
    122122    Vector<Ref<IntersectionObserverEntry>> m_queuedEntries;
     123    Vector<GCReachableRef<Element>> m_targetsWaitingForFirstObservation;
    123124};
    124125
Note: See TracChangeset for help on using the changeset viewer.