Closed
Bug 903419
Opened 11 years ago
Closed 11 years ago
Report unhandled exceptions to the error console when a Promise is GCed
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: dev-doc-complete)
Attachments
(3 files, 1 obsolete file)
20.71 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
21.00 KB,
patch
|
Details | Diff | Splinter Review | |
3.90 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
> 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•11 years ago
|
||
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.
Assignee | ||
Comment 4•11 years ago
|
||
> 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)
Comment 5•11 years ago
|
||
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•11 years ago
|
||
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)
Comment 7•11 years ago
|
||
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.
Assignee | ||
Comment 8•11 years ago
|
||
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.
Assignee | ||
Comment 9•11 years ago
|
||
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 | ||
Updated•11 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Comment 10•11 years ago
|
||
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•11 years ago
|
||
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•11 years ago
|
||
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+
Assignee | ||
Comment 13•11 years ago
|
||
> 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•11 years ago
|
||
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+
Assignee | ||
Comment 15•11 years ago
|
||
> 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•11 years ago
|
||
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+
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #796852 -
Flags: review?(bobbyholley+bmo)
Assignee | ||
Comment 18•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Attachment #796461 -
Attachment is obsolete: true
Assignee | ||
Comment 19•11 years ago
|
||
Attachment #796857 -
Flags: review?(luke)
Comment 20•11 years ago
|
||
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+
Assignee | ||
Comment 21•11 years ago
|
||
> ClassName(JSProtoKey, JSAtomState &atomState)
Good idea. Done.
Comment 22•11 years ago
|
||
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+
Assignee | ||
Comment 23•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/637e1697dd27
https://hg.mozilla.org/integration/mozilla-inbound/rev/0620f5288812
Flags: in-testsuite?
Target Milestone: --- → mozilla26
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/637e1697dd27
https://hg.mozilla.org/mozilla-central/rev/0620f5288812
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Updated•11 years ago
|
Keywords: dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•