903419 - Report unhandled exceptions to the error console when a Promise is GCed

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Bug 902866 describes the problem we're trying to solve.  For the specific case of DOM promises, Neil Rashbrook suggested we just report to console in our destructor (or presumably in Unlink, if we hit it).

Olli, would there be any lifetime issues with this?  I _think_ we wouldn't create any new references to the exception JS object in the process...  or at least can avoid doing so if we're careful.  I assume that creating such a reference from inside Unlink would not be ok?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

so C++ Promise object would keep a JS exception alive. What if someone has added an expando to the
exception which points to the Promise and then during error reporting someone accesses it?

I may misunderstand the setup though.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> so C++ Promise object would keep a JS exception alive.

It already does.  Promise::mResult.

> What if someone has added an expando to the
> exception which points to the Promise and then during error reporting someone accesses it?

The error report would need to not include the exception object itself.

Basically, the idea as I was conceiving it is that in the dtor or Unlink we check whether we can get a JSErrorReport from mResult (we would need to do this without invoking script) and if so grab the filename/lineno/etc from it and post an event to report it directly to the error console (to make sure we're not running script during unlink and such).  At no point would we give JS access to the exception object itself.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

ok, so then the filename/lineno/etc storing needs to happen when we're unlinking since Promise needs to
be a js holder and nullify all the pointers to JS stuff during unlink.
Can we get filename/etc without using APIs which may GC?
If we can't, we could pass the JS object to the Runnable which then calls error reporter.
The Runnable should either root/unroot or be a JS holder and cycle collectable.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Can we get filename/etc without using APIs which may GC?

I will check on that.

> If we can't, we could pass the JS object to the Runnable which then calls error reporter.

Is it ok, in unlink, to suddenly root a JS object that might have been in the cycle being unlinked (as long as we're not creating new JS refs to it, just C++ roots)?

Comment 5

[Andrew McCreight [:mccr8]]

The problem with that is that any C++ objects held onto the JS object will get unlinked, so we can get use-after-unlink problems.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Yeah, indeed, if the JS keeps C++ alive we end up unlinking that C++...
So we need store all the necessary information before unlink/dtor runs and then report using that.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

But even then, if we just use the JS object to grab filename/etc for reporting, using runnables
should be safe.  (GC would kill the js object anyway async).
Safe, but perhaps error prone.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So.  JS_ErrorFromException does not GC and once we have the JSErrorReport we can get things like filename/lineno off it.  So this is perfectly doable, afaict.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Report unhandled rejections in promises.

Luke, could you look over the jsfriendapi bit?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, and I'm not happy about all the duplication with NS_ScriptErrorReporter, but the behavior is different enough I wasn't sure how to share the code with it.

Comment 11

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

Comment on attachment 796461 [details] [diff] [review]
Report unhandled rejections in promises.

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

::: dom/promise/Promise.cpp
@@ +8,1 @@
>  #include "mozilla/dom/Promise.h"

Don't include anything before this one (maybe add an empty line between this and the others).

@@ +394,5 @@
> +    innerWindowID = win->WindowID();
> +  } else {
> +    innerWindowID = 0;
> +  }
> +  

Trailing ws

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 796461 [details] [diff] [review]
Report unhandled rejections in promises.

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

::: js/src/jsfriendapi.h
@@ +595,5 @@
>  IsObjectInContextCompartment(JSObject *obj, const JSContext *cx);
>  
>  /*
> + * ErrorFromException takes a raw Value so that it's safe to call it during
> + * GC/CC/whatever.  It promises to never ever GC.

I'm not sure why taking a raw Value makes you safer for anything (vs. taking a HandleValue).  Is the problem that the caller can't construct a RootedValue to pass a HandleValue in?  If so, perhaps the comment could be reworded to express this.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Is the problem that the caller can't construct a RootedValue to pass a HandleValue in?

Precisely.

> If so, perhaps the comment could be reworded to express this.

I can do that.

Comment 14

[Bobby Holley (:bholley)]

Comment on attachment 796461 [details] [diff] [review]
Report unhandled rejections in promises.

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

::: dom/promise/Promise.cpp
@@ +274,5 @@
>  Promise::RunTask()
>  {
>    MOZ_ASSERT(mState != Pending);
>  
> +  mHadRejectCallback = mHadRejectCallback || !mRejectCallbacks.IsEmpty();

It would seem much more logical to me to put this in AppendCallbacks. Is there any reason you didn't do it that way?

@@ +290,5 @@
>      callbacks[i]->Call(value);
>    }
>  }
>  
> +class PromiseErrorReporter : public nsRunnable

I think it would be cool for this to be an ancestor of ScriptErrorEvent, and have the latter optionally delegate the console reporting to the superclass. I haven't looked at the nitty-gritty though, so don't feel obligated to do it if it feels onerous.

@@ +365,5 @@
> +  // XXXbz this duplicates some code in NS_ScriptErrorReporter and
> +  // ScriptErrorEvent.
> +  MOZ_ASSERT(mResult.isObject(), "How did we get a JSErrorReport?");
> +  nsIPrincipal *principal = nsJSPrincipals::get(
> +    JS_GetCompartmentPrincipals(js::GetObjectCompartment(&mResult.toObject())));

nsContentUtils::GetObjectPrincipal()

@@ +379,5 @@
> +  }
> +
> +  nsString msg;
> +  if (report->ucmessage) {
> +    // XXXbz No JSContext means we can't call js::GetErrorTypeName

Does this need to be XXXbz?

Comment 15

[Boris Zbarsky [:bzbarsky]]

> It would seem much more logical to me to put this in AppendCallbacks.

I considered that.  I think I talked myself out of it because of concerns about what happens if no callback is passed in, but you're right: we could just do this in AppendCallbacks.

> I think it would be cool for this to be an ancestor of ScriptErrorEvent, and have the
> latter optionally delegate the console reporting to the superclass.

I think I could actually do this.  Let me see.  Still won't help the code-duplication from the JSErrorReport munging.  :(

> nsContentUtils::GetObjectPrincipal()

Perfect!

> Does this need to be XXXbz?

Yes, because ideally we would in fact call it.

I really wish we could get a JSContext here without potentially triggering GC.  :(

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 796461 [details] [diff] [review]
Report unhandled rejections in promises.


>+  nsIPrincipal *principal = nsJSPrincipals::get(
nsIPrincipal*

>+  nsCOMPtr<nsPIDOMWindow> win =
>+    do_QueryInterface(nsJSUtils::GetStaticScriptGlobal(&mResult.toObject()));
Hmm, do we want that window or mWindow. Probably the window from mResult is better.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff addressing Bobby's review comments

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated patch, just for reference

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Change js::GetErrorTypeName to take a JSRuntime, not a JSContext.

Comment 20

[Luke Wagner [:luke]]

Comment on attachment 796857 [details] [diff] [review]
part 1.  Change js::GetErrorTypeName to take a JSRuntime, not a JSContext.

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

::: js/src/jsatominlines.h
@@ +199,5 @@
> +    JS_STATIC_ASSERT(offsetof(JSAtomState, Null) +
> +                     JSProto_LIMIT * sizeof(FixedHeapPtr<PropertyName>) <=
> +                     sizeof(JSAtomState));
> +    JS_STATIC_ASSERT(JSProto_Null == 0);
> +    return (&rt->atomState.Null)[key];

Instead of duplicating the body, could you have
  ClassName(JSProtoKey, JSAtomState &atomState)
which would be called by the ExclusiveContext *cx overload, passing cx->names(), and GetErrorTypeName, passing rt->atomState?

Comment 21

[Boris Zbarsky [:bzbarsky]]

>  ClassName(JSProtoKey, JSAtomState &atomState)

Good idea.  Done.

Comment 22

[Bobby Holley (:bholley)]

Comment on attachment 796852 [details] [diff] [review]
Interdiff addressing Bobby's review comments

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

This was kind of a hard patch to review, but I think it all looks ok.

Comment 23

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/637e1697dd27
https://hg.mozilla.org/integration/mozilla-inbound/rev/0620f5288812

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/637e1697dd27
https://hg.mozilla.org/mozilla-central/rev/0620f5288812