<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[280551] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/280551">280551</a></dd>
<dt>Author</dt> <dd>rniwa@webkit.org</dd>
<dt>Date</dt> <dd>2021-08-02 13:03:15 -0700 (Mon, 02 Aug 2021)</dd>
</dl>

<h3>Log Message</h3>
<pre>Calling unobserve on ResizeObserver should not clear existing observations in active targets
https://bugs.webkit.org/show_bug.cgi?id=228693

Reviewed by Chris Dumez.

Source/WebCore:

The bug was caused by ResizeObserver::removeObservation removing it from the active targets.

Note that there is nothing in the specification which alludes to this behavior,
and the new behavior is consistent with Firefox and the way IntersectionObserver works:
https://drafts.csswg.org/resize-observer/#dom-resizeobserver-unobserve

To keep elements alive while they're in the active targets but not in the observation targets,
this patch also makes each element of the active observation as opaque roots of ResizeObserver
in ResizeObserver::isReachableFromOpaqueRoots.

Test: resize-observer/resize-observer-keeps-element-of-queued-entry-alive.html

* page/ResizeObserver.cpp:
(WebCore::ResizeObserver::deliverObservations):
(WebCore::ResizeObserver::isReachableFromOpaqueRoots const):
(WebCore::ResizeObserver::removeObservation):

LayoutTests:

Added a regression test.

* resize-observer/resize-observer-keeps-element-of-queued-entry-alive-expected.txt: Added.
* resize-observer/resize-observer-keeps-element-of-queued-entry-alive.html: Added.</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCorepageResizeObservercpp">trunk/Source/WebCore/page/ResizeObserver.cpp</a></li>
</ul>

<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsresizeobserverresizeobserverkeepselementofqueuedentryaliveexpectedtxt">trunk/LayoutTests/resize-observer/resize-observer-keeps-element-of-queued-entry-alive-expected.txt</a></li>
<li><a href="#trunkLayoutTestsresizeobserverresizeobserverkeepselementofqueuedentryalivehtml">trunk/LayoutTests/resize-observer/resize-observer-keeps-element-of-queued-entry-alive.html</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (280550 => 280551)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog      2021-08-02 19:54:23 UTC (rev 280550)
+++ trunk/LayoutTests/ChangeLog 2021-08-02 20:03:15 UTC (rev 280551)
</span><span class="lines">@@ -1,3 +1,15 @@
</span><ins>+2021-08-01  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Calling unobserve on ResizeObserver should not clear existing observations in active targets
+        https://bugs.webkit.org/show_bug.cgi?id=228693
+
+        Reviewed by Chris Dumez.
+
+        Added a regression test.
+
+        * resize-observer/resize-observer-keeps-element-of-queued-entry-alive-expected.txt: Added.
+        * resize-observer/resize-observer-keeps-element-of-queued-entry-alive.html: Added.
+
</ins><span class="cx"> 2021-08-02  Ryosuke Niwa  <rniwa@webkit.org>
</span><span class="cx"> 
</span><span class="cx">         REGRESSION(r279800): IntersectionObserver may never get a delivery of an observation if the element has been unobserved and is disconnected
</span></span></pre></div>
<a id="trunkLayoutTestsresizeobserverresizeobserverkeepselementofqueuedentryaliveexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/resize-observer/resize-observer-keeps-element-of-queued-entry-alive-expected.txt (0 => 280551)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/resize-observer/resize-observer-keeps-element-of-queued-entry-alive-expected.txt                               (rev 0)
+++ trunk/LayoutTests/resize-observer/resize-observer-keeps-element-of-queued-entry-alive-expected.txt  2021-08-02 20:03:15 UTC (rev 280551)
</span><span class="lines">@@ -0,0 +1,25 @@
</span><ins>+This tests observing an element with an ResizeObserver and removing the element from the document while it is queued for delivery.
+
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+PASS - ResizeObserver wrapper is alive
+
+Done
+
</ins></span></pre></div>
<a id="trunkLayoutTestsresizeobserverresizeobserverkeepselementofqueuedentryalivehtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/resize-observer/resize-observer-keeps-element-of-queued-entry-alive.html (0 => 280551)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/resize-observer/resize-observer-keeps-element-of-queued-entry-alive.html                               (rev 0)
+++ trunk/LayoutTests/resize-observer/resize-observer-keeps-element-of-queued-entry-alive.html  2021-08-02 20:03:15 UTC (rev 280551)
</span><span class="lines">@@ -0,0 +1,88 @@
</span><ins>+<!DOCTYPE html><!-- webkit-test-runner [ ResizeObserverEnabled=true ] -->
+<html>
+<body>
+<pre id="log">This tests observing an element with an ResizeObserver and removing the element from the document while it is queued for delivery.
+
+</pre>
+<script src="../resources/gc.js"></script>
+<script>
+
+let initialNodeCount;
+function runTest()
+{
+    if (window.testRunner) {
+        testRunner.dumpAsText();
+        testRunner.waitUntilDone();
+    }
+
+    const promises = [];
+    for (let i = 0; i < 5; ++i)
+        promises.push(createResizeObserver());
+
+    Promise.all(promises).then(() => {
+        log.textContent += '\nDone\n';
+        if (window.testRunner)
+            testRunner.notifyDone();
+    });
+}
+
+function createResizeObserver()
+{
+    let element = document.createElement('div');
+    element.style.width = '100px';
+    element.style.height = '100px';
+
+    let anotherElement = document.createElement('div');
+
+    let stopped = false;
+    let testing = false;
+    let resizeObservers = [];
+
+    function helper(observer) {
+        if (!testing)
+            return;
+        if (stopped) {
+            log.textContent += observer.alive ? 'PASS - ResizeObserver wrapper is alive\n' : 'FAIL - ResizeObserver wrapper is dead\n';
+            return;
+        }
+        stopped = true;
+        for (const observer of resizeObservers)
+            observer.unobserve(element);
+        resizeObservers = [];
+        element.remove();
+        element = null;
+    }
+
+    (() => {
+        for (let i = 0; i < 5; ++i) {
+            const observer = new ResizeObserver(function () {
+                helper(this);
+                gc();
+            });
+            observer.alive = true;
+            observer.observe(element);
+            observer.observe(anotherElement);
+            resizeObservers.push(observer);
+        }
+    })();
+
+    document.body.appendChild(element);
+
+    return new Promise((resolve) => {
+        requestAnimationFrame(() => {
+            setTimeout(() => {
+                testing = true;
+                element.style.width = '200px';
+                requestAnimationFrame(() => {
+                    setTimeout(resolve, 0);
+                });
+            }, 0);
+        });
+    });
+}
+
+onload = runTest;
+
+</script>
+</body>
+</html>
</ins></span></pre></div>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (280550 => 280551)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog   2021-08-02 19:54:23 UTC (rev 280550)
+++ trunk/Source/WebCore/ChangeLog      2021-08-02 20:03:15 UTC (rev 280551)
</span><span class="lines">@@ -1,3 +1,27 @@
</span><ins>+2021-08-01  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Calling unobserve on ResizeObserver should not clear existing observations in active targets
+        https://bugs.webkit.org/show_bug.cgi?id=228693
+
+        Reviewed by Chris Dumez.
+
+        The bug was caused by ResizeObserver::removeObservation removing it from the active targets.
+
+        Note that there is nothing in the specification which alludes to this behavior,
+        and the new behavior is consistent with Firefox and the way IntersectionObserver works:
+        https://drafts.csswg.org/resize-observer/#dom-resizeobserver-unobserve
+
+        To keep elements alive while they're in the active targets but not in the observation targets,
+        this patch also makes each element of the active observation as opaque roots of ResizeObserver
+        in ResizeObserver::isReachableFromOpaqueRoots.
+
+        Test: resize-observer/resize-observer-keeps-element-of-queued-entry-alive.html
+
+        * page/ResizeObserver.cpp:
+        (WebCore::ResizeObserver::deliverObservations):
+        (WebCore::ResizeObserver::isReachableFromOpaqueRoots const):
+        (WebCore::ResizeObserver::removeObservation):
+
</ins><span class="cx"> 2021-08-02  Ryosuke Niwa  <rniwa@webkit.org>
</span><span class="cx"> 
</span><span class="cx">         REGRESSION(r279800): IntersectionObserver may never get a delivery of an observation if the element
</span></span></pre></div>
<a id="trunkSourceWebCorepageResizeObservercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/ResizeObserver.cpp (280550 => 280551)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/page/ResizeObserver.cpp     2021-08-02 19:54:23 UTC (rev 280550)
+++ trunk/Source/WebCore/page/ResizeObserver.cpp        2021-08-02 20:03:15 UTC (rev 280551)
</span><span class="lines">@@ -122,6 +122,7 @@
</span><span class="cx">         entries.append(ResizeObserverEntry::create(observation->target(), observation->computeContentRect()));
</span><span class="cx">     }
</span><span class="cx">     m_activeObservations.clear();
</span><ins>+    auto activeObservationTargets = std::exchange(m_activeObservationTargets, { });
</ins><span class="cx"> 
</span><span class="cx">     auto* context = m_callback->scriptExecutionContext();
</span><span class="cx">     if (!context)
</span><span class="lines">@@ -138,6 +139,10 @@
</span><span class="cx">         if (auto* target = observation->target(); target && visitor.containsOpaqueRoot(target->opaqueRoot()))
</span><span class="cx">             return true;
</span><span class="cx">     }
</span><ins>+    for (auto& target : m_activeObservationTargets) {
+        if (visitor.containsOpaqueRoot(target->opaqueRoot()))
+            return true;
+    }
</ins><span class="cx">     return false;
</span><span class="cx"> }
</span><span class="cx"> 
</span><span class="lines">@@ -164,14 +169,6 @@
</span><span class="cx"> 
</span><span class="cx"> bool ResizeObserver::removeObservation(const Element& target)
</span><span class="cx"> {
</span><del>-    m_activeObservationTargets.removeFirstMatching([&target](auto& pendingTarget) {
-        return pendingTarget.ptr() == &target;
-    });
-
-    m_activeObservations.removeFirstMatching([&target](auto& observation) {
-        return observation->target() == &target;
-    });
-
</del><span class="cx">     return m_observations.removeFirstMatching([&target](auto& observation) {
</span><span class="cx">         return observation->target() == &target;
</span><span class="cx">     });
</span></span></pre>
</div>
</div>

</body>
</html>