1274922 - Make the shell use autoJSAPIOwnsErrorReporting

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

IIUC Gecko now uses this everywhere, so if we convert the remaining places (JS shell, PAC runtime, etc) we should be able to remove/refactor a bunch of complicated code.

Comment 1

[Jan de Mooij [:jandem]]

I have this working for the JS shell (jit-tests and jstests pass). There are some tricky bits, so I'm going to split this into some smaller patches.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Remove ShellRuntime::gotError

ShellRuntime::gotError is only used for one assertion, and especially with the other patches I'll attach it's not worth keeping around.

This also lets us get rid of the OOM callback (and kills off another JS_IsRunning call!).

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Give the shell its own EnvironmentPreparer

This matches Gecko and is necessary to work with the new error reporting mechanism.

After this we can *require* the embedding to use this - that will hopefully let us remove JS_ReportPendingException.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Don't call the error reporter if autoJSAPIOwnsErrorReporting

In ReportError we have some code to not CallErrorReporter if ErrorToException returns false and autoJSAPIOwnsErrorReporting is true.

This patch adds similar logic to CompileError::throwError.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Fix a test

This test uses werror and with later patches we now correctly throw an exception for the "Successfully compiled asm.js code" warning. We can just ignore it.

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8755469 [details] [diff] [review]
Part 2 - Give the shell its own EnvironmentPreparer

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

::: js/src/shell/js.cpp
@@ +180,5 @@
> +            js::SetScriptEnvironmentPreparer(JS_GetRuntime(cx), this);
> +        }
> +        void invoke(JS::HandleObject scope, Closure& closure) override;
> +    };
> +    EnvironmentPreparer environmentPreparer;

Having this field feels kinda strange. Is there a reason not to put it on the stack in main()?

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Remove JSEXN_NONE

ErrorToException returns if the exception type is JSEXN_NONE, and the caller has to call the error reporter manually, but with autoJSAPIOwnsErrorReporting we can't do that as the error reporter is only used for warnings.

Unfortunately we do use JSEXN_NONE for some errors/warnings. This patch removes JSEXN_NONE and replaces it with either JSEXN_ERR or JSEXN_WARN (a new value I added, but it's hopefully a little saner than JSEXN_NONE).

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Rewrite the shell's error reporting

This patch makes the shell use dontReportUncaught and autoJSAPIOwnsErrorReporting.

The error reporter is only used for warnings now, so I renamed it to WarningReporter.

An AutoReportException class is used to report pending exceptions.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Give the shell its own EnvironmentPreparer

This puts the EnvironmentPreparer on the stack, as Ms2ger suggested.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8755470 [details] [diff] [review]
Part 3 - Don't call the error reporter if autoJSAPIOwnsErrorReporting

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

::: js/src/frontend/TokenStream.cpp
@@ +584,5 @@
> +    // Like ReportError, don't call the error reporter if the embedding is
> +    // responsible for handling exceptions. In this case the error reporter
> +    // must only be used for warnings.
> +    if (cx->options().autoJSAPIOwnsErrorReporting() && !JSREPORT_IS_WARNING(report.flags))
> +        return;

ErrorToException is truly some of our worst code. This is fine for now ... but I hope we're moving toward removing the special cases in ErrorToException which necessitate this.

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 8755471 [details] [diff] [review]
Part 4 - Fix a test

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

r=me but do we really want this? Should we instead delete either this warning or the werror option?

Or: do you have a plan for how to treat this warning in the long run?

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8755485 [details] [diff] [review]
Part 5 - Remove JSEXN_NONE

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

r=me with one comment addressed

::: js/src/vm/ErrorObject.h
@@ +63,1 @@
>          MOZ_ASSERT(type < JSEXN_LIMIT);

This should only go up to JSEXN_WARN, right?

The array of classes is defined in jsexn.cpp.

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 8755487 [details] [diff] [review]
Part 6 - Rewrite the shell's error reporting

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

r=me, no changes necessary.

::: js/src/shell/js.cpp
@@ +5988,5 @@
>  
> +js::shell::AutoReportException::~AutoReportException()
> +{
> +    if (!JS_IsExceptionPending(cx))
> +        return;

It bothers me a little bit that there's so much code here, and it's so arbitrary-looking, but I don't see anything clean that should obviously be factored out and moved to PrintError or somewhere else...

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/511ad37f594b
https://hg.mozilla.org/integration/mozilla-inbound/rev/650ba00927b4

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/511ad37f594b
https://hg.mozilla.org/mozilla-central/rev/650ba00927b4

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/72383ac6936f
part 3 - Fix CompileError::throwError to not call the error reporter if autoJSAPIOwnsErrorReporting. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/502b474b0d6b
part 4 - Fix a werror test to ignore the asm.js warning. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2eb560ec7c8
part 5 - Remove JSEXN_NONE and add JSEXN_WARN. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5ff0cdea30
part 6 - Rewrite the shell's error reporting to handle exceptions in the embedding. r=jorendorff

Comment 17

[Jan de Mooij [:jandem]]

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> ErrorToException is truly some of our worst code. This is fine for now ...
> but I hope we're moving toward removing the special cases in
> ErrorToException which necessitate this.

Agreed.

(In reply to Jason Orendorff [:jorendorff] from comment #11)
> r=me but do we really want this? Should we instead delete either this
> warning or the werror option?
> 
> Or: do you have a plan for how to treat this warning in the long run?

I think we want to remove werror at some point, filed bug 1275508.

(In reply to Jason Orendorff [:jorendorff] from comment #12)
> This should only go up to JSEXN_WARN, right?

Good point, done.

Comment 18

[Jan de Mooij [:jandem]]

Morphing this to cover just the shell, will file new bugs for the remaining issues.

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Fix werror buglet

Some more testing revealed a werror bug.

I'm not happy with these special cases for werror, but one step at a time.

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/72383ac6936f
https://hg.mozilla.org/mozilla-central/rev/502b474b0d6b
https://hg.mozilla.org/mozilla-central/rev/b2eb560ec7c8
https://hg.mozilla.org/mozilla-central/rev/7a5ff0cdea30

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 8758141 [details] [diff] [review]
Part 7 - Fix werror buglet

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

Nice test to have as long as werror is limping along...

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44d925279d36
part 7 - Fix a werror buglet. r=jorendorff

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44d925279d36