Closed Bug 1409852 Opened 7 years ago Closed 6 years ago

Expose a hook to be informed upon any chrome JS errors on the main thread

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox58 --- wontfix
firefox59 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

(Blocks 4 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

See the conversation on dev-platform: https://groups.google.com/forum/#!topic/mozilla.dev.platform/kuGtQr0GTxU

The idea is to report only ReferenceError, TypeError and SyntaxError, only in chrome code, not in WebExtensions, with the general idea that these errors can only be programmer errors (except perhaps in some tests).

This will serve both:

- to report through Telemetry chrome JS errors found on user's machines (note that we will need to add a filter to remove any private information);
- to fail (crash?) tests in case of such error.

This bug is only about the main thread. We'll need to decide how to extend this to Worker threads and whether this is useful.
Bug 920191 (specifically attachment 8376390 [details] [diff] [review]) already has a harness for mochitest. Is that enough for tests? (I'm guessing we could extend it to xpcshell etc).
That's for uncaught errors, and that would already be a nice step forwards. I'm planning to add platform support for any *thrown* error, which would also work for errors caught or hidden accidentally (e.g. Promise, XPCOM.defineLazyModuleGetter, ...).
Priority: -- → P3
Comment on attachment 8928920 [details]
Bug 1409852 - Expose an API in ChromeUtils to detect chrome JS dev errors;

https://reviewboard.mozilla.org/r/200264/#review206028

::: dom/base/ChromeUtils.h:80
(Diff revision 1)
>                                const nsACString& aString,
>                                const Base64URLDecodeOptions& aOptions,
>                                JS::MutableHandle<JSObject*> aRetval,
>                                ErrorResult& aRv);
> +
> +#if defined(NIGHTLY_BUILD)

I'd prefer #ifdef NIGHTLY_BUILD.

::: dom/base/ChromeUtils.h:83
(Diff revision 1)
>                                ErrorResult& aRv);
> +
> +#if defined(NIGHTLY_BUILD)
> +
> +public:
> +  static RefPtr<AnyCallback> GetJsDevErrorInterceptor(GlobalObject&, JSContext*);

Normally we use already_AddRefed for return values like this if they have to be addrefed.

But does this one even need to be?  I think you can return AnyCallback* here.

::: dom/base/ChromeUtils.cpp:137
(Diff revision 1)
>      return;
>    }
>    aRetval.set(buffer);
>  }
>  
> +#if defined(NIGHTLY_BUILD)

Again, #ifdef NIGHTLY_BUILD.

::: dom/base/test/unit/test_js_dev_error_interceptor.js:99
(Diff revision 1)
> +    run_next_test();
> +});
> +
> +
> +add_test(function test_failing_interceptor() {
> +    // Install am interceptor that will consistently fail.

"an interceptor"

::: dom/bindings/Bindings.conf:1088
(Diff revision 1)
>  'ThreadSafeChromeUtils': {
> -    # The codegen is dumb, and doesn't understand that this interface is only a
> -    # collection of static methods, so we have this `concrete: False` hack.
>      'concrete': False,
>      'headerFile': 'mozilla/dom/ChromeUtils.h',
> +    'implicitJSContext': [ 'jsDevErrorInterceptor' ],

Why do you need this?  The JSContext* argument looks unused to me.

::: dom/webidl/ThreadSafeChromeUtils.webidl:91
(Diff revision 1)
>     */
>    [Throws, NewObject]
>    static ArrayBuffer base64URLDecode(ByteString string,
>                                       Base64URLDecodeOptions options);
> +
> +#ifdef NIGHTLY_BUILD

Note that this file is gone on tip.  All this stuff lives over in ChromeUtils.webidl now.

::: dom/webidl/ThreadSafeChromeUtils.webidl:95
(Diff revision 1)
> +
> +#ifdef NIGHTLY_BUILD
> +  /**
> +   * An interceptor for JS Developer Errors.
> +   *
> +   * If non-null, this interceptor is called whenever JS chrome code

Should probably document that this setting is per-JSRuntime, right?

::: dom/webidl/ThreadSafeChromeUtils.webidl:106
(Diff revision 1)
> +   *
> +   * This mechanism is designed to help detect JS errors in the code
> +   * of Firefox. As it might, in the future, prevent some optimizations
> +   * in SpiderMonkey, it has been decided to restrict it to Nightly builds.
> +   */
> +  // Note: Making this an `AnyCallback` is a hack because adding any kind of

Details, please?  If you can mail me a patch that shows those build errors I can take a look at what's going on.

::: xpcom/base/CycleCollectedJSRuntime.h:310
(Diff revision 1)
>    void RemoveContext(CycleCollectedJSContext* aContext);
>  
> +#if defined(NIGHTLY_BUILD)
> +  // Implementation of JSErrorInterceptor.
> +  void SetJsDevErrorInterceptor(dom::AnyCallback* cb);
> +  RefPtr<dom::AnyCallback> GetJsDevErrorInterceptor();

already_AddRefed or AnyCallback*

::: xpcom/base/CycleCollectedJSRuntime.h:353
(Diff revision 1)
>    struct EnvironmentPreparer : public js::ScriptEnvironmentPreparer {
>      void invoke(JS::HandleObject scope, Closure& closure) override;
>    };
>    EnvironmentPreparer mEnvironmentPreparer;
> +
> +#if defined(NIGHTLY_BUILD)

#ifdef

::: xpcom/base/CycleCollectedJSRuntime.h:355
(Diff revision 1)
>    };
>    EnvironmentPreparer mEnvironmentPreparer;
> +
> +#if defined(NIGHTLY_BUILD)
> +  struct ErrorInterceptor: public JSErrorInterceptor {
> +    virtual void interceptError(JSContext* cx, const JS::Value& val) override;

Is there a reason this is a Value ref, not a Handle?

::: xpcom/base/CycleCollectedJSRuntime.h:360
(Diff revision 1)
> +    virtual void interceptError(JSContext* cx, const JS::Value& val) override;
> +
> +    // A higher-level callback to call if `interceptError` receives a developer
> +    // error (SyntaxError, ReferenceError, TypeError) from chrome code.
> +    //
> +    // FIXME: I'm pretty sure that we need to do something more to help the

I don't think you actually do.

Specifically, as long as the CycleCollectedJSRuntime is alive, the mCallback will be alive.  It will handle tracing the JS object.

The CycleCollectedJSRuntime lifetime does not depend on reference counting or cycle collection, more or less.  At least as long as no one holds on to an nsXPConnect instance from inside the callback...

You do need to make sure to null this out in CycleCollectedJSRuntime::Shutdown.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1561
(Diff revision 1)
>      return context->Runtime();
>    }
>    return nullptr;
>  }
> +
> +#if defined(NIGHTLY_BUILD)

#ifdef

::: xpcom/base/CycleCollectedJSRuntime.cpp:1620
(Diff revision 1)
> +
> +  // Ok, propagate error to client.
> +  ErrorResult rv;
> +  JS::RootedValue arg(cx, exn);
> +  JS::RootedValue retval(cx); // Ignored
> +  mCallback->Call(arg, &retval, rv, nullptr, mozilla::dom::CallbackObject::ExceptionHandling::eRethrowExceptions);

Don't need the "mozilla::" bit.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1623
(Diff revision 1)
> +  JS::RootedValue arg(cx, exn);
> +  JS::RootedValue retval(cx); // Ignored
> +  mCallback->Call(arg, &retval, rv, nullptr, mozilla::dom::CallbackObject::ExceptionHandling::eRethrowExceptions);
> +  rv.WouldReportJSException();
> +
> +  if (rv.Failed()) {

The right way to do this is:

    if (rv.MaybeSetPendingException(cx)) {
      // The call failed.
      return.
    }
    
and then you don't need the rv.WouldReportJSException() bit either, because MaybeSetPendingException() does that for you.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1627
(Diff revision 1)
> +
> +  if (rv.Failed()) {
> +    // The call failed.
> +    // Throw the error without reporting it on the console.
> +    mozilla::Unused << rv.MaybeSetPendingException(cx);
> +    MOZ_ASSERT(JS_IsExceptionPending(cx));

You can't assert that.  If mCallback->Call() threw an uncatchable exception, then we can have no exception pending on the JSContext here...
Attachment #8928920 - Flags: review?(bzbarsky) → review-
Comment on attachment 8928918 [details]
Bug 1409852 - Expose a hook to be informed whenever an exception is thrown;

https://reviewboard.mozilla.org/r/200260/#review206066

::: js/src/jscntxtinlines.h:459
(Diff revision 1)
> +        // If the interceptor fails, it is responsible for calling `setPendingException`.
> +        this->runtime()->errorInterception.interceptor->interceptError(this, v);
> +
> +        this->runtime()->errorInterception.isExecuting = false;
> +
> +        // If the interceptor has somehow raised an exception,

How does this handle uncatchable exceptions from the interceptor?

Seems to me we should simply have the interceptor return a boolean indicating whether it failed...
One other thing that's not clear to me is whether this state should be per-runtime or per-context...  Probably per-runtime is OK as long as we keep making sure multiple contexts per runtime can't run concurrently.
Comment on attachment 8928920 [details]
Bug 1409852 - Expose an API in ChromeUtils to detect chrome JS dev errors;

https://reviewboard.mozilla.org/r/200264/#review206028

> Details, please?  If you can mail me a patch that shows those build errors I can take a look at what's going on.

Add the line

```idl
callback SomeCallback any(any x);
```

anywhere in the file. I seem to remember that it works with other arguments, too.

> Is there a reason this is a Value ref, not a Handle?

For consistency with the `JSContext::setPendingError`. I believe that I could change this, though.

> I don't think you actually do.
> 
> Specifically, as long as the CycleCollectedJSRuntime is alive, the mCallback will be alive.  It will handle tracing the JS object.
> 
> The CycleCollectedJSRuntime lifetime does not depend on reference counting or cycle collection, more or less.  At least as long as no one holds on to an nsXPConnect instance from inside the callback...
> 
> You do need to make sure to null this out in CycleCollectedJSRuntime::Shutdown.

So all JS values held by the callback are already rooted for me?
>callback SomeCallback any(any x);

Assuming you mean |callback SomeCallback = any(any x)|, I see the issue, yeah.  Adding that includes ToJSValue.h in ChromeUtilsBinding.h (to handle the this-conversions for the callback).  Then we end up with an include loop:

  ChromeUtilsBinding.h -> ToJSValue.h -> BindingUtils.h -> nsIDocument.h -> nsIDocumentObserver.h -> StyleSheet.h -> ReferrerPolicy.h -> nsIHttpChannel.h -> nsIChannel.h -> nsILoadInfo.h -> BasePrincipal.h -> OriginAttributes.h -> ChromeUtils.h -> ChromeUtilsBinding.h

and various stuff ends up not included, not defined, etc...

In any case, if you do just want the AnyCallback signature, using AnyCallback is the right thing to do in the first place.

> For consistency with the `JSContext::setPendingError`

Ah, OK.  Worth documenting, including on setPendingException, so if someone changes that one they remember to change this one.

> So all JS values held by the callback are already rooted for me?

Yep.  See the mozilla::HoldJSObjects calls in CallbackObject::Init and CallbackObject::FinishSlowJSInitIfMoreThanOneOwner (the latter is probably what actually runs here).
Comment on attachment 8928918 [details]
Bug 1409852 - Expose a hook to be informed whenever an exception is thrown;

https://reviewboard.mozilla.org/r/200260/#review206354

Sorry for the delay! This makes sense overall, just some nits/comments.

::: js/src/jsapi-tests/testErrorInterceptor.cpp:11
(Diff revision 1)
> +
> +// Tests for JS_GetErrorInterceptorCallback and JS_SetErrorInterceptorCallback.
> +
> +
> +namespace {
> +    const double EXN_VALUE = 3.14;

Style nit: we don't indent namespaces. For instance https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/js/src/frontend/TokenStream.cpp#171-173

::: js/src/jsapi.cpp:710
(Diff revision 1)
> +{
> +    return rt->errorInterception.interceptor;
> +}
> +
> +JS_PUBLIC_API(Maybe<JSExnType>)
> +JS_GetErrorType(const JS::Value& val) {

Nit: { on its own line, use 4 space indent below.

::: js/src/jsapi.cpp:712
(Diff revision 1)
> +}
> +
> +JS_PUBLIC_API(Maybe<JSExnType>)
> +JS_GetErrorType(const JS::Value& val) {
> +  if (!val.isObject()) {
> +    fprintf(stderr, "LOG: JS_GetErrorType not an object\n");

We shouldn't print to stderr here or below

::: js/src/jscntxtinlines.h:459
(Diff revision 1)
> +        // If the interceptor fails, it is responsible for calling `setPendingException`.
> +        this->runtime()->errorInterception.interceptor->interceptError(this, v);
> +
> +        this->runtime()->errorInterception.isExecuting = false;
> +
> +        // If the interceptor has somehow raised an exception,

I think it would be nicer if interceptError returned false on error, true otherwise, like (most) other APIs. That also handles the uncatchable exception case better (the one bz mentioned).
Attachment #8928918 - Flags: review?(jdemooij)
After discussing with jandem over IRC, we decided that calling JS code from `setPendingException` was probably *not* a good idea. 

The current plan is to:

- keep the same API in SpiderMonkey, minus fallibility;
- change the Gecko-level API to a method that pops all pending developer errors.

This should still be usable in the test harness. Once we have reached that stage, we'll discuss how to extend this to report through Telemetry and/or Crash in case of pending error.
So, the risk now is that we may end up with the Vector filling and never being emptied.

I can see two ways of handling this:
- maybe we only ever fill the Vector if some flag is set (presumably by the test harness);
- or we actually keep a callback, just making it asynchronous (i.e. trigger a Promise callback).

I'll probably go for the former, as it's simpler for a testing-only feature.
Attachment #8928919 - Attachment is obsolete: true
Comment on attachment 8928918 [details]
Bug 1409852 - Expose a hook to be informed whenever an exception is thrown;

https://reviewboard.mozilla.org/r/200260/#review209898


C/C++ static analysis found 3 defects in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: js/src/jsapi-tests/testErrorInterceptor.cpp:39
(Diff revision 2)
> +
> +BEGIN_TEST(testErrorInterceptor)
> +{
> +    // Run the following snippets.
> +    const char* SAMPLES[] = {
> +        "throw new Error('I am an Error')\0"

Warning: Suspicious string literal, probably missing a comma [clang-tidy: misc-suspicious-missing-comma]

::: js/src/jsapi-tests/testErrorInterceptor.cpp:50
(Diff revision 2)
> +        "foo[0]\0",
> +        "b[\0",
> +    };
> +    // With the simpleInterceptor, we should end up with the following error:
> +    const char* TO_STRING[] = {
> +        "Error: I am an Error\0"

Warning: Suspicious string literal, probably missing a comma [clang-tidy: misc-suspicious-missing-comma]

::: js/src/jsapi-tests/testErrorInterceptor.cpp:70
(Diff revision 2)
> +
> +    // Test without callback.
> +    JS_SetErrorInterceptorCallback(cx->runtime(), nullptr);
> +    CHECK(gLatestMessage == nullptr);
> +
> +    for (size_t i = 0; i < mozilla::ArrayLength(SAMPLES); ++i) {

Warning: Use range-based for loop instead [clang-tidy: modernize-loop-convert]
Comment on attachment 8928918 [details]
Bug 1409852 - Expose a hook to be informed whenever an exception is thrown;

https://reviewboard.mozilla.org/r/200260/#review210606

::: js/src/jsapi-tests/testErrorInterceptor.cpp:7
(Diff revision 3)
> +
> +#include "jsapi-tests/tests.h"
> +
> +#include "vm/StringBuffer.h"
> +
> +// Tests for JS_GetErrorInterceptorCallback and JS_SetErrorInterceptorCallback.

This test file also needs the Nightly build #ifdef/#endif

::: js/src/jsapi.cpp:714
(Diff revision 3)
> +
> +JS_PUBLIC_API(Maybe<JSExnType>)
> +JS_GetErrorType(const JS::Value& val)
> +{
> +    // All errors are objects.
> +      if (!val.isObject())

The indentation here looks off (tabs?)
Attachment #8928918 - Flags: review?(jdemooij) → review+
Assignee: nobody → dteller
Comment on attachment 8928920 [details]
Bug 1409852 - Expose an API in ChromeUtils to detect chrome JS dev errors;

https://reviewboard.mozilla.org/r/200264/#review212638

::: dom/base/ChromeUtils.cpp:444
(Diff revision 4)
> +ChromeUtils::GetRecentJSDevError(GlobalObject& aGlobal,
> +                                JS::MutableHandleValue aRetval)
> +{
> +  aRetval.setUndefined();
> +  auto runtime = CycleCollectedJSRuntime::Get();
> +  if (!runtime) {

This would be _super_ surprising in any situation in which you managed to get your hands on a ChromeUtils!

::: dom/base/ChromeUtils.cpp:448
(Diff revision 4)
> +  auto runtime = CycleCollectedJSRuntime::Get();
> +  if (!runtime) {
> +    return;
> +  }
> +
> +  auto cx = CycleCollectedJSContext::Get();

If you just want this so you can get a JSContext, then you should just use aGlobal.Context().  That will be faster and is more obviously in a sane compartment (which GetRecentDevError depends on anyway).

::: dom/base/ChromeUtils.cpp:456
(Diff revision 4)
> +  }
> +
> +  JS::Rooted<Maybe<JS::Value>> result(cx->Context());
> +  runtime->GetRecentDevError(cx->Context(), &result);
> +
> +  if (result.get().isSome()) {

result.get() can probably be used here without the explicit isSome(), since Maybe has an operator bool.

But I do have to ask: Why not just have the CycleCollectedJSRuntime method hand out a Value instead of a `Maybe<Value>` plus branching here?

::: dom/base/ChromeUtils.cpp:469
(Diff revision 4)
> +
> +/* static */ void
> +ChromeUtils::ClearRecentJSDevError(GlobalObject&)
> +{
> +  auto runtime = CycleCollectedJSRuntime::Get();
> +  if (!runtime) {

Again, this would be super-surprising.  Like "shouldn't happen".  I don't think we need this check...

::: dom/webidl/ChromeUtils.webidl:124
(Diff revision 4)
> +  /**
> +   * Reset `recentJSDevError` to `undefined` for the current JSRuntime.
> +   */
> +  void clearRecentJSDevError();
> +
> +#endif // NIGHTLY_BUILD

Please add a blank line after the #endif (and maybe remove the one before it).

::: dom/webidl/ChromeUtils.webidl:130
(Diff revision 4)
> +
> +

Why add these two blank lines?

::: xpcom/base/CycleCollectedJSRuntime.h:378
(Diff revision 4)
>      void invoke(JS::HandleObject scope, Closure& closure) override;
>    };
>    EnvironmentPreparer mEnvironmentPreparer;
> +
> +#ifdef NIGHTLY_BUILD
> +

Drop this blank line, please.

::: xpcom/base/CycleCollectedJSRuntime.h:382
(Diff revision 4)
> +#ifdef NIGHTLY_BUILD
> +
> +  // Implementation of the error interceptor.
> +  // Built on nightly only to avoid any possible performance impact on release
> +
> +  struct ErrorInterceptor final: public JSErrorInterceptor {

Space after "final"

::: xpcom/base/CycleCollectedJSRuntime.h:389
(Diff revision 4)
> +    void Shutdown(JSRuntime* rt);
> +
> +    // Copy of the details of the exception.
> +    // We store this rather than the exception itself to avoid dealing with complicated
> +    // garbage-collection scenarios, e.g. a JSContext being killed while we still hold
> +    // onto an exception launched from it.

s/launched/thrown/?

::: xpcom/base/CycleCollectedJSRuntime.cpp:579
(Diff revision 4)
>    JS_RemoveExtraGCRootsTracer(cx, TraceGrayJS, this);
>  }
>  
>  CycleCollectedJSRuntime::~CycleCollectedJSRuntime()
>  {
> +#ifdef MOZ_JS_DEV_ERROR_INTERCEPTOR

This seems a bit redundant with the code in Shutdown...

::: xpcom/base/CycleCollectedJSRuntime.cpp:1572
(Diff revision 4)
>  }
> +
> +#ifdef MOZ_JS_DEV_ERROR_INTERCEPTOR
> +
> +namespace js {
> +extern void DumpValue(const JS::Value& val);

Why is this here?  It shouldn't be.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1642
(Diff revision 4)
> +
> +void
> +CycleCollectedJSRuntime::GetRecentDevError(JSContext*cx, JS::MutableHandle<Maybe<JS::Value>> error)
> +{
> +  if (!mErrorInterceptor.mThrownError.isSome()) {
> +    error.get().reset();

So here we would just set error to undefined if it were a `JS::MutableHandle<JS::Value>`.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1648
(Diff revision 4)
> +    return;
> +  }
> +
> +  // Create a copy of the exception.
> +  JS::RootedObject obj(cx, JS_NewPlainObject(cx));
> +  MOZ_ASSERT(obj);

You can't assert this; you could be OOM.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1650
(Diff revision 4)
> +
> +  // Create a copy of the exception.
> +  JS::RootedObject obj(cx, JS_NewPlainObject(cx));
> +  MOZ_ASSERT(obj);
> +  if (!obj) {
> +    return;

This leaves an exception hanging out on cx.  That seems bogus.  Need to either throw that exception out of here to the ChromeUtils caller, or clear it or something.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1656
(Diff revision 4)
> +  }
> +
> +  const auto FLAGS = JSPROP_READONLY | JSPROP_ENUMERATE | JSPROP_PERMANENT;
> +
> +  // Handle field `message`.
> +  JS::RootedString message(cx, JS_NewUCStringCopyZ(cx, mErrorInterceptor.mThrownError->mMessage.get()));

I would really prefer you used ToJSValue here.  It will handle things like embedded nulls, various optimizations, etc, etc.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1657
(Diff revision 4)
> +
> +  const auto FLAGS = JSPROP_READONLY | JSPROP_ENUMERATE | JSPROP_PERMANENT;
> +
> +  // Handle field `message`.
> +  JS::RootedString message(cx, JS_NewUCStringCopyZ(cx, mErrorInterceptor.mThrownError->mMessage.get()));
> +  MOZ_ASSERT(message);

Can't assert this.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1659
(Diff revision 4)
> +
> +  // Handle field `message`.
> +  JS::RootedString message(cx, JS_NewUCStringCopyZ(cx, mErrorInterceptor.mThrownError->mMessage.get()));
> +  MOZ_ASSERT(message);
> +  if (!message) {
> +    return;

Leaving a submarine exception on the JSContext.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1663
(Diff revision 4)
> +  if (!message) {
> +    return;
> +  }
> +
> +  bool result = JS_DefineProperty(cx, obj, "message", message, FLAGS);
> +  MOZ_ASSERT(result);

You can't assert this.  You could be OOM.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1665
(Diff revision 4)
> +  }
> +
> +  bool result = JS_DefineProperty(cx, obj, "message", message, FLAGS);
> +  MOZ_ASSERT(result);
> +  if (!result) {
> +    return;

This is leaving a submarine exception on the JSContext.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1669
(Diff revision 4)
> +  if (!result) {
> +    return;
> +  }
> +
> +  // Handle field `fileName`.
> +  JS::RootedString filename(cx, JS_NewStringCopyZ(cx, mErrorInterceptor.mThrownError->mFilename.get()));

Just so we're clear, this will corrupt non-ASCII filenames, right?  nsContentUtils::ExtractErrorValues assumes filenames are UTF-8, but this code is just byte-inflating them...

I don't know why ExtractErrorValues hands out the filename as a CString, since that's not what anything ever stores it as under the hood of the things it's working with.  Might be worth it to just change it to output an nsString in a patch under this one.  And then this too could be ToJSValue.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1670
(Diff revision 4)
> +    return;
> +  }
> +
> +  // Handle field `fileName`.
> +  JS::RootedString filename(cx, JS_NewStringCopyZ(cx, mErrorInterceptor.mThrownError->mFilename.get()));
> +  MOZ_ASSERT(filename);

Can't do that.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1672
(Diff revision 4)
> +
> +  // Handle field `fileName`.
> +  JS::RootedString filename(cx, JS_NewStringCopyZ(cx, mErrorInterceptor.mThrownError->mFilename.get()));
> +  MOZ_ASSERT(filename);
> +  if (!filename) {
> +    return;

Submarine exception.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1689
(Diff revision 4)
> +  if (!result) {
> +    return;
> +  }
> +
> +  // Handle field `stack`.
> +  JS::RootedString stack(cx, JS_NewStringCopyZ(cx, mErrorInterceptor.mThrownError->mStack.get()));

Again, this will corrupt the stack if it contains non-ASCII bits, right?

::: xpcom/base/CycleCollectedJSRuntime.cpp:1690
(Diff revision 4)
> +    return;
> +  }
> +
> +  // Handle field `stack`.
> +  JS::RootedString stack(cx, JS_NewStringCopyZ(cx, mErrorInterceptor.mThrownError->mStack.get()));
> +  MOZ_ASSERT(stack);

Can't assert that.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1692
(Diff revision 4)
> +
> +  // Handle field `stack`.
> +  JS::RootedString stack(cx, JS_NewStringCopyZ(cx, mErrorInterceptor.mThrownError->mStack.get()));
> +  MOZ_ASSERT(stack);
> +  if (!stack) {
> +    return;

Need to handle the exception.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1695
(Diff revision 4)
> +  MOZ_ASSERT(stack);
> +  if (!stack) {
> +    return;
> +  }
> +
> +  result = JS_DefineProperty(cx, obj, "stack", stack, FLAGS);

What might make sense to do is to get all your pieces of data set up, then do:

    if (!JS_DefinePropety(cx, obj, prop1, val1, FLAGS) ||
        !JS_DefinePropety(cx, obj, prop1, val1, FLAGS) ||
       ....) {
     // Handle the exception on cx.
    }
Attachment #8928920 - Flags: review?(bzbarsky) → review-
Comment on attachment 8928920 [details]
Bug 1409852 - Expose an API in ChromeUtils to detect chrome JS dev errors;

https://reviewboard.mozilla.org/r/200264/#review212638

> This would be _super_ surprising in any situation in which you managed to get your hands on a ChromeUtils!

Want me to MOZ_ASSERT it?

> This seems a bit redundant with the code in Shutdown...

Not entirely, see bug 1422316.

> You can't assert this; you could be OOM.

What would you do on OOM? Just return `undefined` and proceed as normal? Since this is a Nightly-only feature designed for testing, I figured that we could afford to assert. But I can change that if you prefer.
> Want me to MOZ_ASSERT it?

Sure, that works.

> Not entirely, see bug 1422316.

Ugh.  :(  Looks broken to me!

> What would you do on OOM?

Your options are to either return undefined or throw the OOM out to the scripted caller.  Note that "OOM" in this case would just mean "out of JS heap limit", maybe temporarily, not necessarily out of actual memory...
Comment on attachment 8928920 [details]
Bug 1409852 - Expose an API in ChromeUtils to detect chrome JS dev errors;

https://reviewboard.mozilla.org/r/200264/#review214718

r+ with the nits fixed.

::: dom/base/ChromeUtils.cpp:443
(Diff revision 5)
> +/* static */ void
> +ChromeUtils::GetRecentJSDevError(GlobalObject& aGlobal,
> +                                JS::MutableHandleValue aRetval,
> +                                ErrorResult& aRv)
> +{
> +  aRetval.setUndefined();

No need for this.  aRetval is ignored if aRv is not success, and you set it in success conditions.

::: dom/base/ChromeUtils.cpp:449
(Diff revision 5)
> +  auto runtime = CycleCollectedJSRuntime::Get();
> +  MOZ_ASSERT(runtime);
> +
> +  auto cx = aGlobal.Context();
> +  JS::Rooted<JS::Value> result(cx);
> +  if (!runtime->GetRecentDevError(cx, &result)) {

Actually, you could just drop "result" and pass aRetval as the second arg here, I would think.

::: dom/base/ChromeUtils.cpp:450
(Diff revision 5)
> +  MOZ_ASSERT(runtime);
> +
> +  auto cx = aGlobal.Context();
> +  JS::Rooted<JS::Value> result(cx);
> +  if (!runtime->GetRecentDevError(cx, &result)) {
> +    aRv.Throw(NS_ERROR_FAILURE);

aRv.NoteJSContextException(cx), please.  It will handle some edge cases better than throwing a different error on aRv.

::: xpcom/base/CycleCollectedJSRuntime.h:332
(Diff revision 5)
>  
>    void AddContext(CycleCollectedJSContext* aContext);
>    void RemoveContext(CycleCollectedJSContext* aContext);
>  
> +#ifdef NIGHTLY_BUILD
> +  bool GetRecentDevError(JSContext* aContext, JS::MutableHandle<JS::Value> error);

aError for the second arg.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1585
(Diff revision 5)
> +}
> +
> +/* virtual */ void
> +CycleCollectedJSRuntime::ErrorInterceptor::interceptError(JSContext* cx, const JS::Value& exn)
> +{
> +  if (mThrownError.isSome()) {

Why do you need the isSome() here?  Should be able to just `if (mThrownError)`.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1590
(Diff revision 5)
> +  if (mThrownError.isSome()) {
> +    // We already have an error, we don't need anything more.
> +    return;
> +  }
> +
> +  if (!nsContentUtils::ThreadsafeIsCallerChrome()) {

Might be a bit faster to use nsContentUtils::ThreadsafeIsSystemCaller(cx).  Not sure how performance-sensitive this code is.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1596
(Diff revision 5)
> +    // We are only interested in chrome code.
> +    return;
> +  }
> +
> +  const auto type = JS_GetErrorType(exn);
> +  if (!type.isSome()) {

`if (!type)`

::: xpcom/base/CycleCollectedJSRuntime.cpp:1629
(Diff revision 5)
> +  nsContentUtils::ExtractErrorValues(cx, value, details.mFilename, &details.mLine, &details.mColumn, details.mMessage);
> +
> +  nsAutoCString stack;
> +  JS::UniqueChars buf = JS::FormatStackDump(cx, nullptr, /* showArgs = */ false, /* showLocals = */ false, /* showThisProps = */ false);
> +  stack.Append(buf.get());
> +  details.mStack = NS_ConvertUTF8toUTF16(stack);

CopyUTF8toUTF16(buf.get(), details.mStack);
    
so we don't do the two potential extra copies (one into "stack" and one out of the NS_ConvertUTF8toUTF16) that this code has right now.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1643
(Diff revision 5)
> +}
> +
> +bool
> +CycleCollectedJSRuntime::GetRecentDevError(JSContext*cx, JS::MutableHandle<JS::Value> error)
> +{
> +  if (!mErrorInterceptor.mThrownError.isSome()) {

Shouldn't need the isSome().

::: xpcom/base/CycleCollectedJSRuntime.cpp:1644
(Diff revision 5)
> +
> +bool
> +CycleCollectedJSRuntime::GetRecentDevError(JSContext*cx, JS::MutableHandle<JS::Value> error)
> +{
> +  if (!mErrorInterceptor.mThrownError.isSome()) {
> +    error.get().setUndefined();

Shouldn't need the .get() here.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1658
(Diff revision 5)
> +
> +  JS::RootedValue message(cx);
> +  JS::RootedValue filename(cx);
> +  JS::RootedValue stack(cx);
> +  if (!ToJSValue(cx, mErrorInterceptor.mThrownError->mMessage, &message)
> +   || !ToJSValue(cx, mErrorInterceptor.mThrownError->mFilename, &filename)

I think current style is `||` at end of previous line.

::: xpcom/base/CycleCollectedJSRuntime.cpp:1666
(Diff revision 5)
> +  }
> +
> +  // Build the object.
> +  const auto FLAGS = JSPROP_READONLY | JSPROP_ENUMERATE | JSPROP_PERMANENT;
> +  if (!JS_DefineProperty(cx, obj, "message", message, FLAGS)
> +    | !JS_DefineProperty(cx, obj, "fileName", filename, FLAGS)

`||`, not `|` and same in the rest of this if statement.
Attachment #8928920 - Flags: review?(bzbarsky) → review+
Comment on attachment 8936527 [details]
Bug 1409852 - nsContentUtils::ExtractErrorValues with nsAString;

https://reviewboard.mozilla.org/r/207202/#review214720

r=me, but please do file a followup to convert consumers to the new API.  Most of them would actually be simpler that way, as far as I can tell.

::: dom/base/nsContentUtils.cpp:11035
(Diff revision 1)
>                         nsString& aMessageOut)
>  {
>    RefPtr<T> exn;
>    MOZ_TRY((UnwrapObject<PrototypeID, NativeType>(aObj, exn)));
>  
>    nsAutoString filename;

Why do you need the "filename" temporary?  Why can't you just GetFilename(aSourceSpecOut) and then check tht for emptiness?

::: dom/base/nsContentUtils.cpp:11054
(Diff revision 1)
>    return Ok();
>  }
>  
> +template <prototypes::ID PrototypeID, class NativeType, typename T>
> +static Result<Ok, nsresult>
> +ExtractExceptionValues(JSContext* aCx,

Why do you need this?  The only caller of ExtractExceptionValues is ExtractErrorValues, and your nsACString version of the latter doesn't call ExtractExceptionValues.
Attachment #8936527 - Flags: review?(bzbarsky) → review+
Comment on attachment 8936527 [details]
Bug 1409852 - nsContentUtils::ExtractErrorValues with nsAString;

https://reviewboard.mozilla.org/r/207202/#review214720

Field as bug 1426614.
Comment on attachment 8928920 [details]
Bug 1409852 - Expose an API in ChromeUtils to detect chrome JS dev errors;

https://reviewboard.mozilla.org/r/200264/#review214718

> aRv.NoteJSContextException(cx), please.  It will handle some edge cases better than throwing a different error on aRv.

Ah, thanks, I didn't find this method.

> Might be a bit faster to use nsContentUtils::ThreadsafeIsSystemCaller(cx).  Not sure how performance-sensitive this code is.

Not too sensitive, but let's fix that.

> `||`, not `|` and same in the rest of this if statement.

Ouch, thanks for catching that.
Comment on attachment 8928920 [details]
Bug 1409852 - Expose an API in ChromeUtils to detect chrome JS dev errors;

https://reviewboard.mozilla.org/r/200264/#review212638

> Just so we're clear, this will corrupt non-ASCII filenames, right?  nsContentUtils::ExtractErrorValues assumes filenames are UTF-8, but this code is just byte-inflating them...
> 
> I don't know why ExtractErrorValues hands out the filename as a CString, since that's not what anything ever stores it as under the hood of the things it's working with.  Might be worth it to just change it to output an nsString in a patch under this one.  And then this too could be ToJSValue.

Good catch, I assumed that it was UTF-8-friendly, but I was apparently wrong.

In `ExtractErrorValues`, it uses `CopyUTF16toUTF8`, which doesn't sound very useful.
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 93192fd46b9e -d 106a1cbeb62c: rebasing 440026:93192fd46b9e "Bug 1409852 - nsContentUtils::ExtractErrorValues with nsAString;r=bz"
merging dom/base/nsContentUtils.cpp
merging dom/base/nsContentUtils.h
rebasing 440027:062f380a175d "Bug 1409852 - Expose a hook to be informed whenever an exception is thrown;r=jandem"
merging js/src/jsapi-tests/moz.build
merging js/src/jsapi.cpp
merging js/src/jsapi.h
merging js/src/vm/Runtime.h
warning: conflicts while merging js/src/jsapi-tests/moz.build! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/649d7bdf80ad
nsContentUtils::ExtractErrorValues with nsAString;r=bz
https://hg.mozilla.org/integration/autoland/rev/46fce9a2622d
Expose a hook to be informed whenever an exception is thrown;r=jandem
https://hg.mozilla.org/integration/autoland/rev/06368bf1a32c
Expose an API in ChromeUtils to detect chrome JS dev errors;r=bz
Also started failing on this push:
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=06368bf1a32c75852fe4fc940789b5d5af728805&selectedJob=152911452
The dt*, gl*, J* jobs on Windows 7 debug and Windows 10x64 debug have the same signature crash:
  [@ JS_SetErrorInterceptorCallback(JSRuntime *,JSErrorInterceptor *)]
Example log:
https://treeherder.mozilla.org/logviewer.html#?job_id=152911559&repo=autoland&lineNumber=3194
https://treeherder.mozilla.org/logviewer.html#?job_id=152911459&repo=autoland&lineNumber=9339

 INFO -  mozcrash Saved app info as Z:\task_1513862982\build\blobber_upload_dir\af880fb1-8e69-46cc-a676-c320314196b7.extra
9339
13:44:31  WARNING -  PROCESS-CRASH | dom/canvas/test/webgl-conf/generated/test_conformance__ogles__GL__log2__log2_009_to_012.html | application crashed [@ JS_SetErrorInterceptorCallback(JSRuntime *,JSErrorInterceptor *)]
9340
13:44:31     INFO -  Crash dump filename: c:\users\genericworker\appdata\local\temp\tmphyvsep.mozrunner\minidumps\af880fb1-8e69-46cc-a676-c320314196b7.dmp
Investigating, thanks.
Flags: needinfo?(dteller)
Pushed by dteller@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99508707de1e
nsContentUtils::ExtractErrorValues with nsAString;r=bz
https://hg.mozilla.org/integration/autoland/rev/0ea178ea953a
Expose a hook to be informed whenever an exception is thrown;r=jandem
https://hg.mozilla.org/integration/autoland/rev/d49a1de5d569
Expose an API in ChromeUtils to detect chrome JS dev errors;r=bz
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: