Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ResizeObserver] The initial last reported size of ResizeObservation should be -1x-1 which always notify observed elements #9112

Conversation

cathiechen
Copy link
Contributor

@cathiechen cathiechen commented Jan 25, 2023

2861d8c

[ResizeObserver] The initial last reported size of ResizeObservation should be -1x-1 which always notify observed elements
https://bugs.webkit.org/show_bug.cgi?id=251015

Reviewed by Oriol Brufau and Ryosuke Niwa.

This patch initializes the lastReportedSize of ResizeObservation to -1 x -1, which is the conclusion of [1],
it indicates that -1 x -1 is the initial size when first observing an element with ResizeObserver.

The previous change makes every ResizeObservation be delivered at least once, even if the target is a disconnected element.
When GC happens between observe and gather ResizeObservations, JSC::Weak<JSC::JSObject> m_callback inside JSCallbackDataWeak
could be released, and the element and JSResizeObserver might not be released, for they are not strong connected, this scenario
would trigger the ASSERT in ResizeObserver::deliverObservations. To fix it, this patch adds m_targetsWaitingForFirstObservation
to extend the life cycle of ResizeObserver to the first time of deliverObservations. The fix is similar to
what IntersectionObserver does in bug 231235.

The test6 of resize-observer/notify.html still fails in mac-wk1, because updaterendering is not triggered properly.
Filed [2] to track it.

[1] w3c/csswg-drafts#3664
[2] https://bugs.webkit.org/show_bug.cgi?id=250900

* LayoutTests/imported/w3c/web-platform-tests/resize-observer/notify-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/resize-observer/observe-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/resize-observer/svg-expected.txt:
* LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/resize-observer/notify-expected.txt: Copied from LayoutTests/imported/w3c/web-platform-tests/resize-observer/notify-expected.txt.
* LayoutTests/platform/mac/imported/w3c/web-platform-tests/resize-observer/svg-expected.txt:
* Source/WebCore/page/ResizeObservation.cpp:
* Source/WebCore/page/ResizeObserver.cpp:
(WebCore::ResizeObserver::observe):
(WebCore::ResizeObserver::deliverObservations):
(WebCore::ResizeObserver::isReachableFromOpaqueRoots const):
(WebCore::ResizeObserver::removeAllTargets):
(WebCore::ResizeObserver::removeObservation):
* Source/WebCore/page/ResizeObserver.h:

Canonical link: https://commits.webkit.org/259673@main

1408ba6

Misc iOS, tvOS & watchOS macOS Linux Windows
❌ πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  πŸ§ͺ win
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ›  gtk   πŸ›  wincairo
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ gtk-wk2
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1 βœ… πŸ§ͺ api-gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim

@cathiechen cathiechen self-assigned this Jan 25, 2023
@cathiechen cathiechen added the DOM For bugs specific to XML/HTML DOM elements (including parsing). label Jan 25, 2023
@cathiechen
Copy link
Contributor Author

cathiechen commented Jan 25, 2023

Hi, this patch is to address the failure found by #8839.
There is a similar patch for IntersectionObserver to address this kind of issue, see https://bugs.webkit.org/show_bug.cgi?id=231235.
And this patch fails LayoutTests/resize-observer/resize-observer-should-not-leak-observed-nodes.html, it is not ready for review yet.
But I want to start this thread to discuss about it, for I spend several days debugging, and there is some part still confusing me.

We need to address the crash issue in

<script>
  onload = async () => {
    for (let i = 0; i < 8; i++) {
      document.createElement('div');
    }
    for (let i = 0; i < 8; i++) {
      new ResizeObserver(() => {});
    }
    let resizeObserver = new ResizeObserver(() => {});
    await resizeObserver.observe(document.createElement('div'));
    new ImageData(1000, 10000);
  };
</script>

The direct reason for this crash issue is that: when calling ResizeObserver::deliverObservations for the last resizeObserver, the JSC::Weak<JSC::JSObject> m_callback inside JSCallbackDataWeak has been released.
Checking the code, there are two ways to keep m_callback alive:

  1. JSCallbackDataWeak::visitJSFunction, it appends m_callback to visitor. This is called from JSResizeObserver::visitAdditionalChildren. This path won't be triggered in the example, for there is nobody holding JSResizeObserver.
  2. JSCallbackDataWeak::WeakOwner::isReachableFromOpaqueRoots, which calls visitor.containsOpaqueRoot(context), context is JSResizeObserverCallback, which is owned by ResizeObserver, it could not be inside visitor?(Not sure if I understand this correctly)
    Both return false, so GC released m_callback.

Meanwhile, JSResizeObserver and the target element are alive, they are released after ResizeObserver::deliverObservations. I'm confused by this part.
From the logs I can see, JSResizeObserver is alive unless the target element got released. But I don't see a strong binding between them:

When ResizeObserver::observe, target calls ensureResizeObserverData, but ResizeObserverData is a WeakPtr to ResizeObserver.

struct ResizeObserverData {
    WTF_MAKE_STRUCT_FAST_ALLOCATED;
    Vector<WeakPtr<ResizeObserver>> observers;
};

And ResizeObserver would own the ResizeObservation, but m_target in ResizeObservation is also WeakPtr, WeakPtr<Element, WeakPtrImplWithEventTargetData> m_target;.

So the target and ResizeObserver are connected by WeakPtr. But why ResizeObserver is always released after the target. And why both of them won't be released before ResizeObserver::deliverObservations?

I must misunderstand something. Would someone explain me a little bit how does GC work in this scenario? Thanks!

Source/WebCore/page/ResizeObserver.cpp Show resolved Hide resolved
@@ -141,6 +143,8 @@ void ResizeObserver::deliverObservations()
m_activeObservations.clear();
auto activeObservationTargets = std::exchange(m_activeObservationTargets, { });

std::exchange(m_targetsWaitingForFirstObservation, { });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert in these things, but my worry is that, unlike IntersectionObserver::notify, you might not be keeping the old value alive by storing it into a variable.

Could it then be possible for the callbacks to be garbage collected before m_callback->handleEvent(*this, entries, *this);? I will defer the review to someone who knows better about these things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we should clear this after calling m_callback->handleEvent. Maybe use ScopeExit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thanks!

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 25, 2023
@cathiechen
Copy link
Contributor Author

LayoutTests/resize-observer/resize-observer-should-not-leak-observed-nodes.html fails, because we need to make sure every resizeobserver should be delivered once, otherwise it would postpone GC of elements. To avoid the fake failure in EWS, I will merge the two patches together. #8839

@cathiechen cathiechen removed the merging-blocked Applied to prevent a change from being merged label Jan 26, 2023
@cathiechen cathiechen force-pushed the eng/ResizeObserver-ASSERTm_callback-hasCallback-under-ResizeObserverdeliverObservations branch from 075d7ac to 5766a52 Compare January 26, 2023 18:20
@cathiechen cathiechen force-pushed the eng/ResizeObserver-ASSERTm_callback-hasCallback-under-ResizeObserverdeliverObservations branch from 5766a52 to 72c9198 Compare January 26, 2023 18:27
@cathiechen
Copy link
Contributor Author

Thanks for the review!
But could somebody explain a bit more about before this patch how GC works when ResizeObserver observing a disconnected target? This part still confusing me.
After await resizeObserver.observe(document.createElement('div'));, looks like there is nobody holds resizeObserver or the div element, but both of them are alive after GC, and they won't be released before deliverResizeObservation.
And resizeObserver is released after the div element, but they are connected by WeakPtr.
These two behaviors look totally reasonable to me, but I just can't find where in the code to make sure of it.

Copy link
Contributor

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are merging this with #8839, then I think you should use the title and bug number from that one.

@@ -141,6 +144,10 @@ void ResizeObserver::deliverObservations()
m_activeObservations.clear();
auto activeObservationTargets = std::exchange(m_activeObservationTargets, { });

auto scopeExit = makeScopeExit([&] {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understand makeScopeExit. It seems that it will clear m_targetsWaitingForFirstObservation once scopeExit is destroyed, which I guess it's at the end of the function? But then m_targetsWaitingForFirstObservation will be cleared after m_callback->handleEvent, which seems bad, because the callbacks could be observing new elements that will be appended into m_targetsWaitingForFirstObservation and shouldn't be cleared yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yes, that is possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we std::exchange it to a local variable instead? Then the list still keeps object alive during the duration of this function, and it goes away at the end of function.

targetsWaitingForFirstObservation = std::exchange(m_targetsWaitingForFirstObservation);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, like for IntersectionObserver

auto targetsWaitingForFirstObservation = std::exchange(m_targetsWaitingForFirstObservation, { });

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 26, 2023
@cathiechen cathiechen removed the merging-blocked Applied to prevent a change from being merged label Jan 30, 2023
@cathiechen cathiechen force-pushed the eng/ResizeObserver-ASSERTm_callback-hasCallback-under-ResizeObserverdeliverObservations branch from 72c9198 to 3a7305d Compare January 30, 2023 18:17
@cathiechen cathiechen force-pushed the eng/ResizeObserver-ASSERTm_callback-hasCallback-under-ResizeObserverdeliverObservations branch from 3a7305d to 5a3d673 Compare January 30, 2023 18:27
Copy link
Member

@rniwa rniwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jan 31, 2023
Copy link
Contributor

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to address the failure in the style bot:

ERROR: Source/WebCore/page/ResizeObservation.cpp:44: Code inside a namespace should not be indented. [whitespace/indent] [4]

@cathiechen
Copy link
Contributor Author

You need to address the failure in the style bot:

ERROR: Source/WebCore/page/ResizeObservation.cpp:44: Code inside a namespace should not be indented. [whitespace/indent] [4]

Hmm, I don't quite understand the style error. Maybe the uniform initialization is treated as the function body?

Copy link
Contributor

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assert isn't currently failing, so I think the commit title should be about the -1x-1 to always notify observed elements.

Copy link
Contributor

@Loirooriol Loirooriol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with the changes in the commit message

@cathiechen cathiechen removed the merging-blocked Applied to prevent a change from being merged label Jan 31, 2023
@cathiechen cathiechen changed the title [ResizeObserver] ASSERT(m_callback->hasCallback()) under ResizeObserver::deliverObservations [ResizeObserver] The initial last reported size of ResizeObservation should be -1x-1 which always notify observed elements Jan 31, 2023
@cathiechen cathiechen force-pushed the eng/ResizeObserver-ASSERTm_callback-hasCallback-under-ResizeObserverdeliverObservations branch from 5a3d673 to 1408ba6 Compare January 31, 2023 20:09
@cathiechen
Copy link
Contributor Author

cathiechen commented Jan 31, 2023

The assert isn't currently failing, so I think the commit title should be about the -1x-1 to always notify observed elements.

Done, thanks! I just updated the commit message, not the bug URL. The discussion here explains the reason we merge the two patches together, so I prefer to upload it in this PR.

@cathiechen
Copy link
Contributor Author

I think I kinda figured this out \o/

Hi, this patch is to address the failure found by #8839. There is a similar patch for IntersectionObserver to address this kind of issue, see https://bugs.webkit.org/show_bug.cgi?id=231235. And this patch fails LayoutTests/resize-observer/resize-observer-should-not-leak-observed-nodes.html, it is not ready for review yet. But I want to start this thread to discuss about it, for I spend several days debugging, and there is some part still confusing me.

We need to address the crash issue in

<script>
  onload = async () => {
    for (let i = 0; i < 8; i++) {
      document.createElement('div');
    }
    for (let i = 0; i < 8; i++) {
      new ResizeObserver(() => {});
    }
    let resizeObserver = new ResizeObserver(() => {});
    await resizeObserver.observe(document.createElement('div'));
    new ImageData(1000, 10000);
  };
</script>

The direct reason for this crash issue is that: when calling ResizeObserver::deliverObservations for the last resizeObserver, the JSC::Weak<JSC::JSObject> m_callback inside JSCallbackDataWeak has been released. Checking the code, there are two ways to keep m_callback alive:

  1. JSCallbackDataWeak::visitJSFunction, it appends m_callback to visitor. This is called from JSResizeObserver::visitAdditionalChildren. This path won't be triggered in the example, for there is nobody holding JSResizeObserver.
  2. JSCallbackDataWeak::WeakOwner::isReachableFromOpaqueRoots, which calls visitor.containsOpaqueRoot(context), context is JSResizeObserverCallback, which is owned by ResizeObserver, it could not be inside visitor?(Not sure if I understand this correctly)
    Both return false, so GC released m_callback.

Meanwhile, JSResizeObserver and the target element are alive, they are released after ResizeObserver::deliverObservations. I'm confused by this part. From the logs I can see, JSResizeObserver is alive unless the target element got released.

This is not always true. In the example, there are 9 ResizeObservers, and it's the ninth one not released.
If we change the number to 8 or 7, the last ResizeObserver and the target could be released like others.
So it's not observe() that stops releasing ResizeObserver or target. I guess it's the runtime of GC decides.

But I don't see a strong binding between them:

When ResizeObserver::observe, target calls ensureResizeObserverData, but ResizeObserverData is a WeakPtr to ResizeObserver.

struct ResizeObserverData {
    WTF_MAKE_STRUCT_FAST_ALLOCATED;
    Vector<WeakPtr<ResizeObserver>> observers;
};

And ResizeObserver would own the ResizeObservation, but m_target in ResizeObservation is also WeakPtr, WeakPtr<Element, WeakPtrImplWithEventTargetData> m_target;.

So the target and ResizeObserver are connected by WeakPtr. But why ResizeObserver is always released after the target.

This is not true either, ResizeObserver could be released after if change the number of RO.
So they are real weak connected;)

@cathiechen cathiechen added the merge-queue Applied to send a pull request to merge-queue label Feb 1, 2023
…should be -1x-1 which always notify observed elements

https://bugs.webkit.org/show_bug.cgi?id=251015

Reviewed by Oriol Brufau and Ryosuke Niwa.

This patch initializes the lastReportedSize of ResizeObservation to -1 x -1, which is the conclusion of [1],
it indicates that -1 x -1 is the initial size when first observing an element with ResizeObserver.

The previous change makes every ResizeObservation be delivered at least once, even if the target is a disconnected element.
When GC happens between observe and gather ResizeObservations, JSC::Weak<JSC::JSObject> m_callback inside JSCallbackDataWeak
could be released, and the element and JSResizeObserver might not be released, for they are not strong connected, this scenario
would trigger the ASSERT in ResizeObserver::deliverObservations. To fix it, this patch adds m_targetsWaitingForFirstObservation
to extend the life cycle of ResizeObserver to the first time of deliverObservations. The fix is similar to
what IntersectionObserver does in bug 231235.

The test6 of resize-observer/notify.html still fails in mac-wk1, because updaterendering is not triggered properly.
Filed [2] to track it.

[1] w3c/csswg-drafts#3664
[2] https://bugs.webkit.org/show_bug.cgi?id=250900

* LayoutTests/imported/w3c/web-platform-tests/resize-observer/notify-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/resize-observer/observe-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/resize-observer/svg-expected.txt:
* LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/resize-observer/notify-expected.txt: Copied from LayoutTests/imported/w3c/web-platform-tests/resize-observer/notify-expected.txt.
* LayoutTests/platform/mac/imported/w3c/web-platform-tests/resize-observer/svg-expected.txt:
* Source/WebCore/page/ResizeObservation.cpp:
* Source/WebCore/page/ResizeObserver.cpp:
(WebCore::ResizeObserver::observe):
(WebCore::ResizeObserver::deliverObservations):
(WebCore::ResizeObserver::isReachableFromOpaqueRoots const):
(WebCore::ResizeObserver::removeAllTargets):
(WebCore::ResizeObserver::removeObservation):
* Source/WebCore/page/ResizeObserver.h:

Canonical link: https://commits.webkit.org/259673@main
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/ResizeObserver-ASSERTm_callback-hasCallback-under-ResizeObserverdeliverObservations branch from 1408ba6 to 2861d8c Compare February 1, 2023 10:51
@webkit-commit-queue
Copy link
Collaborator

Committed 259673@main (2861d8c): https://commits.webkit.org/259673@main

Reviewed commits have been landed. Closing PR #9112 and removing active labels.

@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Feb 1, 2023
@webkit-early-warning-system webkit-early-warning-system merged commit 2861d8c into WebKit:main Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DOM For bugs specific to XML/HTML DOM elements (including parsing).
Projects
None yet
6 participants