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

Fix tojson unconditionally serializes #26054

Merged
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Next

fixed #25281

  • Loading branch information
shnmorimoto committed Mar 28, 2020
commit 80b2a87be718d81032a14bc96d94bb37318c31d8
@@ -2968,12 +2968,19 @@ def __init__(self, descriptor, toJSONMethod):
def definition_body(self):
ret = ''
interface = self.descriptor.interface

for m in interface.members:
if m.isAttr() and not m.isStatic() and m.type.isJSONType():
name = m.identifier.name
conditions = MemberCondition(None, None, m.exposureSet)
ret_conditions = 'vec![' + ",".join(conditions) + "]"

This comment has been minimized.

@jdm

jdm Mar 30, 2020

Member

Let's use &[ instead of vec![ to avoid allocating a bunch of memory needlessly. Also use ", " when joining to make the resulting code a bit easier to read.

This comment has been minimized.

@shnmorimoto

shnmorimoto Mar 31, 2020

Author Contributor

thanks! fixed 74995a5

ret += fill(
"""
rooted!(in(cx) let mut temp = UndefinedValue());
let conditions = ${conditions};
if !conditions.iter().any(|c| c.is_satisfied(SafeJSContext::from_ptr(cx), HandleObject::from_raw(obj), HandleObject::from_raw(obj))) {
return false;

This comment has been minimized.

@jdm

jdm Mar 28, 2020

Member

I believe the timeouts in the tests are caused by returning here if the condition is not satisfied, rather than only executing the following code if the condition is satisfied.

This comment has been minimized.

@shnmorimoto

shnmorimoto Mar 29, 2020

Author Contributor

Thanks for review and checking. Do you mean that we should skip like below instead of returning false?

if flag = conditions.iter().any(|c|
         c.is_satisfied(
           SafeJSContext::from_ptr(cx),
           HandleObject::from_raw(obj),
           HandleObject::from_raw(obj)));
if flag {
    rooted!(in(cx) let mut temp = UndefinedValue());
    if !get_timing(cx, obj, this, JSJitGetterCallArgs { _base: temp.handle_mut().into() }) {
    return false;
    }
    if !JS_DefineProperty(cx, result.handle().into(),
                          b"timing\0" as *const u8 as *const libc::c_char,
                        temp.handle(), JSPROP_ENUMERATE as u32) {
    return false;
    }
}

I've tried it. But I still got timeout.
I'll continue to look into it.

This comment has been minimized.

@jdm

jdm Mar 29, 2020

Member

Ah, another problem is that we should be passing a global object as the third argument of is_satisfied based on https://doc.servo.org/script/dom/bindings/guard/enum.Condition.html#method.is_satisfied. We should be able to use something like:

let incumbent_global = GlobalScope::incumbent().expect("no incumbent global");
let global = incumbent_global.reflector().get_jsobject();

And then you could pass global as the third argument.

This comment has been minimized.

@shnmorimoto

shnmorimoto Mar 29, 2020

Author Contributor

Oh, sorry. I got it.
I fixed it and confirmed if the timeout isn't occurred.

}
rooted!(in(cx) let mut temp = UndefinedValue());
if !get_${name}(cx, obj, this, JSJitGetterCallArgs { _base: temp.handle_mut().into() }) {
return false;
}
@@ -2983,7 +2990,7 @@ def definition_body(self):
return false;
}
""",
name=name, nameAsArray=str_to_const_array(name))
name=name, nameAsArray=str_to_const_array(name), conditions=ret_conditions)
ret += 'return true;\n'
return CGGeneric(ret)

@@ -50,7 +50,7 @@ pub enum Condition {
}

impl Condition {
fn is_satisfied(&self, cx: JSContext, obj: HandleObject, global: HandleObject) -> bool {
pub fn is_satisfied(&self, cx: JSContext, obj: HandleObject, global: HandleObject) -> bool {
match *self {
Condition::Pref(name) => prefs::pref_map().get(name).as_bool().unwrap_or(false),
Condition::Func(f) => f(cx, obj),
@@ -375,6 +375,9 @@ pub fn is_exposed_in(object: HandleObject, globals: Globals) -> bool {
unsafe {
let unwrapped = UncheckedUnwrapObject(object.get(), /* stopAtWindowProxy = */ 0);
let dom_class = get_dom_class(unwrapped).unwrap();
if (dom_class.global.is_empty()) && (!globals.is_empty()) {
return false
}
globals.contains(dom_class.global)
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.