1277278 - Make autoJSAPIOwnsErrorReporting the default

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

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

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Remove ContextOptions

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(-)

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Remove JS_ReportPendingException and js::ReportUncaughtException

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.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Rename error reporter

Renames JS_{Get,Set}ErrorReporter to JS::{Get,Set}WarningReporter. It also renames some related code from error to warning.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Remove warning case from ErrorToException

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.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Make ErrorToException return void

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.

Comment 6

[Jan de Mooij [:jandem]]

Luke, do you want to take these reviews? Jason is on PTO this week and it's mostly code removal/simplification.

Comment 7

[Luke Wagner [: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.

Comment 8

[Jan de Mooij [:jandem]]

Thanks Luke!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
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

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aaebaccae0a2
https://hg.mozilla.org/mozilla-central/rev/3e9f480972b4
https://hg.mozilla.org/mozilla-central/rev/3b42129e45a9
https://hg.mozilla.org/mozilla-central/rev/b605a7bb8c49
https://hg.mozilla.org/mozilla-central/rev/bd2089fe075e