Guidance on best way to port a WTF::BindOnce() call passing a WeakPtr to be passed a GC-d object

[Colin Blundell]

Hi folks,

We're in the process of eliminating Canvas2DLayerBridge. As part of this we are planning on porting its PageVisibilityChanged method to live directly in that method's caller in CanvasRenderingContext2D. However, I have a question wrt this port:

• Canvas2DLayerBridge passes itself as a WeakPtr in a WTF::BindOnce() call
• CanvasHTMLElement holds Canvas2DLayerBridge as an std::unique_ptr but CanvasRenderingContext2D as a GC'd member.
• HTMLCanvasElement detaches itself as host on CanvasRenderingContext2D at the same time as it destroys Canvas2DLayerBridge. CanvasRenderingContext2D can check whether the host is null.
• As I don't have familiarity with WTF::Bind() and in particular its interaction with GC'd objects, what would be the recommendation for doing this port?

Naively, I thought of either (1) passing a strong reference in the bind and then short-circuiting out if the host is null, or (2) having CanvasRenderingContext2D have a small helper object that it passes a WeakPtr to in the bind and destroys on host detachment. (2) is more clunky but has the advantage of not potentially extending the lifetime of the CanvasRenderingContext2D instance. If there are better/more idiomatic solutions, I'd love to hear them!

Thanks,

Colin

[Kentaro Hara]

If I understand your description correctly, (1) will work.

However...

• HTMLCanvasElement detaches itself as host on CanvasRenderingContext2D at the same time as it destroys Canvas2DLayerBridge. CanvasRenderingContext2D can check whether the host is null.

I don't quite understand why this logic is needed. Note that HTMLCanvasElement::Dispose() is called in its pre-finalizer (i.e., when HTMLCanvasElement becomes unreachable). At that point, CanvasRenderingContext2D should be unreachable too. There's no need to clear CanvasRenderingContext2D::host_.

IIUC HTMLCanvasElement and CanvasRenderingContext2D live and die together. There is no need to clear strong references between them when HTMLCanvasElement is about to be finalized.

--
You received this message because you are subscribed to the Google Groups "platform-architecture-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to platform-architect...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAMGE5NET2KV7zHYYxBKj0y_afF994hq0VBV%2BJc-8nTHt_ncLZQ%40mail.gmail.com.

--

Kentaro Hara, Tokyo

[Omer Katz]

I believe you could use WeakCell (which the GCed counterpart to WeakPtr). Then you don't need a helper object afaict for option 2.

Following up on Kentaro's comment, prefinalizer should definitely not reset references in other unreachable objects, since both go away anytime.

Preferably we would have as few prefinalizer as possible.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CABg10jxHHJtw6H6nLxK3PXFZFB1xnRANOptf9y4ebr26OW31wQ%40mail.gmail.com.

--

Omer Katz

V8, Software Engineer

Google Germany GmbH

Erika-Mann-Straße 33

80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise erhalten haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter, löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen, dass die E-Mail an die falsche Person gesendet wurde. 

     

This e-mail is confidential. If you received this communication by mistake, please don't forward it to anyone else, please erase all copies and attachments, and please let me know that it has gone to the wrong person.

[Dave Tapuska]

I'd think you just want  WrapWeakPersistent no? But yes if you need the invalidation behavior (of WeakPtrFactory) you need to use WeakCell. It isn't clear whether you'll need that in some type of dispose, but I imagine not.

dave.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/platform-architecture-dev/CAJFGqfOC5pXHRhgOAbiLOHm5ShFzqkf2ipYB2cRd5wBn-tVoBw%40mail.gmail.com.

[Kentaro Hara]

My bet is that HTMLCanvasElement and CanvasRenderingContext2D live and die together and thus the weak processing is not needed :)

--

Kentaro Hara, Tokyo

[Dave Tapuska]

Ya but the task is posted as an idle task runner and is thread bound, so it needs some type of weak reference because it is not cancelable. Perhaps I'm misunderstanding though. I pending CL is probably best to comment on if there is one.

dave.

[Colin Blundell]

Hi folks,

Thanks for your responses so far!

I don't know anything about the expected lifetime relationship between HTMLCanvasElement and CanvasRenderingContext2D, and hence have been operating on the assumption that CanvasRenderingContext2D could stay alive beyond HTMLCanvasElement clearing its reference.

My inclination is to use WeakCell for this port (thanks for the reference!) and separate out the task of eliminating Canvas2DLayerBridge from any other cleanup of lifetime assumptions that this code is making or not making about the relationship between HTMLCanvasElement and CanvasRenderingContext2D. See my response to Dave's point below as well, which is relevant to the need for a WeakPtr-like object here.

That said, if you folks see anything that would (a) be clearly a correct change to make and (b) make the port of HibernateOrLogFailure() simpler, please let me know!

On Tue, Jun 18, 2024 at 4:11 PM Dave Tapuska <dtap...@chromium.org> wrote:

Ya but the task is posted as an idle task runner and is thread bound, so it needs some type of weak reference because it is not cancelable. Perhaps I'm misunderstanding though. I pending CL is probably best to comment on if there is one.

I don't have a CL together at this point, but the task is indeed not cancelable and in fact has logic for the case where the weak pointer has gone away.

[Omer Katz]

It may be worth investing a bit more into understanding the lifetimes before going ahead with the migration.

Prefinalizers specifically are performance footguns and have a tendency to stick around once they're in the code base.

If we could get rid of it now instead of leaving it behind for a future project (that may get delayed/reprioritized), I think that could be preferable.

[Colin Blundell]

(from right address)

I'm absolutely happy to do any simplification there that the group determines is possible, but if it's (a) at all risky in terms of potentially needing rollback and (b) orthogonal to the migration, I would do it immediately after the migration just to decouple concerns.

Best,

Colin

[Kentaro Hara]

I think that a straightforward fix is to keep all the logic in HTMLCanvasElement::Dispose() as is and post an idle task with WTF::Bind(..., WrapWeakPersistent(context_)). When the task gets called, you can bail out early if host_ is null.

In a follow-up CL, you can try to remove the clearing logic in HTMLCanvasElement::Dispose() and the host_ check.

Happy to review the CLs :)

--

Kentaro Hara, Tokyo

[Colin Blundell]

Thanks!

On Thu, Jun 20, 2024 at 6:35 PM Kentaro Hara <har...@chromium.org> wrote:

I think that a straightforward fix is to keep all the logic in HTMLCanvasElement::Dispose() as is and post an idle task with WTF::Bind(..., WrapWeakPersistent(context_)). When the task gets called, you can bail out early if host_ is null.

What exactly are the semantics of WrapWeakPersistent()?

[Dave Tapuska]

It's a persistent weak ptr. So that means it is a weak member (V8 term) that is stored in a global table by V8 to trace. As opposed to being reached by another object during tracing. It is valid really only for the thread that you are on executing on. 

Dave

[Omer Katz]

To add/correct to what Dave wrote:

WrapWeakPersistent creates a WeakPersistent which is an Oilpan primitive.

String Persistents are references that are registered with Oilpan (and kept in Oilpan's internal data structures). Oilpan treats such references as roots, and traces them without needing to reach them from another object.

A Weak Persistent differs in that it does not keep an object alive. Like other weak references, it is ignored by the GC when deciding reachability of objects, so an object held from a WeakPersistent may be reclaimed by the GC. The WeakPersistent will be reset to nullptr in such cases.

(Weak)Persistent are meant to be used for keeping references to a GCed objects from the non-GCed heap (e.g. PA) and are only valid on the thread that created them (i.e. do not use (Weak)Persistent for a cross-thread use case).

[Colin Blundell]

(from the right address)

Awesome, as an Oilpan newbie this is very helpful!