Timing of custom element microtasks isn't the way WPT expects it to be

[pshaughn]

WPT custom-elements/microtasks-and-constructors.html and perform-microtask-checkpoint-before-construction.html test many specific expectations about when the microtasks of custom elements happen, and Servo violates many of those expectations.

The microtasks-and-constructors test is a little hard to read, since it's broken up into separate script elements to make interesting event orders happen while the document is still being parsed. My reading of it is that multiple script bodies seem to be firing in a row, where the callbacks of custom elements between those scripts are expected be firing in between them. I'm not sure I understand it, though.

[jdm]

Marking this as high-value since tests that expose issues in Servo's timing/ordering of microtasks could plausibly have broader web compatibility impact.

The real question is whether this is limited to the custom-elements tasks or whether this is exposing an issue in when Servo runs microtasks at all.

[pshaughn]

We're missing a microtask checkpoint that should be coming from inside the parser: when a custom element constructor enqueues a microtask, that microtask should fire BEFORE the start of the next element, but instead in Servo the microtask hangs around in the queue until the checkpoint at the end of the next <script> block. The simplest case for this is named "Microtasks evaluate immediately when the stack is empty inside the parser.", which gives some clue where to look.

[jdm]

One question I have - given:

• servo/components/script/dom/htmlscriptelement.rs

  Line 792 in 0131952

  global.evaluate_script_on_global_with_result(

• servo/components/script/dom/globalscope.rs

  Line 1712 in 0131952

  let _aes = AutoEntryScript::new(self);

• servo/components/script/dom/bindings/settings_stack.rs

  Lines 78 to 80 in 0131952

  	// Step 5
  	if !thread::panicking() && incumbent_global().is_none() {
  	self.global.perform_a_microtask_checkpoint();

Why doesn't the microtask checkpoint occur?

[pshaughn]

That one does! The microtask that was supposed to fire earlier and didn't does fire at that point.

servo/tests/wpt/web-platform-tests/custom-elements/microtasks-and-constructors.html

Lines 37 to 61 in 0131952

	customElements.define("x-0", class extends HTMLElement {
	constructor() {
	super();
	logMicrotasks(window.x0Events);
	}
	});
	</script>
	<x-0></x-0>
	<script>
	"use strict";
	test(() => {
	assert_array_equals(window.x0Events, ["promise microtask", "MutationObserver microtask"]);
	}, "Microtasks evaluate immediately when the stack is empty inside the parser");
	customElements.define("x-bad", class extends HTMLElement {
	constructor() {
	super();
	doMicrotasks(() => this.setAttribute("attribute", "value"));
	}
	});
	</script>

We get checkpoints at 43 and 60; we're also supposed to be getting a checkpoint at 45.

[pshaughn]

https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token says to perform a checkpoint, but it looks like it's saying to do it before the custom element constructor call rather than after, so it doesn't seem to be the one this test wants.

[jdm]

It probably comes from https://heycam.github.io/webidl/#invoke-a-callback-function which comes from https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-element-reactions which comes from step 9 in that algorithm.

[jdm]

Either that or https://heycam.github.io/webidl/#construct-a-callback-function from the Upgrade path.

[pshaughn]

I am think it's case "An end tag whose name is <script>" in https://html.spec.whatwg.org/multipage/parsing.html#parsing-main-incdata actually. It looks like the INSTANT the HTML parser sees a </script>, before actually javascript-parsing the script itself, we are supposed to get a checkpoint.

[jdm]

Hmm, good point!

[pshaughn]

Adding a checkpoint at line 560 here:

servo/components/script/dom/servoparser/mod.rs

Lines 556 to 561 in 904fcb4

	let script = match feed(&mut *self.tokenizer.borrow_mut()) {
	Ok(()) => return,
	Err(script) => script,
	};
	let script_nesting_level = self.script_nesting_level.get();

passes some of the tests, but not all.

[pshaughn]

servo/tests/wpt/web-platform-tests/custom-elements/microtasks-and-constructors.html

Lines 54 to 70 in 904fcb4

	customElements.define("x-bad", class extends HTMLElement {
	constructor() {
	super();
	doMicrotasks(() => this.setAttribute("attribute", "value"));
	}
	});
	</script>
	<x-bad></x-bad>
	<script>
	"use strict";
	test(() => {
	const xBad = document.querySelector("x-bad");
	assert_false(xBad.hasAttribute("attribute"), "The attribute must not be present");
	assert_true(xBad instanceof HTMLUnknownElement, "The element must be a HTMLUnknownElement");

I don't yet understand why this constructor is supposed to fail and leave us with an HTMLUnknownElement, but Firefox and Chrome agree that it does; it fails in Servo by making an HTMLElement with an attribute.

[pshaughn]

I think I've found it; it zig-zags across three specs. https://dom.spec.whatwg.org/#concept-create-element says "the result of constructing C", with a link to the WebIDL spec. https://heycam.github.io/webidl/#construct-a-callback-function includes "Clean up after running script" before returning, with a link to the HTML spec. https://html.spec.whatwg.org/multipage/webappapis.html#clean-up-after-running-script says we run a checkpoint here.

That checkpoint would thus call setAttribute while we are still on step 6.1.2 of "create an element", meaning that when we get to step 6.1.5 we have an attribute and need to throw.

[jdm]

Implementation detail that may help - the codegen for webidl callbacks is based on

servo/components/script/dom/bindings/callback.rs

Lines 220 to 283 in 904fcb4

	/// A class that performs whatever setup we need to safely make a call while
	/// this class is on the stack. After `new` returns, the call is safe to make.
	pub struct CallSetup {
	/// The global for reporting exceptions. This is the global object of the
	/// (possibly wrapped) callback object.
	exception_global: DomRoot<GlobalScope>,
	/// The `JSContext` used for the call.
	cx: JSContext,
	/// The compartment we were in before the call.
	old_realm: *mut Realm,
	/// The exception handling used for the call.
	handling: ExceptionHandling,
	/// <https://heycam.github.io/webidl/#es-invoking-callback-functions>
	/// steps 8 and 18.2.
	entry_script: Option<AutoEntryScript>,
	/// <https://heycam.github.io/webidl/#es-invoking-callback-functions>
	/// steps 9 and 18.1.
	incumbent_script: Option<AutoIncumbentScript>,
	}
	impl CallSetup {
	/// Performs the setup needed to make a call.
	#[allow(unrooted_must_root)]
	pub fn new<T: CallbackContainer>(callback: &T, handling: ExceptionHandling) -> CallSetup {
	let global = unsafe { GlobalScope::from_object(callback.callback()) };
	if let Some(window) = global.downcast::<Window>() {
	window.Document().ensure_safe_to_run_script_or_layout();
	}
	let cx = global.get_cx();
	let aes = AutoEntryScript::new(&global);
	let ais = callback.incumbent().map(AutoIncumbentScript::new);
	CallSetup {
	exception_global: global,
	cx: cx,
	old_realm: unsafe { EnterRealm(*cx, callback.callback()) },
	handling: handling,
	entry_script: Some(aes),
	incumbent_script: ais,
	}
	}
	/// Returns the `JSContext` used for the call.
	pub fn get_context(&self) -> JSContext {
	self.cx
	}
	}
	impl Drop for CallSetup {
	fn drop(&mut self) {
	unsafe {
	LeaveRealm(*self.cx, self.old_realm);
	if self.handling == ExceptionHandling::Report {
	let _ac = JSAutoRealm::new(
	*self.cx,
	self.exception_global.reflector().get_jsobject().get(),
	);
	report_pending_exception(*self.cx, true);
	}
	drop(self.incumbent_script.take());
	drop(self.entry_script.take().unwrap());
	}
	}
	}

, which is based around the same AutoEntryScript that performs a microtask checkpoint in its drop implementation.

[pshaughn]

The "result of constructing C" phrase only appears twice and both times are about custom element definitions, so I don't think this will require a codegen change.

[pshaughn]

These actually aren't going through an AutoEntryScript; they're creating a js::jsapi::JSAutoRealm then calling directly into js::rust::wrappers::Construct1. The only other piece of code doing this is PaintWorkletGlobalScope::invoke_a_paint_callback.

In the paint worklet's case, the spec says "the result of Construct(paintCtor)" and is linking to ECMAScript, not WebIDL, for the definition of "Construct", so the paint worklet doesn't need to perform a microtask checkpoint on the way out of constructing and is of no concern here.

[jdm]

Ooh, that's good to know. We should probably add an AutoEntryScript, in that case.

[pshaughn]

I don't know whether AutoEntryScript applies here. Adding a checkpoint after exiting the Construct1 call's realm, combined with the checkpoint in the tokenizer, passes every test in microtasks_and_constructors.html.

None of this has resolved the failures in perform-a-microtask-before-construction.html. The most obvious difference I can see between the tests is that where microtasks-and-checkpoints.html does everything in the content document of a normally fetched top-level window, perform-a-microtask-before-construction.html sets the srcdoc of an iframe. My first thought is that this one might involve Servo checking the wrong global's custom element registry.

[jdm]

Totally believable!

[pshaughn]

The element is upgraded to custom, so it's the right registry, but the microtask checkpoint on upgrading doesn't fire, and this is almost definitely another timing problem in HTMLIFrameElement::navigate_or_reload_child_browsing_context (#24901, #25098), since that's what assigning to srcdoc actually calls and we already know it's getting things wrong. I will dig a little more to confirm that's what's happening, then if it is I'll do a PR for just the microtasks-and-constructors.html part.

[pshaughn]

Opened #25514 for that so I can close this.