Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error in generated binding for variadic argument of type that needs rooting #8159

Closed
mbrubeck opened this issue Oct 22, 2015 · 7 comments
Closed
Assignees

Comments

@mbrubeck
Copy link
Contributor

@mbrubeck mbrubeck commented Oct 22, 2015

The following WebIDL (where Touch is as defined in #7204):

partial interface Document {
      TouchList createTouchList(Touch... touches);
};

causes this compile error:

DocumentBinding.rs:2184:61: 2184:64 error: no method named `r` found for type `collections::vec::Vec<dom::bindings::js::Root<dom::touch::Touch>>` in the current scope
DocumentBinding.rs:2184     let result: Root<TouchList> = this.CreateTouchList(arg0.r());
                                                                                    ^~~

The generated trait method signature is:

    fn CreateTouchList(&self, touches: Vec<&Touch>) -> Root<TouchList>;

and the generated binding is:

unsafe extern fn createTouchList(cx: *mut JSContext, _obj: HandleObject, this: *const Document, args: *const JSJitMethodCallArgs) -> bool {
    let this = &*this;
    let args = &*args;
    let argc = args._base.argc_;
    let arg0: Vec<Root<Touch>> = if 0 < argc {
        let mut vector: Vec<Root<Touch>> = Vec::with_capacity((argc - 0) as usize);
        for variadicArg in 0..argc {
            let slot: Root<Touch> = if args.get(variadicArg).get().is_object() {
                match native_from_handlevalue(args.get(variadicArg)) {
                    Ok(val) => val,
                    Err(()) => {
                        throw_type_error(cx, "value does not implement interface Touch.");
                        return false;
                    }
                }
            } else {
                throw_type_error(cx, "Value is not an object.");
                return false;
            };
            vector.push(slot);
        }
        vector
    } else {
        Vec::new()
    };
    let result: Root<TouchList> = this.CreateTouchList(arg0.r());

    (result).to_jsval(cx, args.rval());
    return true;
}
@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Oct 23, 2015

This is probably because CGPerSignatureCall.getArguments doesn't check whether the argument is variadic. I suspect adding that check would fix this (but note the duplicated code in CGProxySpecialOperation.getArguments).

@nox nox self-assigned this Oct 23, 2015
@nox
Copy link
Member

@nox nox commented Oct 25, 2015

I would rather make it pass &[&T] instead of just removing that .r() call, no?

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Oct 25, 2015

I guess a Vec<Root<T>> is useless anyway. We could go either way.

@nox
Copy link
Member

@nox nox commented Oct 25, 2015

Sounds like making the proper thing (&[&T]) is more complex than just removing the .r() call, for now.

@nox
Copy link
Member

@nox nox commented Oct 25, 2015

Especially because I think the RootedVec::new() then needs to be moved outside the block that defines arg0, so that it doesn't end up moved.

@nox
Copy link
Member

@nox nox commented Oct 25, 2015

So I managed to get the code to compile:

unsafe extern fn passVariadicInterface(cx: *mut JSContext, _obj: HandleObject, this: *const TestBinding, args: *const JSJitMethodCallArgs) -> bool {
    let this = &*this;
    let args = &*args;
    let argc = args._base.argc_;
    let arg0: _ = if 0 < argc {
        let mut vector = RootedVec::new();
        *vector = Vec::with_capacity((argc - 0) as usize);

        for variadicArg in 0..argc {
            let slot: *const Blob = if args.get(variadicArg).get().is_object() {
                match native_from_handlevalue(args.get(variadicArg)) {
                    Ok(val) => val,
                    Err(()) => {
                        throw_type_error(cx, "value does not implement interface Blob.");
                        return false;
                    }
                }
            } else {
                throw_type_error(cx, "Value is not an object.");
                return false;
            };
            vector.push(JS::from_ref(&*slot));
        }
        vector
    } else {
        RootedVec::new()
    };
    let result: () = this.PassVariadicInterface(arg0.r());

    (result).to_jsval(cx, args.rval());
    return true;
}

But I'm pretty sure this will fail at runtime when the RootedVec is dropped. Will write a test.

@nox
Copy link
Member

@nox nox commented Oct 25, 2015

Yes, this panics.

nox added a commit to nox/servo that referenced this issue Oct 25, 2015
We use a RootedVec value in codegen, of which we use the `r()` method to
pass `&[&T]` to the interface methods.
nox added a commit to nox/servo that referenced this issue Oct 25, 2015
We use a RootedVec value in codegen, of which we use the `r()` method to
pass `&[&T]` to the interface methods.
nox added a commit to nox/servo that referenced this issue Nov 11, 2015
We use a RootedVec value in codegen, of which we use the `r()` method to
pass `&[&T]` to the interface methods.
bors-servo added a commit that referenced this issue Nov 11, 2015
Support variadic interface arguments (fixes #8159)

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8197)
<!-- Reviewable:end -->
@nox nox closed this in acb13dc Nov 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.