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 an exception"/"report an error"

"Report the exception" gets modified to take an exception value, a global object, and an optional script.

The optional script is necessary to perform muted error checks. (See the current "report an error".) If it's not present then we assume the error is un-muted.

The filename/column number/line number are in fact left unspecified for now; the spec will say something like "initialise them to appropriate values". (In particular, we no longer derive the filename from the script.) 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.

For UA-created exceptions, we need to do some kind of stack-walking backward to the most recent author code on the stack. (See discussion at WICG/webcomponents#547 where this came up, and a simple test case in http://jsbin.com/mediwuwodu/edit?html,console.)

We consolidate "report the exception" and "report an error" as the distinction is now unnecessary and has always been confusing.

Add "guarded" user code invocation to Web IDL

We add a new optional flag to all four user-code-calling Web IDL operations: "guarded". It defaults to being set. (Unless the attribute or operation in question returns a promise type, in which case it is not applicable.)

If the guarded flag is set, then when an exception is thrown during any of:

• converting arguments
• Get()/Set()/Call()
• converting return values

Then we do "report the exception" on the thrown exception, passing in the exception, the entry (maybe???) global, and the callback's script.

Fix all call sites of "report an exception"

Very few places should directly call "report an exception". Most should automatically get the correct behavior with the new default-set "guarded" flag.

In HTML, we have the following call sites, with the following proposed resolutions:

• Custom element callback reactions should switch to using IDL callback types and thus get taken care of automatically by the guarded flag.
• Custom element upgrade reactions will need a bit of care; will have to think on it a bit more.
• Failed module resolution should report to the script's settings object's global, which will be the same as the initiating global for the fetch (except for module workers, where it will be the inner worker global, not the outer global)
• creating a module script will report to the script's settings object's global
• running a classic/module script will report to the entry (???) settings object set during running the script
• EnqueueJob will report to the currently-ambiguous "job settings object" (related to entry)
• Timer functions, when supplied a string: CSP checks probably should not be reporting an exception at all. Needs testing.
• Timer functions, when supplied a function: these currently call IDL "invoke" but seem to omit "report the exception." That seems like a straight bug. Adding default-guarded will fix this.
• Animation frame callbacks will benefit from default-guarded.
• Custom element errors during parsing will be reported to the document's Window.

All other specs I've found so far will benefit from default-guarded, and might need to remove a now-unnecessary "report the exception."

[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.