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

Create a 'report an exception' algorithm per #958 #10404

Merged
merged 25 commits into from
Jul 30, 2024

Conversation

jeremyroman
Copy link
Collaborator

@jeremyroman jeremyroman commented Jun 7, 2024

This algorithm directly includes the error propagation and fallback behavior, and requires callers to supply the global scope to be used, rather than magically inferring it.

Call sites within HTML are replaced, but there is more work yet to be done.

  • At least two implementers are interested (and none opposed):
  • Tests are written and can be reviewed and commented upon at:
    • These changes are not intended to modify existing behavior.
  • Implementation bugs are filed:
    • These changes are not intended to modify existing behavior.
  • MDN issue is filed: n/a
  • The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

The following call sites are changed (unchecked ones merit some extra attention):

  • report validity steps: If the element is not being rendered, an unspecified error may be reported. No browser actually seems to fire error events in this case, though Safari does log to the developer console.
  • invoke custom element reactions: The mysterious "associated script" trick is carried forward here, but we should clarify which global and script this in fact is.
  • run a classic script: The script and its settings object (thus global) are readily available.
  • run a module script: Ditto (though the script plumbing may only be necessary for classic scripts, since muting is the only use).
  • reportError(): The global it's called on is used, contrary to that not being plumbed in previously but aligned with what one would expect.
  • register an import map: The errors here are always synthesized in parse an import map string, when there's typically no running script. A type="importmap" script can be synchronously added by other script ought to be reported to that global but is still reported to the relevant global of the script element and its document, which is the same one the register algorithm is being run for.
  • HostEnqueueFinalizationRegistryCleanupJob: finalizationRegistry.[[CleanupCallback]].[[Callback]].[[Realm]] doesn't seem to be used in either Chrome or Firefox (contrary to how timers work), and this can differ from finalizationRegistry.[[Realm]] if the function passed was created in a different realm than the registry itself.
  • HostEnqueuePromiseJob: Not currently cross-vendor compatible (left XXX for now).
  • event handler attributes: If it fails to parse, there's no JavaScript exception sitting around, so a JS SyntaxError is synthesized.
  • timers: I'm pretty confident (with experimentation) that browsers use the global scope for the realm the callback functions come from; in the case of passing strings to setTimeout and setInterval, it's always going to be the global scope that it was called on, since that's where it's evaluated.
  • microtask queuing: Similar to timers.
  • attaching a declarative shadow root: Not confident. It seems you would expect the global object associated with the element we're parsing, but this isn't the case (browser don't actually report this).

I also need to:

  • add links for the previously exported anchor names (or whatever decision Anne and Domenic come to)

/custom-elements.html ( diff )
/form-control-infrastructure.html ( diff )
/imagebitmap-and-animations.html ( diff )
/infrastructure.html ( diff )
/nav-history-apis.html ( diff )
/parsing.html ( diff )
/timers-and-user-prompts.html ( diff )
/webappapis.html ( diff )
/workers.html ( diff )

This algorithm directly includes the error propagation and fallback
behavior, and requires callers to supply the global scope to be used,
rather than magically inferring it.

Call sites within HTML are replaced, but there is more work yet to be
done.
@jeremyroman jeremyroman marked this pull request as ready for review June 14, 2024 21:07
@jeremyroman
Copy link
Collaborator Author

@domenic @annevk When you have a chance, would you mind an early review to make sure this aligns with what you had in mind as the first step of #958? There's probably a couple things that deserve a little extra attention before this is totally mergeable (see checkmarks above and class="XXX" added in this PR, some of which might be resolvable before merge), but want to make sure this is at least mostly correct.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back from vacation! Thanks so much for tackling this.

I got halfway through the review. Then I had an epiphany. (Maybe some parts of the review are still useful.)

A lot of complexity and XXX boxes here and coming from passing the script around, and trying to determine which script to use. But Looking at the Chromium code, muting is not really done by passing a script around. It bottoms out in IsSharedCrossOrigin(), which is a method of v8::Message. In other words, it is derived from the JS engine's representation of the thrown exception, in the same way that we are already hand-waving about for line number, column number, and URL.

Given that we have no interop on muting, and are pretty sure the current spec doesn't match our ideal (per long discussions in #958 and a few other issues like #3149) why are we trying to preserve the framework of it being derived from the script?

My suggestion is that we change the spec hand-wave on the muting. We can have it mention that the user agent will probably want to use, for classic scripts, the "muted errors" boolean. (And that for module scripts, there is no need to mute errors.) But we don't need to find a way to thread through the "right" script.

We can then change the algorithm to only take an exception and a global.

source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated
<span>associated realm</span>'s <span data-x="concept-realm-global">global object</span>.</p>

<p w-nodev class="note">This global object not necessarily the same as <span>this</span>, if
<var>callback</var> comes from another <span>realm</span>.</p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you determine which choice of global to use here? If you're sure you're right, then can we have a tracking issue to write web platform tests to confirm? If you're not sure, we might want to leave this as an XXX box.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lovely test page https://excited-jealous-saturn.glitch.me/ (and a few more pages under that glitch for other cases, like finalization registries and import maps).

Copy link
Collaborator Author

@jeremyroman jeremyroman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

responded to most comments; will address editorial comments later

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated
<span>associated realm</span>'s <span data-x="concept-realm-global">global object</span>.</p>

<p w-nodev class="note">This global object not necessarily the same as <span>this</span>, if
<var>callback</var> comes from another <span>realm</span>.</p>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lovely test page https://excited-jealous-saturn.glitch.me/ (and a few more pages under that glitch for other cases, like finalization registries and import maps).

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for working on this. Great to see some progress on this issue. I wanted to note that "report the exception" has many callers that we cannot break: https://dontcallmedom.github.io/webdex/r.html#report%20the%20exception%40%40html%25%25dfn

Continuing to export the term is probably the bare minimum we need to do. Follow-up issues/downstream PRs might also be needed?

source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
@jeremyroman
Copy link
Collaborator Author

Thank you for working on this. Great to see some progress on this issue. I wanted to note that "report the exception" has many callers that we cannot break: https://dontcallmedom.github.io/webdex/r.html#report%20the%20exception%40%40html%25%25dfn

Continuing to export the term is probably the bare minimum we need to do. Follow-up issues/downstream PRs might also be needed?

I agree. Tracking followup PRs in the original issue makes sense to me once this PR is in a shape you are happy with. I had the task of doing that as an XXX in the original PR because I wasn't sure what the right wattsi syntax is. I'm still not sure I got it right, but <span id="..."></span> is at least used in one other place. I guess the alternative would be to have explicit visible <dfn>s for both of them in a paragraph noting that other specs should migrate.

@annevk
Copy link
Member

annevk commented Jul 8, 2024

Maintaining incoming ID references is no longer enough now HTML participates in Bikeshed's cross-referencing system. Once you drop a <dfn export> you break downstream that relies on it.

@domenic
Copy link
Member

domenic commented Jul 8, 2024

My plan was to break all of those so that they would notice and update to the new algorithm, which will have new inputs and possibly be used quite differently (e.g. if we do the automatic handling of Web IDL callbacks). That feels a bit better than having them call the new algorithm with the wrong arguments silently...

@annevk
Copy link
Member

annevk commented Jul 8, 2024

Hmm. It's usually pretty frustrating when a build is broken, especially when fixing it is tricky, but also just because it might block landing other PRs. (Although I understand that outside the WHATWG folks typically don't have fatal builds, that's more of a problem worth fixing than something we should rely on I think.)

@jeremyroman
Copy link
Collaborator Author

The alternative might be to expose "stub" dfns, rewrite the uses indexed by webdex, and then convert them into mere anchors?

@annevk
Copy link
Member

annevk commented Jul 9, 2024

Right yeah, I'm not opposed to that at all. I'd just like to avoid build time errors for people trying to fix something else in their specification unrelated to this.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great.

So we have two cases where the global object is unclear. Previously you've had good success testing those and then making a call. Do you want to do that for the last two? Or should we box them off with XXXs, to get this over the finish line?

I think the biggest question is whether to keep the scripts or not, per my #10404 (review). It seems like there are several cases for the script:

  1. The script is clear: "run a classic script" and "run a module script".
  2. An API is being called directly, e.g. reportError(). Probably the active script would work fine here.
  3. An API indirectly queues a callback, e.g. requestAnimationFrame(), queueMicrotask(), promise.then(), any DOM manipulation that triggers custom elements. Probably capturing the active script at queueing time would be correct and relatively straightforward here?
  4. An API that very indirectly triggers script execution, e.g. the script that originally set an onload="" attribute value or the script that caused the element to load; the script that inserted HTML that was parsed by declarative shadow DOM; the script that used document.write() to write a <script type="importmap">.
  5. There is definitely no script. E.g. declarative shadow DOM or <script type="importmap"> when reading from the server response.

Your work so far seems to give the clear unambiguous answer for (1) and (5). For (2)-(4) it's more mixed; sometimes you put "the appropriate script" and sometimes you put nothing. I suspect that no browsers do muting for (4), but as you note there are obvious interop differences for (2), and I suspect there are for (3) as well.

Given this, I think two paths are reasonable:

  • Make the script completely handwavey for now, as I suggested in Create a 'report an exception' algorithm per #958 #10404 (review). This might be more aligned with the actual architecture anyway.
  • Make it clearer when we're in cases (1) vs. (5) vs. (2)-(4). E.g. make the script argument non-optional, and always pass either the actual script value for (1), null for (5), and a <dfn>d "the appropriate script" for (2)-(4), where the dfn is a handwave and there's a nice XXX box discussing the problem space and linking to a followup issue. (I guess the fact that (4) and (5) end up in the same call sites in many cases can make this a bit complicated.)

What do you think?

source Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Outdated Show resolved Hide resolved
source Show resolved Hide resolved
source Outdated
throwing functions before passing them on to the function passed to the Promise superclass
constructor. Which global is to be used then, considering that the current realm could be
different at each of those steps, by using a Promise constructor or Promise.prototype.then
from another realm?</p>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this in Chrome, Firefox Nightly and Safari TP on the NewPromiseReactionJob path. They all do different things.

  • Chrome doesn't report the error at all, unless the subclassed reject function also throws -- then the realm in which the throw occurred gets the event (not necessarily the realm which defined the custom reject function, or which constructed the error) -- though I don't see in the ECMAScript spec why the subclassed reject function should be implicated at all
  • Firefox reports the error to the realm which defined the Promise subclass
  • Safari reports no error, either as an error event or to the developer console

Of those I guess Firefox's behavior may make the most sense to me, but I don't see an easy way to describe it without additional plumbing, and this feels like something that probably would require consideration by actual JS runtime experts, if we cared. My impression is that promise subclassing is pretty disfavoured these days anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's leave this as an XXX box and merge. Can you open a new issue that gives a code example and explains the results, and the question? We can link to it from the XXX box, either in this PR or in a quick followup.

source Outdated
<span>insert an element at the adjusted insertion location</span> with
<var>template</var>, and return.</p></li>
<var>serializable</var>, <var>delegatesFocus</var>, and "<code data-x="">named</code>". If
an exception is thrown, then catch it, <span data-x="report an exception">report</span> it
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, browsers don't seem to actually do this (at least for the exception type I tried -- the throwing a NotSupportedError DOMException for an invalid shadow host name).

Chrome will report the error to the console, but doesn't fire an error event. Firefox doesn't report it at all as far as I can tell. I can't make declarative shadow DOM work in my copy of Safari TP, so nothing happens there, either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Let's go with what you have written, and open a new issue with the code example. We can tag mfreed7 who wrote this spec text to get his take.

I suspect the outcome will be keeping the spec text you've written, and treating the lack of reports as browser bugs, since this is a relatively new feature.

I don't think an XXX box is necessary here but it wouldn't hurt to add.

@jeremyroman
Copy link
Collaborator Author

Replied first to the more specific items. On muting (which is what the script plumbing is for), I'd hoped to avoid doing a ton of research into how that is implemented in all three major engines, at least as a prerequisite to this PR. Not opposed to yanking it entirely into implementation-defined behavior if you think that the existing plumbing for it does more harm than good.

@jeremyroman
Copy link
Collaborator Author

As suggested, switched the script used for muting to an implementation-defined one with a "should" recommendation that it usually be the running script, and removed that argument from the few call sites which supplied it.

@domenic
Copy link
Member

domenic commented Jul 25, 2024

Replied first to the more specific items. On muting (which is what the script plumbing is for), I'd hoped to avoid doing a ton of research into how that is implemented in all three major engines, at least as a prerequisite to this PR.

Agreed; we should not block on that. The two paths I was offering in #10404 (review) were just alternate ways of deferring the hard cases to the future.

I'll open a new issue specifically about muting which we can link to and work on as a followup.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM. I'll give it a few days in case anyone else wants to take a look, and/or we want to make some final tweaks to link to followup issues.

@domenic
Copy link
Member

domenic commented Jul 25, 2024

I realized "report an error" has no external references per https://dontcallmedom.github.io/webdex/r.html. I will push a commit that removes its <dfn> but leaves the ID intact.

@jeremyroman
Copy link
Collaborator Author

Unsure whether this qualifies as purely editorial. If not, I think technically we need a second implementer interested (I assume @annevk on behalf of WebKit qualifies, if he supports this?).

@domenic
Copy link
Member

domenic commented Jul 26, 2024

I'm happy to count this as not requiring implementer support, using the carveout in https://whatwg.org/working-mode#non-normative-issues. It's a bit more ambitious than the examples there, but you're not changing anything here that might be controversial, from what I can tell. It's a combination of large editorial improvements and a few cases either making already-interoperable behavior explicit, or making non-interoperable behavior more obvious.

I'll still give this a few days in case others want to do any more review. Although I think @annevk is on European vacation?

@jeremyroman
Copy link
Collaborator Author

Assuming #958 (comment) was intended for here, so linking it.

@jeremyroman
Copy link
Collaborator Author

Happy with your draft commit message. The relevant issues are #10526 and #10527.

@domenic domenic merged commit 853c831 into whatwg:main Jul 30, 2024
2 checks passed
Ms2ger added a commit that referenced this pull request Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants