ToJSON unconditionally serializes all conditionally-exposed interface members

[shnmorimoto]

This test will crash after this PR.

▶ CRASH [expected OK] /hr-time/idlharness.any.worker.html
│
│
│
│
│ internal error: entered unreachable code: Are we trying to expose Performance.timing in workers? (thread WebWorker for http://web-platform.test:8000/hr-time/idlharness.any.worker.js, at components/script/dom/performan
ce.rs:394)
│ stack backtrace:
│ 0: backtrace::backtrace::trace_unsynchronized
│ 1: <servo::backtrace::Print as core::fmt::Debug>::fmt
│ 2: core::fmt::write
│ 3: <std::io::stdio::Stdout as std::io::Write>::write_fmt
│ 4: std::io::stdio::_print
│ 5: servo::backtrace::print
│ 6: servo::main::{{closure}}
│ 7: std::panicking::rust_panic_with_hook
│ 8: rust_begin_unwind
│ 9: std::panicking::begin_panic_fmt
│ 10: <script::dom::performance::Performance as script::dom::bindings::codegen::Bindings::PerformanceBinding::PerformanceBinding::PerformanceMethods>::Timing
│ 11: script::dom::bindings::codegen::Bindings::PerformanceBinding::PerformanceBinding::get_timing::{{closure}}
│ 12: core::ops::function::FnOnce::call_once
│ 13: <std::panic::AssertUnwindSafe as core::ops::function::FnOnce<()>>::call_once
│ 14: std::panicking::try::do_call
│ 15: __rust_maybe_catch_panic
│ 16: std::panicking::try
│ 17: std::panic::catch_unwind
│ 18: mozjs::panic::wrap_panic
│ 19: script::dom::bindings::codegen::Bindings::PerformanceBinding::PerformanceBinding::get_timing
│ 20: script::dom::bindings::codegen::Bindings::PerformanceBinding::PerformanceBinding::CollectJSONAttributes
│ 21: script::dom::bindings::codegen::Bindings::PerformanceBinding::PerformanceBinding::toJSON::{{closure}}
│ 22: core::ops::function::FnOnce::call_once
│ 23: <std::panic::AssertUnwindSafe as core::ops::function::FnOnce<()>>::call_once
│ 24: std::panicking::try::do_call
│ 25: __rust_maybe_catch_panic
│ 26: std::panicking::try
│ 27: std::panic::catch_unwind
│ 28: mozjs::panic::wrap_panic
│ 29: script::dom::bindings::codegen::Bindings::PerformanceBinding::PerformanceBinding::toJSON
│ 30: CallJitMethodOp
│ 31: script::dom::bindings::utils::generic_call
│ 32: script::dom::bindings::utils::generic_method
│ 33: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
│ 34: _ZL9InterpretP9JSContextRN2js8RunStateE
│ 35: _ZN2js9RunScriptEP9JSContextRNS_8RunStateE
│ 36: _ZN2js23InternalCallOrConstructEP9JSContextRKN2JS8CallArgsENS_14MaybeConstructE
│ 37: _ZN2js4CallEP9JSContextN2JS6HandleINS2_5ValueEEES5_RKNS_13AnyInvokeArgsENS2_13MutableHandleIS4_EE
│ 38: _ZN2js3jit14InvokeFunctionEP9JSContextN2JS6HandleIP8JSObjectEEbbjPNS3_5ValueENS3_13MutableHandleIS8_EE
│ 39: _ZN2js3jit25InvokeFromInterpreterStubEP9JSContextPNS0_30InterpreterStubExitFrameLayoutE
│
│ [2019-12-13T14:29:19Z ERROR servo] internal error: entered unreachable code: Are we trying to expose Performance.timing in workers?
└ Pipeline failed in hard-fail mode. Crashing!

[jdm]

This looks like the generated CollectJSONAttributes for Performance does not take into account a) the set of globals that each attribute is exposed in and b) the global that is present when toJSON() is invoked.

[jdm]

Some relevant pointers:

servo/components/script/dom/bindings/codegen/CodegenRust.py

Lines 2920 to 2934 in 0954871

	for m in interface.members:
	if m.isAttr() and not m.isStatic() and m.type.isJSONType():
	name = m.identifier.name
	ret += fill(
	"""
	rooted!(in(*cx) let mut temp = UndefinedValue());
	if !get_${name}(cx, obj, this, JSJitGetterCallArgs { _base: temp.handle_mut().into() }) {
	return false;
	}
	if !JS_DefineProperty(*cx, result.handle().into(),
	${nameAsArray} as *const u8 as *const libc::c_char,
	temp.handle(), JSPROP_ENUMERATE as u32) {
	return false;
	}
	""",

servo/components/script/dom/bindings/codegen/CodegenRust.py

Lines 1498 to 1518 in 0954871

	def MemberCondition(pref, func, exposed):
	"""
	A string representing the condition for a member to actually be exposed.
	Any of the arguments can be None. If not None, they should have the
	following types:
	pref: The name of the preference.
	func: The name of the function.
	exposed: One or more names of an exposed global.
	"""
	assert pref is None or isinstance(pref, str)
	assert func is None or isinstance(func, str)
	assert exposed is None or isinstance(exposed, set)
	assert func is None or pref is None or exposed is None
	if pref:
	return 'Condition::Pref("%s")' % pref
	if func:
	return 'Condition::Func(%s)' % func
	if exposed:
	return ["Condition::Exposed(InterfaceObjectMap::Globals::%s)" % camel_to_upper_snake(i) for i in exposed]
	return "Condition::Satisfied"

[shnmorimoto]

I would like to try and solve this one.
@highfive: assign me

[highfive]

Hey @shnmorimoto! Thanks for your interest in working on this issue. It's now assigned to you!

[shnmorimoto]

@jdm If there are a good document about codegen, I'd be happy if you could let me know.

[jdm]

Unfortunately it is not documented well :( #23332 is the issue tracking the work that added Exposed support for interface members, and #23500 is the PR for that work.

[shnmorimoto]

Thank you for the helpful issue and PR.
I will refer to them and try to solve this issue.

[shnmorimoto]

How do I view the code generated by CodegenRust.py?
I checked the options of mach but could not find it.

[pshaughn]

It's still on the filesystem after the build ends; if you know a string you want to find, you can grep -r for it. There's probably an easier way but I don't happen to know it.

[shnmorimoto]

@pshaughn Thanks! I'll give it a try!

[jdm]

You will find the generated files under target/debug/build/script-[hash]/out/Bindings/.

[shnmorimoto]

I'm sorry to be away from here for a long time.

Could you confirm my understanding is correct?

• Performance is only exposed to Window. This crash occurs when called from elsewhere.
• CollectJSONAttributes should return false when called from where it is not exposed.
  (Actually I don't really understand what CollectJSONAttributes does..)

[shnmorimoto]

@jdm I added bellow code to CGCollectJSONAttributesMethod in CodegenRust.py

    def definition_body(self):
        ret = ''
        interface = self.descriptor.interface
        conditions = MemberCondition(None, None, interface.exposureSet)

        if isinstance(conditions, list):
            ret += "let condition = vec!["
            for condition in conditions:
                ret += condition
                ret += ","
            ret += "];\n"
            ret += """
                if !condition.iter().any(|c| c.is_satisfied(SafeJSContext::from_ptr(cx), Handle::from_raw(obj), Handle::from_raw(obj))) {
                  return false;
                } 
                """

and Regarding Performance, I expect

let condition = vec![Condition::Exposed(InterfaceObjectMap::Globals::WINDOW)]

since webidl is bellow.

servo/components/script/dom/webidls/Performance.webidl

Lines 47 to 53 in 191f6c4

	[Exposed=Window]
	partial interface Performance {
	[SameObject]
	readonly attribute PerformanceNavigationTiming timing;
	[SameObject]
	readonly attribute PerformanceNavigation navigation;
	};

but got

let condition = vec![Condition::Exposed(InterfaceObjectMap::Globals::WINDOW),Condition::Exposed(InterfaceObjectMap::Globals::SERVICE_WORKER_GLOBAL_SCOPE),Condition::Exposed(InterfaceObjectMap::Globals::DEDICATED_WORKER_GLOBAL_SCOPE),];

How do I get expose definition from webidl?

[jdm]

@shnmorimoto The Performance interface is exposed to all of those via

servo/components/script/dom/webidls/Performance.webidl

Line 12 in 191f6c4

[Exposed=(Window, Worker)]

. For the ToJSON, we'll need to look at the exposure set for each member.

[jdm]

More specifically, in this code we'll want to use m.exposureSet to create a list of conditions from MemberCondition, rather than using interface.exposure, and we'll need to call is_satisfied for each member and skip it if it's false.

[shnmorimoto]

Thanks for the helpful info! I got it.
I'll do that.