Bug 227194

<rdar://problem/79839851>

Now fixed in Gecko (https://bugzilla.mozilla.org/show_bug.cgi?id=1717362) and Chromium (https://bugs.chromium.org/p/chromium/issues/detail?id=1220041).

Oh, intersting. I think I misunderstood the semantics of ResizeObserver / InteractionObserver when I was advising Cathie to keep JS wrapper alive for these objects. We can't use GCReachableRef in these observers since m_pendingTargets aren't really transient entires but rather persistent entires that are always there while an element is being observed. Instead, we need to make visitChildren of JSResizeObserver and JSInteractionObserver each visit the opaque root of the observed nodes. This in turn implies that we need a lock for these data structures since visitChildren will be called concurrently to the main thread.

WTF? IntersectionObserver and ResizeObserver are ActiveDOMObjects? That doesn't make any sense whatsoever.

Created attachment 433178 [details] WIP

LOL, intersection-observer/root-element-deleted.html is failing because the test is asserting a bad GC behavior that I fixed in this very patch.

Created attachment 433204 [details] Fixes the bug

Comment on attachment 433204 [details] Fixes the bug View in context: https://bugs.webkit.org/attachment.cgi?id=433204&action=review Hi Ryosuke, Thanks for working on this! > Source/WebCore/ChangeLog:13 > + ResizeObserver and IntersectionObserver may get new observation entries queued later. Hmmm, I don't quite understand here. I think it seems right to keep ResizeObserver and IntersectionObserver's JS wrapper objects if there are observed elements. Not sure if I understand it correctly. Since now only the elements inside the active observations using GCReachableRef, so gc is able to remove the other elements. Then it will call `Element::~Element()` and `Element::disconnectFromResizeObservers()`. And the element will be removed from ResizeObserver. As to the elements inside the active observations, they should be removed after the active observations are cleared. > LayoutTests/resize-observer/resize-observer-should-not-leak-observed-nodes.html:37 > +function createIntersectionObserver() Do you mean createResizeObserver here?

Created attachment 433234 [details] Fixed the test

(In reply to cathiechen from comment #8) > Comment on attachment 433204 [details] > Fixes the bug > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433204&action=review > > Hi Ryosuke, > Thanks for working on this! > > > Source/WebCore/ChangeLog:13 > > + ResizeObserver and IntersectionObserver may get new observation entries queued later. > > Hmmm, I don't quite understand here. I think it seems right to keep > ResizeObserver and IntersectionObserver's JS wrapper objects if there are > observed elements. That's not right quite. We need to keep ResizeObserver & IntersectionObserver so long as their respective observed nodes are themselves kept alive. Consider the scenario in which ResizeObserver R_1 observes an element E_1. Note that this relationship never ends unless the observation of E_1 is explicitly cleared for R_1, which may never happen. R_1 can't just stop observing E_1 when E_1 is disconnected from a document because it can be connected to a document later. But when can it be inserted back? It could be 10ms from now, or 10 million years from now. We never know. The correct way to think of the lifetime of ResizeObserver & IntersectionObserver is that they must be kept alive so long as their observed elements are also kept alive. The way we achieve this condition is by treating those observed elements's opaque roots as opaque roots of ResizeObserver & IntersectionObserver. > Not sure if I understand it correctly. > Since now only the elements inside the active observations using > GCReachableRef, so gc is able to remove the other elements. That's right. This is precisely what this bug is about. We need to be able to GC the observed elements of ResizeObserver & IntersectionObserver if they're otherwise inaccessible to the script. The easiest way to think of the GC model is that the observed nodes and ResizeObserver are in the same strongly connected component in the GC heap. Either all of them will get collected at the same time, or none of them will get collected. This is so because if ResizeObserver exists without any active observations, there is no way for scripts to gain access to nodes it's observing. So the only way ResizeObserver must be kept is when JS has access to ResizeObserver itself or some of the observed nodes are kept alive by an external reference (e.g. JS reference or maybe it's connected to an active document). > As to the elements inside the active observations, they should be removed > after the active observations are cleared. Right. For active observations, the situation is a bit different. Because active observation is a mechanism by which observed nodes can be re-exposed to scripts at a later time, we must make sure such nodes' JS wrappers are kept alive even if it's not a part of any document and there is JS reference to it. In this case, ResizeObserver is the one that must keep its JS wrapper alive. i.e. ResizeObserver conceptually / semantically acts as an opaque root of these nodes. > > LayoutTests/resize-observer/resize-observer-should-not-leak-observed-nodes.html:37 > > +function createIntersectionObserver() > > Do you mean createResizeObserver here? Fixed.

Comment on attachment 433234 [details] Fixed the test View in context: https://bugs.webkit.org/attachment.cgi?id=433234&action=review > Source/WebCore/page/IntersectionObserver.cpp:129 > + ASSERT(is<Element>(root)); nit: This assertion is not needed since downcast<>() below will do the same check. > Source/WebCore/page/IntersectionObserver.cpp:144 > + ASSERT(is<Element>(root)); ditto. > Source/WebCore/page/IntersectionObserver.cpp:292 > + for (auto& target : m_observationTargets) { How is this thread safe? Cannot m_observationTargets be modified on the main thread while we're iterating over it here on the GC thread? > Source/WebCore/page/ResizeObserver.cpp:137 > + for (auto& observation : m_observations) { Same comment about thread safety.

(In reply to Chris Dumez from comment #11) > Comment on attachment 433234 [details] > Fixed the test > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433234&action=review > > > Source/WebCore/page/IntersectionObserver.cpp:129 > > + ASSERT(is<Element>(root)); > > nit: This assertion is not needed since downcast<>() below will do the same > check. Sure, will remove. > > Source/WebCore/page/IntersectionObserver.cpp:144 > > + ASSERT(is<Element>(root)); > > ditto. Done. > > Source/WebCore/page/IntersectionObserver.cpp:292 > > + for (auto& target : m_observationTargets) { > > How is this thread safe? > > Cannot m_observationTargets be modified on the main thread while we're > iterating over it here on the GC thread? This is safe because isReachableFromOpaqueRoots runs while the main thread is paused. So it could run in a GC thread but not while the main thread is running. > > Source/WebCore/page/ResizeObserver.cpp:137 > > + for (auto& observation : m_observations) { > > Same comment about thread safety. Ditto.

Created attachment 433249 [details] Patch for landing

Committed r279800 (239565@main): <https://commits.webkit.org/239565@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 433249 [details].

(In reply to Ryosuke Niwa from comment #10) > That's not right quite. We need to keep ResizeObserver & > IntersectionObserver so long as their respective observed nodes are > themselves kept alive. Consider the scenario in which ResizeObserver R_1 > observes an element E_1. Note that this relationship never ends unless the > observation of E_1 is explicitly cleared for R_1, which may never happen. > R_1 can't just stop observing E_1 when E_1 is disconnected from a document > because it can be connected to a document later. But when can it be inserted > back? It could be 10ms from now, or 10 million years from now. We never know. > I see... In the test case, the element is passed to setTimeout() and is not cleared explicitly. And it can not be gc somehow which causes ResizeObserver & IntersectionObserver leak. Thanks for the explanation!

(In reply to cathiechen from comment #15) > (In reply to Ryosuke Niwa from comment #10) > > That's not right quite. We need to keep ResizeObserver & > > IntersectionObserver so long as their respective observed nodes are > > themselves kept alive. Consider the scenario in which ResizeObserver R_1 > > observes an element E_1. Note that this relationship never ends unless the > > observation of E_1 is explicitly cleared for R_1, which may never happen. > > R_1 can't just stop observing E_1 when E_1 is disconnected from a document > > because it can be connected to a document later. But when can it be inserted > > back? It could be 10ms from now, or 10 million years from now. We never know. > > > > I see... > In the test case, the element is passed to setTimeout() and is not cleared > explicitly. And it can not be gc somehow which causes ResizeObserver & > IntersectionObserver leak. Right. To be precise here, the issue is that a JS function which is used as a callback of ResizeObserver captures the element (because it's in the outer scope). Since ResizeObserver is kept alive as it has an observed element (i.e. JSResizeObserver::isReachableFromOpaqueRoots will return true), this JS callback is also kept alive (via JSResizeObserver::visitAdditionalChildren). As more general rule, GCReachableRef and ActiveDOMObject aren't appropriate mechanism to keep objects alive unless there is a known time limit.

Comment on attachment 433249 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=433249&action=review > Source/WebCore/ChangeLog:17 > + would use their respective observed nodes as opaque roots. i.e. so long as any of the observed I am investigating rdar://81057759. I see that IntersectionObserver maintains a queue of pending entries (m_queuedEntries). What makes sure that the IntersectionObserver wrapper stays alive after its observed node has been collected but there are still queued entries that will be dispatched later on? We seem to have a wrapper lifetime bug and this is one possible explanation for it (which may or may not be correct).

(In reply to Chris Dumez from comment #17) > Comment on attachment 433249 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=433249&action=review > > > Source/WebCore/ChangeLog:17 > > + would use their respective observed nodes as opaque roots. i.e. so long as any of the observed > > I am investigating rdar://81057759. I see that IntersectionObserver > maintains a queue of pending entries (m_queuedEntries). What makes sure that > the IntersectionObserver wrapper stays alive after its observed node has > been collected but there are still queued entries that will be dispatched > later on? > > We seem to have a wrapper lifetime bug and this is one possible explanation > for it (which may or may not be correct). Hm... IntersectionObserver::isReachableFromOpaqueRoots returns true if any of the observation target (m_observationTargets) is still reachable. And any element that was added to m_queuedEntries is also added to m_pendingTargets which keeps its wrapper alive with GCReachableRef so in theory, it should be fine.