Closed Bug 1534967 Opened 9 months ago Closed 8 months 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: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Regressions: 1542980
You need to log in before you can comment on or make changes to this bug.