1285377 - Figure out what we should do with CustomAutoRooter in DOM code

Status: NEW

(Core :: DOM: Core & HTML, defect, P3)

Description

[Boris Zbarsky [:bzbarsky]]

Should we try to switch to Rooted?  It's a bit unclear to me how to make that work for RootedDictionary and its direct member access (because it's just a struct!) but maybe we'd just end up converting consumers to use some sort of get() on it to poke the members.  We'd need conversion operators to the dictionary types, and similar for RootedTypedArray.

And we'd need to think a bit about how SequenceRooter should work.

Some of this stuff may need to change all at once, depending on how similar the APIs are (e.g. whether a dictionary trace() method as needed for Rooted can be called from a SequenceRooter and vice versa).

Comment 1

[Andrew Overholt [:overholt]]

P3 = backlog but feel free to disagree as always.

Comment 2

[Steve Fink [:sfink] [:s:]]

Generally we like to use Rooted for everything, and it would be sort of nice to eliminate CustomAutoRooter entirely just so we don't have to think about it and can delete some code, but I'm not really feeling this is a huge pain point. The analysis understands inheritance pretty well these days, and the rooting happens in a self-contained type (not as an external rooter that lives separately on the stack). So I'm not feeling a lot of urgency behind this.

For the record, bz explained to me on IRC that the blocker to using Rooted is that currently you can do

RootedDictionary<MyStruct> d;
d.foo = 5; // Where foo is a data member of MyStruct.

Rooted cannot do that until C++ gains support for operator.(). Which it might; see eg https://isocpp.org/blog/2016/02/a-bit-of-background-for-the-operator-dot-proposal-bjarne-stroustrup though I don't know the current status.

Or with some work this could be d->SetFoo(5). I don't know if that's enough; you might also need things like &d.foo to work?

Honestly, RootedDictionary seems kinda nice. I suppose it has a vtable that might not otherwise be necessary.

Comment 3

[Steve Fink [:sfink] [:s:]]

This has come up again. An additional argument in favor of switching to Rooted is so that callees can be marked as taking Handles, and thereby require the caller to root. The analysis can be improved to flag unrooted cases, but Handle would enable catching this at compile time. Example:

  Request::Constructor(RequestInit());

which should be

  RootedDictionary<RequestInit> init(aCx);
  Request::Constructor(init);

Currently, Request::Constructor will be accept a const RequestInit& and so both versions will compile.

Comment 4

[Steve Fink [:sfink] [:s:]]

The work in bug 1729602 enables eg Rooted<RequestInit>, at least if you update the codegen to generate a trace() method as an exact clone of TraceDictionary (or a call to TraceDictionary). It does not magically make d.foo = 5 work. Or &d.foo, but the latter is a good thing -- it should not be that easy to create unrooted pointers to the interior of rooted structs.

So to switch to Rooted after that bug lands, we'd still need to switch to using .get() or setters or something.