LibWeb: PointerEvent improvements #1854
Conversation
gmta
force-pushed
the
libweb-uievents-pointerevent-improvements
branch
from
8f2f744
to
ac48ea7
Compare
| // https://w3c.github.io/pointerevents/#dom-pointerevent-getcoalescedevents | ||
| AK::Vector<JS::Handle<PointerEvent>> m_coalesced_events; | ||
|
|
||
| // https://w3c.github.io/pointerevents/#dom-pointerevent-getpredictedevents | ||
| AK::Vector<JS::Handle<PointerEvent>> m_predicted_events; |
There was a problem hiding this comment.
please change these two vectors to be visited instead of using JS::Handle. we generally try to visit whenever it's possible, because using JS::Handle, especially when it's owned by another GC-allocated object will cause a leak.
There was a problem hiding this comment.
On the flip side, don't we also try and avoid exposing public getters which return a Vector<NonnullGCPtr> since those GCPtrs if copied by a caller are not tracked by the GC since the pointer on the heap and not the stack?
Not suggesting any change at all - but it feels like either approach does have a downside. Do we typically use a MarkedVector in that scenario or something?
There was a problem hiding this comment.
On the flip side, don't we also try and avoid exposing public getters which return a Vector since those GCPtrs if copied by a caller are not tracked by the GC since the pointer on the heap and not the stack?
If caller wants a copy of vector then it should be responsible for visiting it. JS::Handle creates a strong root and we want to limit its usage only to the cases when GC-allocated object is owned by non GC-allocated object.
Not suggesting any change at all - but it feels like either approach does have a downside. Do we typically use a MarkedVector in that scenario or something?
MarkedVector same as JS::Handle creates a strong root. we never want to have GC-allocated object that own a strong root, because 99% it will lead to GC-allocated caused by objects visiting each other.
There was a problem hiding this comment.
If caller wants a copy of vector then it should be responsible for visiting it.
The problem with this is when GC occurs between the caller getting this vector and having a chance to store it, which is the case throughout our generated prototypes.
So as currently written, this getter is unsafe:
AK::Vector<JS::NonnullGCPtr<PointerEvent>> get_coalesced_events() const { return m_coalesced_events; }Because the caller in the generated prototype looks like:
auto retval = TRY(throw_dom_exception_if_needed(vm, [&] { return impl->get_coalesced_events(); }));
auto new_array0 = MUST(JS::Array::create(realm, 0));If GC is triggered during Array::create, the returned vector is not protected. I think even if we return a const& from get_coalesced_events, the declaration of auto retval will actually create a copy.
It's safest/necessary to do one of two things:
- Store as a
Vector<NNGCPtr>as the class member, and returnVector<Handle>orMarkedVectorfrom this getter. This is what we usually do, and is safe, but is unnecessarily expensive, so... - Return
ReadonlySpan<NNGCPtr>from this getter.
There was a problem hiding this comment.
The problem with this is when GC occurs between the caller getting this vector and having a chance to store it, which is the case throughout our generated prototypes.
right, that's what I started thinking about once I posted this message 😅 definitely something I should revisit on the next round of GC-leaks fixing.
It's safest/necessary to do one of two things:
both options seems good, thanks for suggestion!
There was a problem hiding this comment.
Went with ReadonlySpan<NNGCPtr>!
gmta
force-pushed
the
libweb-uievents-pointerevent-improvements
branch
2 times, most recently
from
394a195
to
1c00040
Compare
Fixes at least 2 WPT subtests in /pointerevents.
Fixes at least 4 WPT subtests in /pointerevents.
gmta
force-pushed
the
libweb-uievents-pointerevent-improvements
branch
from
1c00040
to
92f0f36
Compare
| m_coalesced_events.unchecked_append(static_cast<PointerEvent&>(*coalesced_event)); | ||
|
|
||
| m_predicted_events.ensure_capacity(event_init.predicted_events.size()); | ||
| for (auto const& predicted_event : event_init.predicted_events) | ||
| m_predicted_events.unchecked_append(static_cast<PointerEvent&>(*predicted_event)); |
There was a problem hiding this comment.
Ah, sorry, was just about to merge this, but noticed these casts on a last look - they don't seem necessary? Compiles without the casts for me
Fixes at least 6 WPT subtests. :^)