Closed Bug 1274922 Opened 4 years ago Closed 4 years ago

Make the shell use autoJSAPIOwnsErrorReporting

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

(Depends on 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

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.
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.
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!).
Attachment #8755467 - Flags: review?(jcoppeard)
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.
Attachment #8755469 - Flags: review?(jorendorff)
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.
Attachment #8755470 - Flags: review?(jorendorff)
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.
Attachment #8755471 - Flags: review?(jorendorff)
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()?
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).
Attachment #8755485 - Flags: review?(jorendorff)
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.
Attachment #8755487 - Flags: review?(jorendorff)
This puts the EnvironmentPreparer on the stack, as Ms2ger suggested.
Attachment #8755469 - Attachment is obsolete: true
Attachment #8755469 - Flags: review?(jorendorff)
Attachment #8755493 - Flags: review?(jorendorff)
Attachment #8755467 - Flags: review?(jcoppeard) → review+
Attachment #8755493 - Flags: review?(jorendorff) → review+
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.
Attachment #8755470 - Flags: review?(jorendorff) → review+
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?
Attachment #8755471 - Flags: review?(jorendorff) → review+
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.
Attachment #8755485 - Flags: review?(jorendorff) → review+
Depends on: 1275508
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...
Attachment #8755487 - Flags: review?(jorendorff) → review+
Keywords: leave-open
Pushed by jandemooij@gmail.com:
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
(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.
Morphing this to cover just the shell, will file new bugs for the remaining issues.
Keywords: leave-open
Summary: Always use autoJSAPIOwnsErrorReporting → Make the shell use autoJSAPIOwnsErrorReporting
Some more testing revealed a werror bug.

I'm not happy with these special cases for werror, but one step at a time.
Attachment #8758141 - Flags: review?(jorendorff)
Blocks: 1277278
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...
Attachment #8758141 - Flags: review?(jorendorff) → review+
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44d925279d36
part 7 - Fix a werror buglet. r=jorendorff
Depends on: 1465693
You need to log in before you can comment on or make changes to this bug.