Clarify "report an exception"

[Ms2ger]

the user agent must report the error for the relevant script

What is "the relevant script"?

---

Edit by @domenic: the current plan to be executed in order to fix this bug is #958 (comment). Comments between here and there are by now somewhat misleading and historical

[Ms2ger]

In particular in relation to the tests in https://bugzilla.mozilla.org/show_bug.cgi?id=1259784

[domenic]

I at first thought this was best solved by having "report an exception" explicitly specify which global the error goes to. However, I realize that doesn't really work, as we actually need the information on which script caused the exception, for filename (and line/column number) purposes.

I am pretty sure that the relevant script should generally be GetActiveScriptOrModule's [[HostDefined]] value (i.e. its corresponding HTML "script").

As discussed in the new #1189, there are scenarios where this returns null. Maybe this means that #1189 needs to expand to have a "backup incumbent script stack", not a "backup incumbent settings object stack".

/cc @bzbarsky.

[domenic]

Dang, nope, that makes no sense, because of cases like the user clicking on a button with throwing error handlers. In that case there's no GetActiveScriptOrModule() and no backup possible while inside the "dispatch an event" algorithm. Nevermind...

[bzbarsky]

The way this was meant to work, which everyone agreed on back when this was last discussed and when Hixie wrote the spec for this, before the later changes to it was like so:

1. When invoking callbacks the entry settings represent the thing being invoked and the incumbent settings represent the entity that added the callback.
2. Error reporting in the simple window1.setTimeout(functionFromWindow2) case should happen on window2, not window1. Conveniently, that corresponds to the entry settings object for the call in that case.

At the time, it seemed to me that the "invoking callbacks" section in HTML made this all fairly clear. It looks like that's all been removed since then, so now nothing specifies things... It doesn't help (but is far for the course) that some UAs agreed on all the above at the time and then never actually changed their behavior to align with the resulting spec.

[domenic]

So, looking through the old https://html.spec.whatwg.org/commit-snapshots/5fa33f072011f29ed56adb0f2f63bb4404c92aae#calling-scripts, I don't see how this was made clear. Error reporting has always been ambiguous and said

When the user agent is to report an exception E, the user agent must report the error for the relevant script, with the problematic position (line number and column number) in the resource containing the script, using the global object specified by the script's settings object as the target.

As I said over in whatwg/webidl#113 (comment) (sorry for splitting the discussion), I see two options for specifying this. The one not based on entry settings objects, but instead based on the settings object of the script that declared the function that threw an exception, seems simpler to me, since we need that script for its filename/line number/column number anyway.

[bzbarsky]

The function that threw an exception may not have been declared in a script at all. It can be a built-in function.

[bzbarsky]

And I guess it's possible that after the various discussion about this stuff things never made it back into the spec after all. :( I distinctly recall there being language proposed that made all this much clearer than what you link to. :(

[domenic]

For posterity:

@bzbarsky and I worked out a plan for fixing this in whatwg/webidl#113 (comment) (steps 2-4) plus @bzbarsky's subsequent reply (on which I agree with all points).

I've explained where this fits in my priority queue of "fix script execution" at the bottom of whatwg/webidl#113 (comment).

[domenic]

The plan:

Fix "report the exception"/"report an error"

The goal is to consolidate both of the existing algorithms "report the exception" and "report an error" into a single new algorithm, and update all call sites.

• The new algorithm's name will be "report an exception". (This is intentionally slightly different from both old algorithms. We will break spec cross-references in this way, to help move the ecosystem, but we can preserve the IDs so that actual links are not broken.)
• Its arguments will be an exception value, a global object, and an optional script
• We can get rid of the "handled" and "not handled" concepts, and instead include the line about reporting the error to the developer console directly inside the algorithm.

The contents of this algorithm will be based on "report an error", with the following notable differences:

• message, line, col, and filename are derived from the exception value, in an unspecified way. This allows the better results that in practice browsers exhibit for e.g. eval code. In the future, this may become specified in detail when the JavaScript spec specifies error stack traces in detail.

• The optional script will be used to determine muting (and not to determine the filename). If it is not supplied, assume the error is not muted.

• As noted above, incorporate the handled/not handled action directly into the algorithm.

Then, update all call sites of these two algorithms. Only do this for HTML for now, and it would be done in the same PR that updates the algorithm.

Port these changes to rejected promises

This might be best done as part of the previous PR. Essentially, get rid of the "handled" and "not handled" concepts, and incorporate the developer console part directly into the algorithm.

Change how web developer code invocation works in Web IDL

We add a new optional named parameter rethrowExceptions (a boolean) to the following three algorithms in the Web IDL spec:

• https://webidl.spec.whatwg.org/#call-a-user-objects-operation
• https://webidl.spec.whatwg.org/#invoke-a-callback-function
• https://webidl.spec.whatwg.org/#construct-a-callback-function

It defaults to false. When it is false, we introduce the following new behavior: any exceptions thrown by web developer code get caught and routed to HTML's new "report the exception". ("Web developer code" is roughly everything between the "prepare" and "cleanup" steps, including both argument conversion and the actual code invocation.)

The exact global and script to pass for these is unclear at this time. We can initially leave them as <span class="XXX">unclear which global</span>, <span class="XXX">unclear which script</span> and work on that in later steps.

Fix all call sites

Now we need to audit all the call sites of the algorithms we've changed:

• Report the exception
• Invoke
• Call a user object's operation
• Construct

In most cases, the pattern is that another spec is using invoke (or friends), then catching exceptions and routing them to "report the exception". After the above changes, this can be simplified to just calling invoke; the exception will be reported automatically through the shared infrastructure.

If another spec is using invoke and not catching exceptions, this might be an unintentional bug. In that case, we can leave it as-is. Or it might be intentional that they want the exception to bubble out, or be caught with a different reaction than reporting the exception. In that case the spec will need to be updated to pass the new rethrowExceptions parameter.

If another spec is directly using "report the exception", it will need to be given the appropriate new arguments, and possibly update its return value handling.

Further work on nailing down the specifics

At this point we've got good infrastructure, centralized in HTML and Web IDL. But a few core specifics are still unclear:

• Which global do we report exceptions to?
• How does error muting work?

The way to resolve these involves writing web platform tests, and/or code inspection of browser codebases. There are likely interop problems, especially for muting as discussed downthread. A test suite to illustrate the problems is step 1; after that we can discuss the desired resolution.

Any work on this sub-task feels like a bonus to me. If we accomplish the mostly-editorial refactoring above, we can split this work into new issues.

[bzbarsky]

The muted flag situation is hard: in reality the important thing is whether the script that threw the exception is muted, not the script that we're immediately invoking.

So if we load a cross-origin (muted) script with a function named f that throws, then have function g() { f() }, directly in our page, and pass g to a callback, then the exception ought to be muted, I would think. Not sure what UAs do here; in practice I suspect their muted exception stuff doesn't really match the spec's very well. For example, Gecko stores the muted state directly on the exception (and only for some types of exception values, afaict).

I'd be quite interested in what UAs do in practice with the muted error thing in various situations...

[domenic]

@TimothyGu pointed out that the confusion here, especially about which global to use, also applies to unhandledrejection.

[bzbarsky]

in reality the important thing is whether the script that threw the exception is muted

To be clear, this is only important in the spec's current conception of muting. I in practice, for the sort of cases where you call a function that calls another function that throws, muting is pointless because you can just catch the exception and get all the info out of it.

One case where muting is really important, and the reason it was added, is during initial script execution. At that point the only thing on the stack is the cross-site script (assuming you're not just reporting a SyntaxError in it), and the page should not be able to extract information via "error" listeners that it can't get otherwise from the cross-site file.

Arguably muting is also relevant when browser code directly invokes callbacks from a cross-site script, for similar reasons...

Anyway, it would be good if the spec actually clearly defined muting, ideally in a way that does not involve dynamic introspection of the "scripted stuff" stack.... Defining it in terms of the way script is being entered, for example, would address the actual use cases without requiring that sort of introspection.

[bzbarsky]

So I just did some testing, with the following HTML:

  <script>
    window.onerror = function(...args) {
      console.log(args);
    }
    function throwSameOrigin(err) {
      throw err;
    }

    function pong() {
      throwSameOrigin(new Error("Create same, throw same, cross on stack"));
    }

    throwSameOrigin(new Error("Create same, throw same"));
  </script>
  <script src="cross-origin-script"></script>
  <script>
      throwCrossOrigin(new Error("Create same, throw cross"));
  </script>
  <script>
    try {
      throwCrossOrigin(new Error("Create same, throw cross, rethrow same"));
    } catch (e) {
      throw e;
    }
  </script>
  <script>
    ping();
  </script>

and the following cross-origin script:

function throwCrossOrigin(err) {
  throw err;
}

throwSameOrigin(new Error("Create cross, throw same"));

function ping() {
  pong();
}

and the results I see are:

• Chrome: Mutes only the "Create same, throw cross" case.
• Safari: Mutes the "Create cross, throw same" and "Create same, throw cross" cases.
• Firefox: Mutes none of these.

So even just Chrome and Safari don't have interop on this...

[bzbarsky]

Looking at the state of this stuff, right now the basic "report an error" bits are pretty broken because they are not consistently invoked, afaict. For example, https://heycam.github.io/webidl/#es-invoking-callback-functions doesn't actually do it.

Would it make sense to have "Clean up after running script" do the error-reporting? If we did that, then we could also have it decide to mute or not based on what script it was being invoked. That doesn't match any existing browser's behavior, but does at least guarantee that if we start by entering cross-origin script then errors will be muted, while not trying to (pointlessly) mute errors thrown from cross-origin scripts after we enter at a same-origin script. And should be pretty clear to specify and implement...

@domenic @annevk thoughts?

[annevk]

I think so. Let me try to describe it in a different way. At some point the browser needs to parse and execute an opaque response as classic script. That's a synchronous operation. Every exception that happens during that operation ought to be muted. Beyond that we should not bother as it requires stack inspection or similar such measures that are not worth it.

[pshaughn]

So, if that opaque response script calls setTimeout with a callback that throws, you're thinking don't mute that callback's error?

[annevk]

Yeah, I think the historical motivation here was to avoid leaking file contents. E.g., you fetch some HTML using <script>, that results in a parsing exception that ends up leaking a ton of information.

I think we should make that even harder, via https://github.com/annevk/orb, but for resources that are JavaScript I don't think it's worth going above and beyond. (And covering setTimeout() would meet that as you would have to taint or stack trace, unless I'm missing something.) If you want to have secrets in your script, use Cross-Origin-Resource-Policy or equivalent to prevent being fetched from elsewhere.

[bzbarsky]

So, if that opaque response script calls setTimeout with a callback that throws

Just to be clear, if our threat model is that we are trying to protect that opaque response's exceptions from the main page, then if it does that it has lost. Consider the main page doing:

const orig = window.setTimeout.bind(window);
window.setTimeout = function(f, ...rest) {
  orig.call(function(...args) {
    try { f(...args); }
    catch (e) { /* Inspect the exception */ }
  }, ...rest);
}

Now it might be easier to do this via the global error handler than by instrumenting all the various callback-taking entrypoints, of course.

[annevk]

cc @wanderview @evilpie

[pshaughn]

It seems like it might be more consistent to only mute errors from compiling the script, and not from running it.

[bzbarsky]

Consider a "script" that looks like this:

Doe,John,"Munitions expert"
Doe,Jane,"Demolition expert"

That will compile fine, thanks to the wonders of ASI and the comma operator, but leak "Doe" in the ReferenceError that then gets thrown. So you need to deal with muting inital-run errors to avoid this very common attack on CSV files.

[pshaughn]

Thank you for the example.

[annevk]

One thing we should clarify about filename is that it's from before redirects. Additionally, if it's from before redirects we wouldn't need to mute it.

[evilpie]

One thing we should clarify about filename is that it's from before redirects. Additionally, if it's from before redirects we wouldn't need to mute it.

This ties into the current behavior in Firefox that I think we should specify. Firefox only exposes the filename/URL before any redirect. This means it should be okay to expose that URL even for cross-origin scripts. (You don't get any additional information, you can already tell which specifc script failed to load and you can obviously read .src)

This testcase shows that Firefox always uses the initial (pre-redirect) URL and doesn't censor it in script error events for cross-origin scripts. Chrome censors the filename and uses the final (post-redirect) URL.

[jeremyroman]

I've made a little headway here (spec code health rotation) according to @domenic's plan above. Mentioning this only to avoid duplicate work (not near ready).

[domenic]

Here is an idea on how to reduce the hand-waving for line number, column number, URL, and script. And maybe muting, if muting is a property of script.

• The JavaScript spec gets a host hook that is called whenever a throw completion is created. The host hook can attach data to the throw completion.
  • Alternate strategy: if attaching data to completion records or making room inside them for a [[HostDefined]] field is icky, we can store the data in a side table.
• We can use that hook to store the appropriate script, line number, column number, and URL in the completion record. The appropriate script can probably be derived via GetActiveScriptOrModule(), and from it, the URL. The line and column number might need hand-waving. (Or maybe we can try to figure out the current Parse Node using some JavaScript spec machinery magic?)
  • Alternate strategy: no host hook. The JS spec just directly stores the ScriptOrModule + Parse Node on all throw completions.
• We change some of the relevant infrastructure (most of which is in HTML, I think) to thread the whole abrupt completion to "report an exception", instead of just the exception value.

I think this is less of a priority than figuring out an interoperable story for muting that everyone agrees is ideal, though. It might be a useful technique for specifying that interoperable story, once we got there.

So to recap the current plan:

• Fix the mess of the spec having two algorithms, the Web IDL integration, etc. @jeremyroman is making great progress. Keep hand-waving on URL, line number, column number. And, per my latest proposal in Create a 'report an exception' algorithm per #958 #10404 (review), start hand-waving on script and muting.

• Write a bunch of WPT test cases for muting, as well as for URLs (filename). E.g. pre- or post-redirect.

• Agree on what we want those test case results to be, then commit them as .tentative WPTs.

• Figure out a spec for muting + URLs, and maybe lineno + colno, that matches those test cases. Maybe using something like what I describe above.

[annevk]

@ljharb @codehag @littledan @Ms2ger @syg thoughts on #958 (comment)? Would be nice to have exception handling detailed a bit better.

[ljharb]

@annevk the proposed hook seems plausible to me as far as providing extra data, but any behavior hosts alter about error stack traces risks making https://github.com/tc39/proposal-error-stacks even more difficult to advance, and should be done very carefully.

[codehag]

I haven't worked on this for a while so, please excuse any dumb thoughts here. cc @smaug---- because you may have some insights here.

1. It sort of sounds like incumbent realm might be involved here, especially if the error comes from an on-click handler. In which case the target global is probably going to be the incumbent? In which case are async contexts going to be potentially useful for this?

2. This is wobbly and I can be convinced otherwise but: "Alternate strategy: no host hook. The JS spec just directly stores the ScriptOrModule + Parse Node on all throw completions." -- feels weird because a) it affects all throw completions and b) this will make throw completions have additional fields compared to other completions, whereas now I think they are all the same per this table? The host hook is more annoying but I would worry about wide reaching effects and that is what I would want to check but don't have time for right now.

overall agree that we could spec this better. Would like to hear from @littledan and @syg and since I am probably not the best person to do a deep dive right now, cc @dminor for that potentially if he has time.

[Ms2ger]

I appreciate the work being done here, but have no time to look in detail at the proposed changes, unfortunately.