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

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.

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.

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

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.

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

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

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

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

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.

Some more testing revealed a werror bug. I'm not happy with these special cases for werror, but one step at a time.

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

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/44d925279d36 part 7 - Fix a werror buglet. r=jorendorff