Closed Bug 903419 Opened 7 years ago Closed 7 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 1 obsolete file)

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?
Blocks: 902866
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
> 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.
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.
> 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)?
Flags: needinfo?(bugs)
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.
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.
Flags: needinfo?(bugs)
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.
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.
Luke, could you look over the jsfriendapi bit?
Attachment #796461 - Flags: review?(luke)
Attachment #796461 - Flags: review?(bugs)
Attachment #796461 - Flags: review?(bobbyholley+bmo)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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 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 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.
Attachment #796461 - Flags: review?(luke) → review+
> 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 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?
Attachment #796461 - Flags: review?(bobbyholley+bmo) → review+
> 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 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.
Attachment #796461 - Flags: review?(bugs) → review+
Attachment #796461 - Attachment is obsolete: true
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?
Attachment #796857 - Flags: review?(luke) → review+
>  ClassName(JSProtoKey, JSAtomState &atomState)

Good idea.  Done.
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.
Attachment #796852 - Flags: review?(bobbyholley+bmo) → review+
Blocks: 966452
Depends on: 1257096
Depends on: 1257172
Depends on: 1257721
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.