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

Add promise rejection tracking events #224

Merged
merged 1 commit into from
Nov 30, 2015
Merged

Conversation

domenic
Copy link
Member

@domenic domenic commented Oct 2, 2015

This new feature uses a pair of events on global scopes, viz. unhandledrejection and rejectionhandled, to allow authors to track unhandled promise rejections. The specification was originally drafted at https://github.com/domenic/unhandled-rejections-browser-spec in collaboration with @jeisinger and @bzbarsky.

This feature depends on tc39/ecma262#76 and shouldn't be merged until that is merged by @bterlson.

Although I feel pretty confident in the semantics here, given that we worked them out over in that repo (with tests!), I could use some careful editorial review.

@domenic domenic added addition/proposal New features or enhancements do not merge yet Pull request must not be merged per rationale in comment labels Oct 2, 2015
@@ -85710,6 +85712,12 @@ interface <dfn>NavigatorOnLine</dfn> {

</dl>

<p>An <span>environment settings object</span> also has an <dfn>outstanding rejected promises weak set</dfn> and
Copy link
Member

Choose a reason for hiding this comment

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

Wrapping.

@domenic
Copy link
Member Author

domenic commented Oct 5, 2015

Pushed another commit addressing most review comments. I'd rather stick with optional event init unless there is an example of going the other direction.

@annevk
Copy link
Member

annevk commented Oct 6, 2015

@bzbarsky do you have an example of an event dictionary where a parameter is required? I thought we had precedent for that by now.

@bzbarsky
Copy link
Contributor

bzbarsky commented Oct 6, 2015

@domenic
Copy link
Member Author

domenic commented Oct 6, 2015

@bzbarsky @annevk so the use of "optional NotificationEventInit" instead of just "NotificationEventInit" is an error in Notifications? Or does Web IDL allow omitting dictionaries with required members? (I see that it requires that dictionaries without required members are marked optional, but I don't see the inverse requirement...)

@bzbarsky
Copy link
Contributor

bzbarsky commented Oct 6, 2015

so the use of "optional NotificationEventInit" instead of just "NotificationEventInit" is an error in
Notifications?

That's a good question. I'm not quite sure what the intended handling of "optional" dictionary arguments is in Web IDL. I thought it was "handle it as if it had empty dictionary as default value", but what the spec actually says is:

If the type of an argument is a dictionary type or a union type that has a dictionary type as one of its flattened member types, and that dictionary type and its ancestors have no required members, and the argument is either the final argument or is followed only by optional arguments, then the argument MUST be specified as optional. Such arguments are always considered to have a default value of an empty dictionary, unless otherwise specified.

which suggests that in an API like this:

void foo(optional SomeDictionary dict, long x);

the first arg does NOT have empty dictionary as a default value. Instead, if undefined is passed then the argument will be treated as "not passed".

I suspect this is a bug in the spec, and the intent is that in any context in which things could have a default value (optional arguments, dictionary members, etc) we want a dictionary type to have a default value of empty dictionary. @heycam, does that seem reasonable?

The resolution of that will determine what happens with the NotificationEvent stuff. I my proposal of always having empty dictionary as default value is used, then the optional in that spec is silly (in that it's valid IDL, but passing undefined or not passing an argument will simply throw anyway just like it would without the optional, though possibly with a different error message). If, on the other hand, we want to allow a separate "not passed" for optional dictionaries that are not in trailing position or have required members, then the IDL in the NotificationEvent spec means something different from what it would mean without the optional and the spec probably needs to define the behavior if the argument is not passed or is passed as undefined.

@domenic
Copy link
Member Author

domenic commented Oct 6, 2015

OK cool. Removed optional from this PR since it sounds like the direction @annevk is leaning is to disallow omitting that argument.

Also, note to self: I don't think this properly specs cross-origin error censoring. Or if it does, it does so by accident. I need to investigate further.

domenic added a commit to tc39/ecma262 that referenced this pull request Oct 6, 2015
This is a non-observable change that will allow HTML to provide events for tracking promise rejections, as outlined in https://github.com/domenic/unhandled-rejections-browser-spec.

For more information on the original proposal and use cases (mostly on the HTML side), see https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Sep/0024.html.

The corresponding HTML pull request is at whatwg/html#224.
domenic added a commit to tc39/ecma262 that referenced this pull request Oct 6, 2015
This is a non-observable change that will allow HTML to provide events for tracking promise rejections, as outlined in https://github.com/domenic/unhandled-rejections-browser-spec.

For more information on the original proposal and use cases (mostly on the HTML side), see https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Sep/0024.html.

The corresponding HTML pull request is at whatwg/html#224.
@jeisinger
Copy link
Member

Any updates on this?

@domenic
Copy link
Member Author

domenic commented Oct 19, 2015

Any updates on this?

This is somewhat blocked on tc39/ecma262#76, which due to TC39's work mode will need to wait until late November for a face to face discussion. However, I believe we have editor consensus on adding this once that lands.

(To truly do this correctly, we need to fix https://www.w3.org/Bugs/Public/show_bug.cgi?id=25981 first. However my plan is to fudge things a little in the meantime, and get a reference to the currently running script with a bit of hand-waving that will be formalized once that bug is fixed. (The currently running script is necessary for the cross-origin check.))

@bzbarsky
Copy link
Contributor

How is "currently running script" different from "incumbent settings object" in this case?

@domenic
Copy link
Member Author

domenic commented Oct 19, 2015

How is "currently running script" different from "incumbent settings object" in this case?

My comment was about the gymnastics of getting from ES-land to HTML-land. ES-land calls HostPromiseRejectionTracker, and in ES land the relevant primitive is "running execution context". We need to go from there to a settings object, but currently the specs do not talk to each other well enough to do so. The plan for doing that is partially in the linked bugzilla, and partially in tc39/ecma262#78.

@jeisinger
Copy link
Member

about the cross origin scripts - we only filter parsing errors, once the script runs, you get full errors, no?

@zcorpan
Copy link
Member

zcorpan commented Oct 23, 2015

No runtime script errors are also muted.

@jeisinger
Copy link
Member

ah, you're right

@jeisinger
Copy link
Member

remains the question which URL you'd associate with a promise rejection event? the file that contained the code that creates the promise? The file that rejected the promise?

I'm still not convinced we need to censor anything here - once the script runs, you can just access it anyways

@zcorpan
Copy link
Member

zcorpan commented Oct 26, 2015

You can't necessarily read the script itself (e.g. if it's wrapped in an anonymous function and doesn't set any properties accessible to you). If you don't mute exceptions, you can include the script in a document with a different DOM that what the script expects, or otherwise do something to cause an exception, which lets you read the stacktrace. I don't know if there's a concrete attack, but possibly people include secrets like session cookies in scripts if the user is logged in, or company secrets behind IP-based auth, etc, that could be exposed in a stacktrace but not otherwise.

@jeisinger
Copy link
Member

so you're proposing to just censor the "reason" iff the reason is an Error object and it was created in a non-cors cross origin script?

data-x="concept-promise-rejection-nothandled">not handled</span> state of promise rejections
when determining what to display in any debugging interfaces. That is, intercepting an
<code>unhandledrejection</code> event and calling <code>preventDefault()</code> should prevent
the corresponding rejection from showing up in any developer tools.</p>
Copy link
Member

Choose a reason for hiding this comment

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

I think this should use similar wording as for runtime script errors (in the normative paragraph above, not in the note):

If the error is still not handled after this, then the error may be reported to the user.

Also, avoid using should/may/must inside notes, since they are normative keywords but notes are non-normative.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not so sure. It seems like that paragraph for errors is talking about the old days when browsers would pop up an alert()-like thing for script errors. I'm talking about developer tooling instead.

Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure @Hixie also meant devtools/error console when he wrote that. But I wouldn't mind changing the language in both places to be clearer.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @zcorpan about the intent of that phrase. Even back in the old days only Internet Explorer would show such messages to end users in the main UI and everyone considered that bad practice. However, in most browsers users do have access to them if they open the developer tools.

domenic added a commit to tc39/ecma262 that referenced this pull request Nov 23, 2015
This is a non-observable change that will allow HTML to provide events for tracking promise rejections, as outlined in https://github.com/domenic/unhandled-rejections-browser-spec.

For more information on the original proposal and use cases (mostly on the HTML side), see https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Sep/0024.html.

The corresponding HTML pull request is at whatwg/html#224.
@domenic
Copy link
Member Author

domenic commented Nov 23, 2015

@zcorpan @annevk comments addressed! All improvements; thanks for the close critique.

<h6>The HostPromiseRejectionTracker implementation</h6>

<p>ECMAScript contains an implementation-defined HostPromiseRejectionTracker(<var>promise</var>,
<var>operation</var>) abstract operation. <ref spec=ECMA262> User agents must use the following
Copy link
Member

Choose a reason for hiding this comment

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

I think <ref> is consistently put at the end of the paragraph. Other than that, LGTM.

Copy link
Member

Choose a reason for hiding this comment

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

LGTM2

Copy link
Member Author

Choose a reason for hiding this comment

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

I managed to find a single counterexample ("The value must be a valid BCP 47 language tag. [BCP47] ...") but you are in general right. Still, I think it would be too awkward to place it after the colon, so unless you have a better idea I think I'll keep it where it is.

Now just need @bterlson to merge tc39/ecma262#76!

Copy link
Member

Choose a reason for hiding this comment

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

That counterexample seems more like an oversight given the other BCP 47 references. And when you search for : [ you'll find plenty of precedent.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair point. Moved.

This new feature uses a pair of events on global scopes, viz. unhandledrejection and rejectionhandled, to allow authors to track unhandled promise rejections. The specification was originally drafted at https://github.com/domenic/unhandled-rejections-browser-spec in collaboration with @jeisinger and @bzbarsky.

This feature depends on tc39/ecma262#76.
domenic added a commit to tc39/ecma262 that referenced this pull request Nov 30, 2015
This is a non-observable change that will allow HTML to provide events for tracking promise rejections, as outlined in https://github.com/domenic/unhandled-rejections-browser-spec.

For more information on the original proposal and use cases (mostly on the HTML side), see https://lists.w3.org/Archives/Public/public-whatwg-archive/2014Sep/0024.html.

The corresponding HTML pull request is at whatwg/html#224.
@domenic domenic removed the do not merge yet Pull request must not be merged per rationale in comment label Nov 30, 2015
@domenic domenic merged commit 61ccc05 into master Nov 30, 2015
@domenic domenic deleted the unhandledrejections branch November 30, 2015 22:56
domenic added a commit that referenced this pull request Dec 24, 2015
In #224 we had a brief discussion which concluded that these statements about error reporting really referred to a developer console. Alternately, maybe they are holdovers from the early days of the web when browsers did in fact show popups with every JavaScript error. Regardless, this uniformly updates all script error reporting to be to a developer console, instead of to the user.
domenic added a commit that referenced this pull request Dec 24, 2015
In #224 we had a brief discussion which concluded that these statements about error reporting really referred to a developer console. Alternately, maybe they are holdovers from the early days of the web when browsers did in fact show popups with every JavaScript error. Regardless, this uniformly updates all script error reporting to be to a developer console, instead of to the user.
@DanielHerr
Copy link

Could the event be provided with file and line properties?

@domenic
Copy link
Member Author

domenic commented Jul 1, 2016

@DanielHerr a better avenue for that is to add file and line properties to all JavaScript errors, which is something that people are actively working to standardize in JavaScript.

@DanielHerr
Copy link

The problem with that is event.reason may be undefined or not an error object, leaving no way to detect the source of the error.

@domenic
Copy link
Member Author

domenic commented Jul 1, 2016

Right, that's why you should always reject your promises with error objects.

@DanielHerr
Copy link

But it would be useful to catch instances where it is not an error object, just like onunhandledrejection is useful to catch promises that should have been .caught()

@jeisinger
Copy link
Member

I think in Chrome's devtools, we always expose the stack, whether you reject with an error or not

domenic added a commit to web-platform-tests/wpt that referenced this pull request Feb 15, 2017
Follows whatwg/html#224. Tests reviewed upstream in Chromium; small changes made to fit with web platform tests (e.g. avoiding duplicate test names).
pull bot pushed a commit to Mu-L/chromium that referenced this pull request Jun 11, 2021
The changes were merged into the WHATWG HTML Standard in
whatwg/html#224.

Change-Id: I21ca4eebdda22d4138ea844ccd6277b786bd5f36
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2951212
Commit-Queue: Timothy Gu <timothygu@chromium.org>
Auto-Submit: Timothy Gu <timothygu@chromium.org>
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#891344}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
The changes were merged into the WHATWG HTML Standard in
whatwg/html#224.

Change-Id: I21ca4eebdda22d4138ea844ccd6277b786bd5f36
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2951212
Commit-Queue: Timothy Gu <timothygu@chromium.org>
Auto-Submit: Timothy Gu <timothygu@chromium.org>
Reviewed-by: Domenic Denicola <domenic@chromium.org>
Reviewed-by: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#891344}
NOKEYCHECK=True
GitOrigin-RevId: 8ceba2c5ea1d805abaabecfbc4c238141221426d
@RealAlphabet
Copy link

RealAlphabet commented May 31, 2023

Excuse me for bringing this up again, but why doesn't the unhandledrejection handler return information on where the promise was rejected?

I think this would be really necessary in many error monitoring cases where JavaScript primitives or objects (which don't include a stack trace) have caused an error. Even if it's not good practice, it can and often does happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements
Development

Successfully merging this pull request may close these issues.

7 participants