- Revision
- 283590
- Author
- [email protected]
- Date
- 2021-10-05 17:16:33 -0700 (Tue, 05 Oct 2021)
Log 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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (283589 => 283590)
--- trunk/LayoutTests/ChangeLog 2021-10-05 23:52:35 UTC (rev 283589)
+++ trunk/LayoutTests/ChangeLog 2021-10-06 00:16:33 UTC (rev 283590)
@@ -1,3 +1,18 @@
+2021-10-05 Chris Dumez <[email protected]>
+
+ ASSERT(m_callback->hasCallback()) under IntersectionObserver::notify()
+ https://bugs.webkit.org/show_bug.cgi?id=231235
+ <rdar://80837616>
+
+ Reviewed by Ryosuke Niwa.
+
+ 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.
+
2021-10-05 Ayumi Kojima <[email protected]>
[ iOS Release ] fast/events/ios/rotation/layout-viewport-during-safari-type-rotation.html is flaky failing.
Added: trunk/LayoutTests/intersection-observer/observe-disconnected-target-crash-expected.txt (0 => 283590)
--- trunk/LayoutTests/intersection-observer/observe-disconnected-target-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/intersection-observer/observe-disconnected-target-crash-expected.txt 2021-10-06 00:16:33 UTC (rev 283590)
@@ -0,0 +1 @@
+This test passes if it doesn't crash.
Added: trunk/LayoutTests/intersection-observer/observe-disconnected-target-crash.html (0 => 283590)
--- trunk/LayoutTests/intersection-observer/observe-disconnected-target-crash.html (rev 0)
+++ trunk/LayoutTests/intersection-observer/observe-disconnected-target-crash.html 2021-10-06 00:16:33 UTC (rev 283590)
@@ -0,0 +1,20 @@
+<p>This test passes if it doesn't crash.</p>
+<script>
+ if (window.testRunner) {
+ testRunner.dumpAsText();
+ testRunner.waitUntilDone();
+ }
+ _onload_ = async () => {
+ for (let i = 0; i < 8; i++) {
+ document.createElement('div');
+ }
+ for (let i = 0; i < 8; i++) {
+ new IntersectionObserver(() => {});
+ }
+ let intersectionObserver = new IntersectionObserver(() => {});
+ await intersectionObserver.observe(document.createElement('div'));
+ new ImageData(1000, 10000);
+ if (window.testRunner)
+ testRunner.notifyDone();
+ };
+</script>
Added: trunk/LayoutTests/intersection-observer/observe-disconnected-target-expected.txt (0 => 283590)
--- trunk/LayoutTests/intersection-observer/observe-disconnected-target-expected.txt (rev 0)
+++ trunk/LayoutTests/intersection-observer/observe-disconnected-target-expected.txt 2021-10-06 00:16:33 UTC (rev 283590)
@@ -0,0 +1,15 @@
+Tests that an observation is received when calling IntersectionObserver.observe() with a disconnected target.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS receivedObservations is false
+PASS observations.length is 1
+PASS observations[0].target.tagName is "DIV"
+PASS observations[0].isIntersecting is false
+PASS observations[0].intersectionRatio is 0.0
+PASS observations[0].target.foo is 1
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/intersection-observer/observe-disconnected-target.html (0 => 283590)
--- trunk/LayoutTests/intersection-observer/observe-disconnected-target.html (rev 0)
+++ trunk/LayoutTests/intersection-observer/observe-disconnected-target.html 2021-10-06 00:16:33 UTC (rev 283590)
@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<script>
+ description("Tests that an observation is received when calling IntersectionObserver.observe() with a disconnected target.");
+ jsTestIsAsync = true;
+
+ let receivedObservations = false;
+ _onload_ = () => {
+ let intersectionObserver = new IntersectionObserver((_observations) => {
+ gc();
+ observations = _observations;
+
+ shouldBeFalse("receivedObservations");
+ receivedObservations = true;
+
+ shouldBe("observations.length", "1");
+ shouldBeEqualToString("observations[0].target.tagName", "DIV");
+ shouldBeFalse("observations[0].isIntersecting");
+ shouldBe("observations[0].intersectionRatio", "0.0");
+ shouldBe("observations[0].target.foo", "1");
+ setTimeout(finishJSTest, 100);
+ });
+ let div = document.createElement('div');
+ div.foo = 1;
+ intersectionObserver.observe(div);
+ div = null;
+ gc();
+ setTimeout(gc, 0);
+ };
+</script>
+</body>
+</html>
Added: trunk/LayoutTests/intersection-observer/observe-then-disconnect-target-expected.txt (0 => 283590)
--- trunk/LayoutTests/intersection-observer/observe-then-disconnect-target-expected.txt (rev 0)
+++ trunk/LayoutTests/intersection-observer/observe-then-disconnect-target-expected.txt 2021-10-06 00:16:33 UTC (rev 283590)
@@ -0,0 +1,18 @@
+Tests that an observation is received after disconnecting a visible target
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS observations.length is 1
+PASS observations[0].target.tagName is "DIV"
+PASS observations[0].target.foo is 1
+PASS observations[0].isIntersecting is true
+* Removing target from document
+PASS observations.length is 1
+PASS observations[0].target.tagName is "DIV"
+PASS observations[0].target.foo is 1
+PASS observations[0].isIntersecting is false
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/intersection-observer/observe-then-disconnect-target.html (0 => 283590)
--- trunk/LayoutTests/intersection-observer/observe-then-disconnect-target.html (rev 0)
+++ trunk/LayoutTests/intersection-observer/observe-then-disconnect-target.html 2021-10-06 00:16:33 UTC (rev 283590)
@@ -0,0 +1,43 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<div id="testDiv">test</div>
+<script>
+ description("Tests that an observation is received after disconnecting a visible target");
+ jsTestIsAsync = true;
+
+ let receivedInitialObservation = false;
+ _onload_ = () => {
+ let intersectionObserver = new IntersectionObserver((_observations) => {
+ gc();
+ observations = _observations;
+
+ shouldBe("observations.length", "1");
+ shouldBeEqualToString("observations[0].target.tagName", "DIV");
+ shouldBe("observations[0].target.foo", "1");
+
+ if (!receivedInitialObservation) {
+ receivedInitialObservation = true;
+ shouldBeTrue("observations[0].isIntersecting");
+ setTimeout(() => {
+ debug("* Removing target from document");
+ document.getElementById("testDiv").remove();
+ gc();
+ setTimeout(gc, 0);
+ }, 0);
+ } else {
+ shouldBeFalse("observations[0].isIntersecting");
+ setTimeout(finishJSTest, 100);
+ }
+ });
+ let div = document.getElementById("testDiv");
+ div.foo = 1;
+ intersectionObserver.observe(div);
+ div = null;
+ gc();
+ setTimeout(gc, 0);
+ };
+</script>
+</body>
+</html>
Modified: trunk/Source/WebCore/ChangeLog (283589 => 283590)
--- trunk/Source/WebCore/ChangeLog 2021-10-05 23:52:35 UTC (rev 283589)
+++ trunk/Source/WebCore/ChangeLog 2021-10-06 00:16:33 UTC (rev 283590)
@@ -1,3 +1,38 @@
+2021-10-05 Chris Dumez <[email protected]>
+
+ ASSERT(m_callback->hasCallback()) under IntersectionObserver::notify()
+ https://bugs.webkit.org/show_bug.cgi?id=231235
+ <rdar://80837616>
+
+ Reviewed by Ryosuke Niwa.
+
+ 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:
+
2021-10-05 Jean-Yves Avenard <[email protected]>
createImageBitmap using a HLS video as source always return a black image.
Modified: trunk/Source/WebCore/page/IntersectionObserver.cpp (283589 => 283590)
--- trunk/Source/WebCore/page/IntersectionObserver.cpp 2021-10-05 23:52:35 UTC (rev 283589)
+++ trunk/Source/WebCore/page/IntersectionObserver.cpp 2021-10-06 00:16:33 UTC (rev 283590)
@@ -169,6 +169,11 @@
bool hadObservationTargets = hasObservationTargets();
m_observationTargets.append(makeWeakPtr(target));
+ // Per the specification, we should dispatch at least one observation for the target. For this reason, we make sure to keep the
+ // target alive until this first observation. This, in turn, will keep the IntersectionObserver's JS wrapper alive via
+ // isReachableFromOpaqueRoots(), so the callback stays alive.
+ m_targetsWaitingForFirstObservation.append(target);
+
auto* document = trackingDocument();
if (!hadObservationTargets)
document->addIntersectionObserver(*this);
@@ -182,6 +187,7 @@
bool removed = m_observationTargets.removeFirst(&target);
ASSERT_UNUSED(removed, removed);
+ m_targetsWaitingForFirstObservation.removeFirstMatching([&](auto& pendingTarget) { return pendingTarget.ptr() == ⌖ });
if (!hasObservationTargets()) {
if (auto* document = trackingDocument())
@@ -207,6 +213,7 @@
void IntersectionObserver::targetDestroyed(Element& target)
{
m_observationTargets.removeFirst(&target);
+ m_targetsWaitingForFirstObservation.removeFirstMatching([&](auto& pendingTarget) { return pendingTarget.ptr() == ⌖ });
if (!hasObservationTargets()) {
if (auto* document = trackingDocument())
document->removeIntersectionObserver(*this);
@@ -232,6 +239,7 @@
ASSERT_UNUSED(removed, removed);
}
m_observationTargets.clear();
+ m_targetsWaitingForFirstObservation.clear();
}
void IntersectionObserver::rootDestroyed()
@@ -273,6 +281,7 @@
}
auto takenRecords = takeRecords();
+ auto targetsWaitingForFirstObservation = std::exchange(m_targetsWaitingForFirstObservation, { });
// FIXME: The JSIntersectionObserver wrapper should be kept alive as long as the intersection observer can fire events.
ASSERT(m_callback->hasCallback());
@@ -298,7 +307,7 @@
if (visitor.containsOpaqueRoot(target->opaqueRoot()))
return true;
}
- return false;
+ return !m_targetsWaitingForFirstObservation.isEmpty();
}
} // namespace WebCore
Modified: trunk/Source/WebCore/page/IntersectionObserver.h (283589 => 283590)
--- trunk/Source/WebCore/page/IntersectionObserver.h 2021-10-05 23:52:35 UTC (rev 283589)
+++ trunk/Source/WebCore/page/IntersectionObserver.h 2021-10-06 00:16:33 UTC (rev 283590)
@@ -120,6 +120,7 @@
Vector<WeakPtr<Element>> m_observationTargets;
Vector<GCReachableRef<Element>> m_pendingTargets;
Vector<Ref<IntersectionObserverEntry>> m_queuedEntries;
+ Vector<GCReachableRef<Element>> m_targetsWaitingForFirstObservation;
};
