1063241 - Fix error throwing and warning reporting APIs

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect, P1)

Description

[Terrence Cole [:terrence]]

Attachment: implement_sane_error_api-v0.diff

Split off from bug 981187 comment 3 and 4:


This patch introduces JS::ThrowException, JS::ReportWarning, and JS::ReportStrictWarning. Throw does SendPendingException and report calls the error reporter, modulo WERROR and strict error reporting options.

What is here is a good start, but it is not yet sufficient for what gecko (or the GC) need from it.

Most importantly, the distinction between JSErrorReport, ErrorObject, and ExceptionObject is pretty much totally bogus. I need to spend some "quality" time with our various reporter callbacks to find the right set of properties to expose and how. In particular, both reports and exceptions end up going through the report callback, so it seems silly to lazily create either the exception or the report, much less both. We generally end up creating at least 3 simultaneous copies of the error report string, sometimes more.

Off thread parsing: the current patch is capable of replacing PJS exceptions -- a nice simplification -- but it totally ignores off-thread-parsing errors. These will need attention before I get too much further.

Error args: I don't even. The only real user is gecko's own js.msg, which appears to have copied our own terrible ideas wholesale. This is a pretty high priority for me to kill off. The test that uses them is just insane and needs to die ingloriously.

Uncatchable exceptions: We currently represent these by JSEXN_NONE in the report and no pending exception. This means the API user has to have two error paths, when one should be sufficient.

There are probably other things I've yet to uncover as well. I expect there will be some semantic changes as we convert stuff to use the new algorithm, but I'm not terribly worried about this: our current API is too confusing to use correctly, and thus simply isn't in most places. In the hour or so I've spent using the new API to replace the old, the vast majority of the sites I've replaced have had something subtly wrong with them. We'll see how things pan out as we replace more.

Comment 1

[Terrence Cole [:terrence]]

Attachment: replace_some_reporting-v0.diff

Comment 2

[Bobby Holley (:bholley)]

To summarize our IRC discussion:

The plan is to get rid of error reporting callbacks entirely, and replace them with a simpler mechanism that is only used for warnings. Exceptions will simple bubble up the stack until they reach the top, at which point they are plucked off by machinery that belongs to the embedding (in our case, AutoJSAPI).

Comment 3

[Terrence Cole [:terrence]]

Attachment: implement_sane_error_api-v1.diff

This handles many more cases: PJS errors, throwing id and object parameters, and more temporary backwards compatibility.

Comment 4

[Terrence Cole [:terrence]]

Attachment: replace_some_reporting-v1.diff

Even though we're not removing the old error messages, and the new message definitions are 3 lines instead of 1, we're already breaking even in line count from the added simplicity in the users. Given that this only converts a fraction of the errors, I expect the new api to save us a ton of lines eventually.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8486009 [details] [diff] [review]
implement_sane_error_api-v1.diff

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

OK. Overall, the changes are an improvement.

Feedback, for whatever it's worth: Generally, stupid code has some serious software engineering advantages over smart code. Data tables and CPP macros, for example, have advantages over copy-and-paste subclasses, templates, and SFINAE. They're simpler. Macros are better at DRY than base classes, and are cleaner abstractions -- they do not end up tightly coupled with every single use the way base classes do. This code is really smart. It doesn't seem like the simplest approach that would do the job well.

I know you've said you've already got a 200KB patch and it's too late to change approaches. OK. That's how it is sometimes. I don't have to like every single piece of code that lands. In general, though, I think our codebase tends to be too complex, so I'm always looking for ways it could be simpler.

Separately, the void* is a pre-existing wart --- you already mentioned to me you'd look into possibly getting rid of it. Hope that works out!

::: js/public/CharacterEncoding.h
@@ +62,1 @@
>        : Base(aBytes, aLength)

changes in this file would make a nice separate patch... Utility.h, too, they seem to require a separate explanation. (?)

@@ +85,5 @@
> +    UTF8Chars(unsigned char *aBytes, size_t aLength)
> +      : Base(aBytes, aLength)
> +    {}
> +    UTF8Chars(const unsigned char *aBytes, size_t aLength)
> +      : Base(const_cast<unsigned char *>(aBytes), aLength)

Either UTF8Chars permit modifying the buffer or they don't.

If yes, the constructors that take 'const char *' and such need to be removed.

If no, UTF8Chars needs to inherit from Range<const unsigned char>, not Range<unsigned char>.

Pre-existing issue, I know.

::: js/public/ErrorAPI.h
@@ +148,5 @@
> +// These functions "throw" a script-catchable exception. Script |except| blocks
> +// are able to "catch" these exceptions, inspect the exception object, and
> +// respond as desired in script to the runtime fault. If script does not catch
> +// the exception, SM will return from script with a return value of |false| and
> +// a pending exception set on the context. It is the embeddings responsibility

embedding's

@@ +248,5 @@
> +//
> +// Calls to this API will report iff the compartment options contains
> +// |extraWarnings|. Even though the things we report about in extraWarnings are
> +// generally good style, they are allowed in the spec, so reporting about them
> +// unconditionally would be incorrect.

"...generally good style, they are pretty common even in widely-used JS libraries, so reporting about them unconditionally would be useless and annoying."

@@ +251,5 @@
> +// generally good style, they are allowed in the spec, so reporting about them
> +// unconditionally would be incorrect.
> +template <typename ErrorType, typename ContextType>
> +MOZ_ALWAYS_INLINE bool
> +ReportStrictWarning(ContextType *cx, void *formatData = nullptr,

ExtraWarning, please! Let's phase out the phrase "strict warning"; it confuses people.

Comment 6

[Jason Orendorff [:jorendorff]]

Terrence, I had one more question. Comment 2 sounds pretty great to me, but are we planning to get there in this bug? As far as I can tell from the patch, the idea is to leave dontReportUncaught and AutoLastFrameCheck in place for now.

Comment 7

[Terrence Cole [:terrence]]

I was hoping to wait for Bobby to have the simpler usage in place so that I won't get burned by the insane memory management in the old code while stabilizing the new paths. I'm frankly terrified that if I touch anything relating to JSErrorReporter I'm going to end up having a very bad few weeks. I may need to forge ahead anyway, I'll see where Bobby is at with this next week.

Comment 8

[Terrence Cole [:terrence]]

Comment on attachment 8486012 [details] [diff] [review]
replace_some_reporting-v1.diff

Tom and Jason are right: this could be cleaner and probably deserves to be, even if I don't have the time for it atm.

Comment 9

[Terrence Cole [:terrence]]

Attachment: 1_give_jserrorreport_a_constructor-v0.diff

Well, I think the first step is to make the memory management of the error stack even passingly sane. This patch give JSErrorReport a constructor and removes the ridiculous, manual PodZero we are currently doing.

Comment 10

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/52b2e7d0c652

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/52b2e7d0c652

Comment 13

[Jeff Walden [:Waldo]]

Comment on attachment 8486009 [details] [diff] [review]
implement_sane_error_api-v1.diff

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

::: js/public/ErrorAPI.h
@@ +18,5 @@
> +
> +// Possible exception types. These types are part of a JSErrorFormatString
> +// structure. They define which error to throw in case of a runtime error.
> +// JSEXN_NONE marks an unthrowable error.
> +typedef enum JSExnType {

No need for this typedef nonsense any more.

@@ +85,5 @@
> +    static const char *name() { return "SyntaxError"; }
> +};
> +template <int ArgCount> struct TypeError : public Error<ArgCount> {
> +    const static JSExnType code = JSEXN_TYPEERR;
> +    static const char *name() { return "TypeError"; }

I understand the method-ness of this is to deal with the current js_GetErrorMessage goop currently existing.  We should get rid of that and just have this be a const char[] in a slightly longer run.

Also, static const T name, not const static.  Consistency and all.

@@ +138,5 @@
> +DispatchStrictWarning(ContextType *cx, JSExnType exnType, const UniqueUTF8CharsZ &message);
> +
> +template <typename ContextType>
> +extern JS_PUBLIC_API(bool)
> +DispatchStrictModeError(ContextType *tscx, JSExnType exnType, const UniqueUTF8CharsZ &message);

s/tscx/cx/

@@ +144,5 @@
> +} /* namespace detail */
> +
> +// Error throwing API.
> +//
> +// These functions "throw" a script-catchable exception. Script |except| blocks

So, all of these could use a single example of how to do it.  "typename ErrorType" and "typename ContextType" indicate nothing, by the discovered-after-the-fact nature of SFINAE.  Explain that the former has a particular structure with an example:

    struct OutOfRangeError : JS::RangeError<1>
    {
      public:
        // {}-count is consistent with |1| above
        static const char* message() { return "value {0} is out of range"; }
    };

would do wonders for understanding how to use this.  (This'll get nicer once we require gcc 4.5.  Then, a one-off ErrorType class can be right next to its use, not declared at namespace scope.)

Regarding jorendorff's comments on the code being "clever", I think a solution would be to include an overview comment just before the |struct JSPrincipals| that quickly explains the implementation from a C++ point of view.  Shouldn't need to be that long to explain the idea of what's done.  It's unfortunate how C++'s system says you can't declare the concept that a type parameter implements, other than latently.  :-\  Maybe C++ will have concepts sometime eventually or so.

@@ +151,5 @@
> +// the exception, SM will return from script with a return value of |false| and
> +// a pending exception set on the context. It is the embeddings responsibility
> +// to report, propogate, or ignore the exception before running more script.
> +template <typename ErrorType, typename ContextType>
> +MOZ_ALWAYS_INLINE bool

Just inline should be good enough.  No need for actual inlining everywhere, we don't need speed in reporting stuff.

@@ +190,5 @@
> +    return detail::DispatchException(cx, ErrorType::code,
> +                                   detail::FormatErrorMessage(cx, ErrorType::message(formatData),
> +                                                              detail::StringifyArgument(cx, arg0),
> +                                                              detail::StringifyArgument(cx, arg1),
> +                                                              detail::StringifyArgument(cx, arg2)));

These "parallel" calls could have three separate exceptions be set as pending.  I think we want

  auto a0 = detail::StringifyArgument(cx, arg0);
  if (!a0)
    return false;
  auto a1 = detail::StringifyArgument(cx, arg1);
  if (!a1)
    return false;
  auto a2 = detail::StringifyArgument(cx, arg2);
  if (!a2)
    return false;

to address this, for everything.  (The alternative of having StringifyArgument check cx->exceptingPending() and returning false early seems bad to me, *particularly* as long as stringification order [which would be implementation-defined, per C++ order of evaluation rules] is observable by JS, as it is due to bug 633623.)  Less concise, but I think nicer overall.

@@ +202,5 @@
> +// Reporting a warning itself may, of course, throw an exception either when an
> +// OOM occurs or when running with WERROR.
> +template <typename ErrorType, typename ContextType>
> +MOZ_ALWAYS_INLINE bool
> +ReportWarning(ContextType *cx, void *formatData = nullptr,

"report" is kind of a nebulous verb in SpiderMonkey and JSAPI.  Better perhaps to just have JS::Warning for the name?  Could do Warn, but then there's not a clear parallel for the extra warnings stuff, for which JS::ExtraWarning seems best.

@@ +260,5 @@
> +}
> +
> +template <typename ErrorType, typename ContextType, typename T0>
> +MOZ_ALWAYS_INLINE bool
> +ReportStrictWarning(ContextType *cx, const T0 &arg0, void *formatData = nullptr,

JS::ExtraWarning for this, maybe.

@@ +293,5 @@
> +                                                              detail::StringifyArgument(cx, arg1),
> +                                                              detail::StringifyArgument(cx, arg2)));
> +}
> +
> +// Strict Mode Error API

This should really only be a JS-engine-internal thing.  If there are non-JS callers, add a friend API and file a bug to remove it.  Strict mode is something the JS engine itself should always handle; the setProperty hooks having a strict bool is just a bug/hack.

@@ +300,5 @@
> +// reported in a script with "use strict" they will instead get thrown as
> +// exceptions.
> +template <typename ErrorType, typename ContextType>
> +MOZ_ALWAYS_INLINE bool
> +ReportOrThrowStrictModeError(ContextType *cx, void *formatData = nullptr,

Plain old StrictModeViolation for the name for this, maybe.  Minor preference for no "Error" in this, because it doesn't always complain with an error.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Terrence, are you blocked on something Gecko-side at this point here?

Comment 15

[Terrence Cole [:terrence]]

No, I am not.

Comment 16

[Terrence Cole [:terrence]]

Let's get errormageddon landed and then see what we want to do about generating errors. I like the syntax of this particular approach, but I totally understand why everyone else disagrees. :-)

I think once errormageddon is everywhere, we'll have a much better idea of what exactly error construction should look like to be pleasant to use.