Auto merge of #20265 - Xanewok:fix-js-objects-in-unions, r=jdm

Fix JS object conversion in unions

<!-- Please describe your changes on the following line: -->
Requires safe `Heap::boxed` constructor from https://github.com/servo/rust-mozjs/pull/395 (more info on it is in the PR).

Since unions currently assume that their respective members root themselves and can be stored on heap, I modified the union member object conversion branch to convert to a `RootedTraceableBox<Heap<*mut JSObject>>` (which is the currently generated type for objects in said unions).

I did it only for Unions and not dictionaries, since some dictionaries had bare `*mut JSObject` members - is this a mistake and something that needs further fixing?

Does this need a test, e.g. passing a union with object to a function that returns said object, and comparing the results for equality?

r? @jdm

Generated code with this patch (for `StringOrObject`):
```rust
impl FromJSValConvertible for StringOrObject {
    type Config = ();
    unsafe fn from_jsval(cx: *mut JSContext,
                         value: HandleValue,
                         _option: ())
                         -> Result<ConversionResult<StringOrObject>, ()> {
        if value.get().is_object() {
            match StringOrObject::TryConvertToObject(cx, value) {
                Err(_) => return Err(()),
                Ok(Some(value)) => return Ok(ConversionResult::Success(StringOrObject::Object(value))),
                Ok(None) => (),
            }

        }
        // (...)
    }
}

impl StringOrObject {
    // (...)
    unsafe fn TryConvertToObject(cx: *mut JSContext, value: HandleValue) -> Result<Option<RootedTraceableBox<Heap<*mut JSObject>>>, ()> {
        Ok(Some(RootedTraceableBox::from_box(Heap::boxed(value.get().to_object()))))
    }
}
```

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [ ] These changes fix #17011 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20265)
<!-- Reviewable:end -->
This commit is contained in:
bors-servo 2018-03-14 12:04:23 -04:00 committed by GitHub
commit e597cd9e1a
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
8 changed files with 68 additions and 18 deletions

View file

@ -1075,20 +1075,24 @@ def getJSToNativeConversionInfo(type, descriptorProvider, failureCode=None,
if type.isObject():
assert not isEnforceRange and not isClamp
# TODO: Need to root somehow
# https://github.com/servo/servo/issues/6382
templateBody = "${val}.get().to_object()"
default = "ptr::null_mut()"
templateBody = wrapObjectTemplate("${val}.get().to_object()",
default,
isDefinitelyObject, type, failureCode)
if isMember in ("Dictionary", "Union"):
# TODO: Do we need to do the same for dictionaries?
if isMember == "Union":
templateBody = "RootedTraceableBox::from_box(Heap::boxed(%s))" % templateBody
default = "RootedTraceableBox::new(Heap::default())"
declType = CGGeneric("RootedTraceableBox<Heap<*mut JSObject>>")
elif isMember == "Dictionary":
declType = CGGeneric("Heap<*mut JSObject>")
else:
# TODO: Need to root somehow
# https://github.com/servo/servo/issues/6382
declType = CGGeneric("*mut JSObject")
templateBody = wrapObjectTemplate(templateBody, default,
isDefinitelyObject, type, failureCode)
return handleOptional(templateBody, declType,
handleDefaultNull(default))
@ -4291,11 +4295,13 @@ class CGUnionConversionStruct(CGThing):
else:
mozMapObject = None
hasObjectTypes = interfaceObject or arrayObject or dateObject or object or mozMapObject
hasObjectTypes = object or interfaceObject or arrayObject or dateObject or mozMapObject
if hasObjectTypes:
# "object" is not distinguishable from other types
assert not object or not (interfaceObject or arrayObject or dateObject or callbackObject or mozMapObject)
templateBody = CGList([], "\n")
if object:
templateBody.append(object)
if interfaceObject:
templateBody.append(interfaceObject)
if arrayObject:
@ -4363,11 +4369,6 @@ class CGUnionConversionStruct(CGThing):
returnType = "Result<Option<%s>, ()>" % actualType
jsConversion = templateVars["jsConversion"]
# Any code to convert to Object is unused, since we're already converting
# from an Object value.
if t.name == 'Object':
return CGGeneric('')
return CGWrapper(
CGIndenter(jsConversion, 4),
pre="unsafe fn TryConvertTo%s(cx: *mut JSContext, value: HandleValue) -> %s {\n"

View file

@ -765,7 +765,12 @@ unsafe impl<T: JSTraceable + 'static> JSTraceable for RootedTraceableBox<T> {
impl<T: JSTraceable + 'static> RootedTraceableBox<T> {
/// DomRoot a JSTraceable thing for the life of this RootedTraceable
pub fn new(traceable: T) -> RootedTraceableBox<T> {
let traceable = Box::into_raw(Box::new(traceable));
Self::from_box(Box::new(traceable))
}
/// Consumes a boxed JSTraceable and roots it for the life of this RootedTraceable.
pub fn from_box(boxed_traceable: Box<T>) -> RootedTraceableBox<T> {
let traceable = Box::into_raw(boxed_traceable);
unsafe {
RootedTraceableSet::add(traceable);
}

View file

@ -293,6 +293,14 @@ impl TestBindingMethods for TestBinding {
fn ReceiveInterfaceSequence(&self) -> Vec<DomRoot<Blob>> {
vec![Blob::new(&self.global(), BlobImpl::new_from_bytes(vec![]), "".to_owned())]
}
#[allow(unsafe_code)]
unsafe fn ReceiveUnionIdentity(
&self,
_: *mut JSContext,
arg: UnionTypes::StringOrObject,
) -> UnionTypes::StringOrObject {
arg
}
fn ReceiveNullableBoolean(&self) -> Option<bool> { Some(false) }
fn ReceiveNullableByte(&self) -> Option<i8> { Some(0) }

View file

@ -228,6 +228,8 @@ interface TestBinding {
TestDictionary receiveTestDictionaryWithSuccessOnKeyword();
boolean dictMatchesPassedValues(TestDictionary arg);
(DOMString or object) receiveUnionIdentity((DOMString or object) arg);
void passBoolean(boolean arg);
void passByte(byte arg);
void passOctet(octet arg);