1534967 - Replace AutoIdVector with RootedIdVector

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement, P3)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

In Bug 1521732 I'll land RootedVector, and convert AutoValueVector to RootedValueVector, and same for AutoObjectVector.
However I found there are too many occurrences of AutoIdVector, I'll split this as a separate bug.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

XPIDL will pass AutoIdVector&
https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/js/xpconnect/idl/nsIXPCScriptable.idl#82
so I have to pass RootedIdVector& for classes implementing this.

same for JSNewEnumerateOp
https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/js/public/Class.h#421

and JS_NewEnumerateStandardClasses(IncludingResolved)
https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/js/src/jsapi.h#654

However XrayWrapper::ownPropertyKeys
https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/js/xpconnect/wrappers/XrayWrapper.h#388
will call Enumerate
https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/js/xpconnect/wrappers/XrayWrapper.cpp#1731

that means ownPropertyKeys, or getPropertyKeys will have to use RootedIdVector&
https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/js/public/Proxy.h#277
https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/js/ipc/WrapperOwner.cpp#224

and ownPropertyKeys and getPropertyKeys are the two major callers of using AutoIdVector& or AutoIdVector*
that means for most cases we pass RootedIdVector&, instead of (Mutable)HandldIdVector,

Jonco, could you help to comment on this?
not sure passing ref is what we want here.

Thanks

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh] from comment #1)

XPIDL will pass AutoIdVector&
https://searchfox.org/mozilla-central/rev/2c912888e3b7ae4baf161d98d7a01434f31830aa/js/xpconnect/idl/nsIXPCScriptable.idl#82
so I have to pass RootedIdVector& for classes implementing this.

For XPIDL I could just pass MutableHandlIdVector, just use
native JSMutableHandleIdVector(JS::MutableHandleIdVector);

no need to use [ptr] or [ref].

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh] from comment #2)
If that works then great, lets' do that.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1534967 - Part 1: use RootedIdVector.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Jonco
I forgot to use RootedVector<jsid> outside js/, as some of patches are done before you reviewed bug 1521732.
I'll fix them in next version.
Feel free to let me know if I need to split these patches into different parts, although most changes are just one line change.

Comment 6

[Jon Coppeard (:jonco)]

Comment on attachment 9053876 [details]
Bug 1534967 - Part 1: use RootedIdVector.

This all looks good, except for the style changes outside /js as you noted above.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Jonco
One thing I am not sure about, now I am fixing failure in rust binding, and in
https://searchfox.org/mozilla-central/source/js/rust/tests/enumerate.rs#41

It will call GetPropertyKeys, which rust-bindgen has already changed the signature to (from jsapi_debug.rs)

            pub fn GetPropertyKeys(
                cx: *mut root::JSContext,
                obj: root::JS::HandleObject,
                flags: ::std::os::raw::c_uint,
                props: root::JS::MutableHandleIdVector,
            ) -> bool;

However AutoIdVector, RootedIdVector, MutabldHandleIdVector are all type of u8 in rust (from jsapi_debug.rs)

 pub type MutableHandleIdVector = u8;

This looks wrong...

Should I first try to generate a correct binding for *Vector first?

Comment 8

[Jon Coppeard (:jonco)]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh] from comment #7)
To make this code work the same as before I think this rust IdVector should now wrap a RootedIdVector rather than an AutoIdVector. But now that I look at it I'm not sure the existing code is safe.

Nick, IdVector ends up wrapping a JS::AutoIdVector which is implemented as a Rooted<> and needs to have stack lifetime. Is there something that guarantees that this happens? Fortunately this doesn't look like it's widely used.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1534967 - Part 2: Use RootedIdVector in rust binding.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh] from comment #9)

Created attachment 9054417 [details]
Bug 1534967: rust binding

This patch bypasses the compilation error(incorrectly, MutableHandleIdVector is still u8),

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

However AutoIdVector, RootedIdVector, MutabldHandleIdVector are all type of
u8 in rust (from jsapi_debug.rs)

pub type MutableHandleIdVector = u8;

This is a case where bindgen isn't smart enough to understand the template magic going on here. In these scenarios, you generally have to write a replacement type[0] that is ABI compatible with the original type that bindgen can't understand.

[0] https://rust-lang.github.io/rust-bindgen/replacing-types.html

Nick, IdVector ends up wrapping a JS::AutoIdVector which is implemented as
a Rooted<> and needs to have stack lifetime. Is there something that guarantees
that this happens?

Bindgen can't understand this restriction since it cannot be expressed in the C(++) types, so it is up to users to write unsafe code that wraps up the underlying types and FFI calls in a safe interface.

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #11)

This is a case where bindgen isn't smart enough to understand the template magic going on here. In these scenarios, you generally have to write a replacement type[0] that is ABI compatible with the original type that bindgen can't understand.

Thanks.

Bindgen can't understand this restriction since it cannot be expressed in the C(++) types, so it is up to users to write unsafe code that wraps up the underlying types and FFI calls in a safe interface.

You mean IdVector as the safe wrapper? I don't think it safe though because it doesn't enforce this restriction. It doesn't seem to be used outside of test code though, where it is used in a safe manner.

Comment 13

[Jon Coppeard (:jonco)]

(following IRC comments)

It would be great make rust's rooted! macro also work for things like StackGCVector<>. Alternatively we could make IdVector create a PersistentRooted<StackGCVector<jsid>>. As Yoshi says this is probably for another bug though.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1534967 - Part 3: remove Auto(Id)Vector.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/427b854cdb1c
Part 1: use RootedIdVector. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbc1ac9d28a
Part 2: Use RootedIdVector in rust binding. r=jonco,fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c5c5fd10cd6
Part 3: remove Auto(Id)Vector. r=jonco

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/427b854cdb1c
https://hg.mozilla.org/mozilla-central/rev/fbbc1ac9d28a
https://hg.mozilla.org/mozilla-central/rev/2c5c5fd10cd6

Comment 17

[Anthony Ramine [:nox]]

I'm doing a SpiderMonkey update in Servo… What are we supposed to use now instead of AutoIdVector? Note that before this patch, we would just create an AutoIdVector and make sure we root it properly with our rooted! macro.