Closed Bug 1534967 Opened 2 years ago Closed 2 years ago

Replace AutoIdVector with RootedIdVector

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.

Assignee: nobody → allstars.chh

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

Flags: needinfo?(jcoppeard)

(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].

Flags: needinfo?(jcoppeard)
Status: NEW → ASSIGNED

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

Attachment #9053876 - Flags: feedback?(jcoppeard)

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 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.

Attachment #9053876 - Flags: feedback?(jcoppeard) → feedback+

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?

Flags: needinfo?(jcoppeard)

(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.

Flags: needinfo?(jcoppeard) → needinfo?(nfitzgerald)

(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),

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.

Flags: needinfo?(nfitzgerald)

(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.

(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.

Blocks: 1538386
Attachment #9053876 - Attachment description: Bug 1534967 - use RootedIdVector → Bug 1534967 - Part 1: use RootedIdVector.
Attachment #9054417 - Attachment description: Bug 1534967: rust binding → Bug 1534967 - Part 2: Use RootedIdVector in rust binding.
Pushed by allstars.chh@gmail.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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.

You need to log in before you can comment on or make changes to this bug.