Make autoJSAPIOwnsErrorReporting the default

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(5 attachments)

Once all dependent bugs are fixed, we should do a Try run to double check we use autoJSAPIOwnsErrorReporting everywhere.

If that's green, we can remove the autoJSAPIOwnsErrorReporting and dontReportUncaught options.

After that, we can rename JS_SetErrorReporter to JS_SetWarningReporter and remove a lot of code (AutoLastFrameCheck, JS_ReportPendingException, ReportUncaughtException, etc).
The first bomb. This removes ContextOptions, AutoLastFrameCheck and some related code.

This leaves some stale comments related to the error/warning reporter, but other patches will address that.

 21 files changed, 17 insertions(+), 292 deletions(-)
Attachment #8759994 - Flags: review?(jorendorff)
The main problem was PrepareScriptEnvironmentAndInvoke: if we don't have a scriptEnvironmentPreparer callback we will use JS_ReportPendingException.

I changed this to MOZ_RELEASE_ASSERT we have a scriptEnvironmentPreparer. We could also print to stderr or ignore the exception if you prefer that.
Attachment #8759996 - Flags: review?(jorendorff)
Renames JS_{Get,Set}ErrorReporter to JS::{Get,Set}WarningReporter. It also renames some related code from error to warning.
Attachment #8760000 - Flags: review?(jorendorff)
ErrorToException returns false if the report is a warning. We can change this to an assert, and have the callers check for warnings themselves before calling ErrorToException.
Attachment #8760001 - Flags: review?(jorendorff)
The ErrorToException callers no longer care about the return value, so we can just make ErrorToException's return void. This nicely simplifies things.

That's the last patch for this bug, but there's a lot more we can clean up in followup bugs.
Attachment #8760003 - Flags: review?(jorendorff)
Blocks: 1278193
Luke, do you want to take these reviews? Jason is on PTO this week and it's mostly code removal/simplification.
Flags: needinfo?(luke)
Comment on attachment 8759994 [details] [diff] [review]
Part 1 - Remove ContextOptions

Review of attachment 8759994 [details] [diff] [review]:
-----------------------------------------------------------------

You bet I do.  This is glorious.
Attachment #8759994 - Flags: review?(jorendorff) → review+
Attachment #8759996 - Flags: review?(jorendorff) → review+
Attachment #8760000 - Flags: review?(jorendorff) → review+
Attachment #8760001 - Flags: review?(jorendorff) → review+
Attachment #8760003 - Flags: review?(jorendorff) → review+
Thanks Luke!
Flags: needinfo?(luke)
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaebaccae0a2
part 1 - Remove ContextOptions and make autoJSAPIOwnsErrorReporting the default. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e9f480972b4
part 2 - Remove JS_ReportPendingException and js::ReportUncaughtException. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b42129e45a9
part 3 - Rename error reporter callback to warning reporter, assert it's only used for warnings. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/b605a7bb8c49
part 4 - Remove the warning case from ErrorToException. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd2089fe075e
part 5 - Change ErrorToException's return type from bool to void. r=luke
Duplicate of this bug: 1278193
You need to log in before you can comment on or make changes to this bug.