Changeset 281188 in webkit

Timestamp:

Aug 18, 2021, 8:25:04 AM (5 years ago)

Author:

Chris Dumez

Message:

Crash under JSIntersectionObserverCallback::handleEvent()
​https://bugs.webkit.org/show_bug.cgi?id=229196
<rdar://82016054>

Reviewed by Geoffrey Garen.

Early return in IntersectionObserver::notify() if the callback has already been destroyed.
This is not supposed to happen as we're supposed to be keeping the JSIntersectionObserver
wrapper alive as long as the IntersectionObserver may fire events, which should keep the
callback alive too. However, despite Ryosuke's fix in r280549, the crash is still happening
in the wild. To address the crash, I am simply doing an early return for now, similarly to
what we already do in MutationObserver::deliver(), while keeping a debug assertion around
in hope of finding the root cause at some point.

No new tests, we do not not how this is happening yet.

• page/IntersectionObserver.cpp:

(WebCore::IntersectionObserver::notify):

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• html/LazyLoadFrameObserver.cpp (modified) (1 diff)
• html/LazyLoadImageObserver.cpp (modified) (1 diff)
• page/IntersectionObserver.cpp (modified) (1 diff)
• page/IntersectionObserverCallback.h (modified) (1 diff)
• page/ResizeObserver.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-08-18  Chris Dumez  <cdumez@apple.com>
+
+        Crash under JSIntersectionObserverCallback::handleEvent()
+        https://bugs.webkit.org/show_bug.cgi?id=229196
+        <rdar://82016054>
+
+        Reviewed by Geoffrey Garen.
+
+        Early return in IntersectionObserver::notify() if the callback has already been destroyed.
+        This is not supposed to happen as we're supposed to be keeping the JSIntersectionObserver
+        wrapper alive as long as the IntersectionObserver may fire events, which should keep the
+        callback alive too. However, despite Ryosuke's fix in r280549, the crash is still happening
+        in the wild. To address the crash, I am simply doing an early return for now, similarly to
+        what we already do in MutationObserver::deliver(), while keeping a debug assertion around
+        in hope of finding the root cause at some point.
+
+        No new tests, we do not not how this is happening yet.
+
+        * page/IntersectionObserver.cpp:
+        (WebCore::IntersectionObserver::notify):
+
 2021-08-18  Martin Robinson  <mrobinson@webkit.org>
 

trunk/Source/WebCore/html/LazyLoadFrameObserver.cpp

@@ -44 +44 @@
 
 private:
+    bool hasCallback() const final { return true; }
+
     CallbackResult<void> handleEvent(IntersectionObserver&, const Vector<Ref<IntersectionObserverEntry>>& entries, IntersectionObserver&) final
     {

trunk/Source/WebCore/html/LazyLoadImageObserver.cpp

@@ -44 +44 @@
 
 private:
+    bool hasCallback() const final { return true; }
+
     CallbackResult<void> handleEvent(IntersectionObserver&, const Vector<Ref<IntersectionObserverEntry>>& entries, IntersectionObserver&) final
     {

trunk/Source/WebCore/page/IntersectionObserver.cpp

@@ -274 +274 @@
     }
 
+    auto takenRecords = takeRecords();
+
+    // FIXME: The JSIntersectionObserver wrapper should be kept alive as long as the intersection observer can fire events.
+    ASSERT(m_callback->hasCallback());
+    if (!m_callback->hasCallback())
+        return;
+
     auto* context = m_callback->scriptExecutionContext();
     if (!context)
         return;
-
-    auto takenRecords = takeRecords();
 
     InspectorInstrumentation::willFireObserverCallback(*context, "IntersectionObserver"_s);

trunk/Source/WebCore/page/IntersectionObserverCallback.h

@@ -42 +42 @@
     using ActiveDOMCallback::ActiveDOMCallback;
 
-    virtual bool hasCallback() const { return false; }
+    virtual bool hasCallback() const = 0;
 
     virtual CallbackResult<void> handleEvent(IntersectionObserver&, const Vector<Ref<IntersectionObserverEntry>>&, IntersectionObserver&) = 0;

trunk/Source/WebCore/page/ResizeObserver.cpp

@@ -125 +125 @@
     auto activeObservationTargets = std::exchange(m_activeObservationTargets, { });
 
+    // FIXME: The JSResizeObserver wrapper should be kept alive as long as the resize observer can fire events.
+    ASSERT(m_callback->hasCallback());
+    if (!m_callback->hasCallback())
+        return;
+
     auto* context = m_callback->scriptExecutionContext();
     if (!context)