Add microtask checkpoints to script elements and custom elements

[pshaughn]

Servo had a microtask checkpoint at the end of running a script, but there was also supposed to be one at the end of HTML-parsing a script element before Javascript-parsing the script itself, and there were supposed to be checkpoints immediately after the call to a custom element constructor. This adds those, passing all cases of one WPT test file.

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• These changes fix #25016 except for the remaining not-really-about-microtasks case #25514

• There are tests for these changes

[highfive]

Heads up! This PR modifies the following files:

• @asajeffrey: components/script/dom/customelementregistry.rs, components/script/dom/servoparser/mod.rs
• @KiChjang: components/script/dom/customelementregistry.rs, components/script/dom/servoparser/mod.rs

[nox]

@bors-servo r+

[bors-servo]

📌 Commit e4a5d8d has been approved by nox

[bors-servo]

⌛ Testing commit e4a5d8d with merge d8a7ae9...

[bors-servo]

💔 Test failed - status-taskcluster

[jdm]

  ▶ Unexpected subtest result in /dom/nodes/MutationObserver-document.html:
  └ PASS [expected FAIL] parser insertion mutations
  ▶ Unexpected subtest result in /dom/nodes/MutationObserver-document.html:
  └ PASS [expected FAIL] parser script insertion mutation
  ▶ Unexpected subtest result in /dom/nodes/MutationObserver-document.html:
  └ PASS [expected FAIL] removal of parent during parsing

This also changes the behaviour of a reftest, interestingly:

  ▶ FAIL [expected PASS] /_mozilla/mozilla/FileAPI/blob_url_upload.html
  │   → /_mozilla/mozilla/FileAPI/blob_url_upload.html 072d32c1bd0b78ac1c5af7657d6e5cedfb9ec8d6
  └   → /_mozilla/mozilla/FileAPI/blob_url_upload_ref.html 84f6d3bd2a1b363f32fc2f41c2c8dc78d0dfd881

[pshaughn]

That mozilla blob one happens intermittently for me on this branch; I'll see if it also happens intermittently for me on master. My first guess here is that the screenshot has a chance of being taken before the file is done loading.

[pshaughn]

It doesn't seem to happen on master.

Hypothesis: The new checkpoint that happens when parsing </script> before compiling the script body fires an ImageMicrotask, that ImageMicrotask does something that tells the document "I'm done being loaded", the document sees that html5ever is done parsing it, and the load event fires even though we're still inside the tokenizer for the final script element.

The one thing I know I didn't follow the exact spec for was that the checkpoint is supposed to happen before "Switch the insertion mode to the original insertion mode", but there wasn't a way to do that with the way Servo and html5ever are connected so I hoped it wasn't observable. I need to look at how the document decides whether it's done being loaded; maybe the insertion mode is part of the problem.

[pshaughn]

Nothing is logged from document, servoparser, or htmlimageelement between entering and exiting the suspicious checkpoint; all the interesting action happens after it.

Running it with logs on, in this branch it looks there is a race between "Document got finish_load: Image([blob url])" and "Document got finish_load: PageSource([test url]), and if Image comes first the test fails. Next question I need to look at: in master, do these events always have a strict order (so I need to make sure one waits for the other), or do we pass the test regardless of order (so I need to make something else insensitive to the difference)?

[pshaughn]

It looks like with my changes, the document is seeing the finish_load for the the image before the image's process_response has been reached (but after the image has had its blob URL assigned). That then means we have a race between when process_response happens and when the screenshot is taken. In master, the finish_load for the image always happens after process_response. These calls all come later in the timeline than my actual code changes, but I can infer that adding a microtask checkpoint breaks some expectation an image had about its lifecycle, and finding that expectation is the next step.

[pshaughn]

servo/components/script/dom/htmlimageelement.rs

Lines 843 to 847 in 85ea8df

	// FIXME(nox): According to the spec, setting the current
	// request to the broken state is done prior to queuing a
	// task, why is this here?
	this.abort_request(State::Broken, ImageRequestPhase::Current);
	this.abort_request(State::Broken, ImageRequestPhase::Pending);

This seems to be the thing that is firing at the wrong time; moving it out of the task like the comment says, nothing immediately crashes and the one reftest seems to pass consistently. I'm expecting something else to break.

[pshaughn]

@bors-servo try=wpt

[bors-servo]

⌛ Trying commit da0c8c4 with merge 8d585bc...

[bors-servo]

💔 Test failed - status-taskcluster

[pshaughn]

Just another one of those background-color intermittents and the new MutationObserver passes. I am surprised that change worked so smoothly.

[jdm]

@bors-servo r+

[bors-servo]

📌 Commit c725f9e has been approved by jdm

[bors-servo]

⌛ Testing commit c725f9e with merge 11bbb25...

[bors-servo]

💔 Test failed - status-taskcluster

[jdm]

@bors-servo retry

• #25513 #23290

[bors-servo]

⌛ Testing commit c725f9e with merge 5387994...

[bors-servo]

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing 5387994 to master...