Closed Bug 877896 Opened 7 years ago Closed 4 years ago

When a JavaScript Error occurs, have a way to show the stacktrace

Categories

(Firefox OS Graveyard :: Gaia, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
FxOS-S8 (02Oct)
Tracking Status
firefox44 --- fixed

People

(Reporter: gkw, Assigned: wcpan)

References

Details

(Keywords: b2g-testdriver, foxfood, unagi)

Attachments

(1 file, 5 obsolete files)

When a JavaScript Error occurs, currently only the error is toString'ed in adb logcat. e.g. from bug 877889:

E/GeckoConsole(  108): [JavaScript Error: "TypeError: nextCard is null" {file: "app://system.gaiamobile.org/js/cards_view.js" line: 515}]

There is no way to get a stacktrace for this from a B2G test driver Unagi phone.

Doug and I spoke about this and it would be nice to have a way to get the stacktrace to help in future JS errors, either as a developer option or otherwise. (Feel free to re-assign this as necessary)

===

I'm on 2013-05-24 v1.1.0 git revision cc2fd02fd461aa12c96e02229a78293365d65264
These stacks can possibly then be stored somewhere for a future (if possible) about:gaia-errors location, see bug 857843.
See Also: → 857843
If you could also make so that the 'debugger' keyword prints to logcat, that would be great.
This might be one more thing that would profit from bug 355430...
Exception handling is pretty insane right now, largely because of the way it interacts with the JSContext. In the coming months I'm going to be doing a lot of work to straighten this stuff out, after which point we'll be in a better position to fix issues like this one.
Depends on: 881811
Looking at this again, I don't think it needs to depend on bug 881811.
No longer depends on: 881811
nihsanullah, this is important to QA - can you find an owner?
Assignee: doug.turner → nihsanullah
I don't think Naveed is working on this.

Also, this is probably important for fuzzers and foxfooders too, so adding foxfood keyword.
Assignee: nihsanullah → nobody
Blocks: marifuzz
Status: ASSIGNED → NEW
Keywords: foxfood
Assignee: nobody → wpan
Status: NEW → ASSIGNED
Attached patch h.patch (obsolete) — Splinter Review
This is a prototype to print uncaught exceptions in logcat.
For now it can print uncaught exceptions in a webapp, with or without a Promise.

But for those exceptions that cause webapp crashes to system app, I still can't get the stack information.
When the exceptions popped from pending queue, they have message, file name, file number ... etc. but only stack property is null.

I'm still trying to find out how to retrieve, if you have any suggestion, please enlighten me, thanks!
(In reply to Wei-Cheng Pan [:wcp] [:legnaleurc] from comment #8)
> But for those exceptions that cause webapp crashes to system app, I still
> can't get the stack information.
> When the exceptions popped from pending queue, they have message, file name,
> file number ... etc. but only stack property is null.
> 
> I'm still trying to find out how to retrieve, if you have any suggestion,
> please enlighten me, thanks!

You'd probably should set a needinfo? from someone for a response. Setting this from Vivien Nicolas and/or Etienne Segonzac, System app owner/peer as a start, for other points of view.
Flags: needinfo?(etienne)
Flags: needinfo?(21)
Thanks for your help!
Looks like a question for Vivien :)
Flags: needinfo?(etienne)
Only prints the exceptions in webapps, but I think it's a start.
Attachment #8656505 - Attachment is obsolete: true
Attachment #8661050 - Flags: review?(nfitzgerald)
Comment on attachment 8661050 [details] [diff] [review]
print-stack-trace-in-the-console-service.patch

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

I'm not an xpconnect peer, but I did leave a couple comments below.

::: js/xpconnect/src/nsScriptErrorWithStack.cpp
@@ +19,5 @@
> +
> +namespace {
> +
> +static nsAutoCString
> +FormatStackString(JSContext * cx, HandleObject aStack) {

Style nit: JSContext* cx

@@ +118,5 @@
> +        tempMessage = ToNewUTF8String(mMessage);
> +    if (!mSourceName.IsEmpty())
> +        // Use at most 512 characters from mSourceName.
> +        tempSourceName = ToNewUTF8String(StringHead(mSourceName, 512));
> +    if (!mSourceLine.IsEmpty())

This is a bit of an existing issue and an aside, but I think that nsScriptErrorWithStack shouldn't have/use the mLine/mSourceName/etc members. All that information is recoverable from the SavedFrame stack, but depending on the principals of the caller's cx you may get different answers. Because the members aren't methods and can not consider principals, they will either leak information about privileged frames to unprivileged callers, or be limited to talking about unprivileged frames only even if the caller could use the information about more privileged frames.

I think nsScriptErrorWithStack should not be a subclass of nsScriptErrorBase, and should have getter methods which take a cx for accessing the error's line/source/etc that talk to the SavedFrame stack directly.

At minimum, I think you can ignore those members here and use JS::GetSavedFrameLine/etc instead.
Attachment #8661050 - Flags: review?(nfitzgerald)
Thanks for your comment. This patch has fixed the pointed problem.

Also I'm curious why some exception have a stack that is null?
Attachment #8661050 - Attachment is obsolete: true
Attachment #8661726 - Flags: review?(gkrizsanits)
(In reply to Wei-Cheng Pan [:wcp] [:legnaleurc] from comment #14)
> Also I'm curious why some exception have a stack that is null?

Usually that happens because there is no JS on the stack when the error occurs, eg C++ rejecting a fetch() promise after receiving a bad HTTP response.

If you are certain there is JS on the stack when the error is thrown and the stack member is still null, then I think we have a bug either with nsScriptErrorWithStack or the SavedFrame stacks machinery.
Oh, I see, thank you.
So maybe we don't have a change to capture the stacks for some exceptions.
Attachment #8661726 - Attachment is obsolete: true
Attachment #8661726 - Flags: review?(gkrizsanits)
Attachment #8663536 - Flags: review?(gkrizsanits)
Attachment #8663536 - Flags: review?(bobbyholley)
Comment on attachment 8663536 [details] [diff] [review]
print-stack-trace-in-the-console-service.patch

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

::: js/xpconnect/src/nsScriptErrorWithStack.cpp
@@ +104,5 @@
> +    static const char warning[] = "JavaScript Warning";
> +
> +    const char* severity = !(mFlags & JSREPORT_WARNING) ? error : warning;
> +
> +    ThreadsafeAutoJSContext cx;

Are you intending this to run off-main-thread? Given that the stack is a main-thread JSObject, that would be totally unsafe.

@@ +154,5 @@
> +        free(tempMessage);
> +    if (nullptr != tempSourceName)
> +        free(tempSourceName);
> +    if (nullptr != tempSourceLine)
> +        free(tempSourceLine);

This is too much dangerous string manipulation for me to be comfortable adding in C++, unfortunately. It looks like this is copied from nsScriptErrorWithStack. Please find a way to share that code such that this patch doesn't add much scary string code. Hosting a |protected| helper method to nsScriptErrorBase might be a good start.
Attachment #8663536 - Flags: review?(gkrizsanits)
Attachment #8663536 - Flags: review?(bobbyholley)
Attachment #8663536 - Flags: review-
(In reply to Bobby Holley (:bholley) from comment #17)
> Comment on attachment 8663536 [details] [diff] [review]
> print-stack-trace-in-the-console-service.patch
> 
> Review of attachment 8663536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/src/nsScriptErrorWithStack.cpp
> @@ +104,5 @@
> > +    static const char warning[] = "JavaScript Warning";
> > +
> > +    const char* severity = !(mFlags & JSREPORT_WARNING) ? error : warning;
> > +
> > +    ThreadsafeAutoJSContext cx;
> 
> Are you intending this to run off-main-thread? Given that the stack is a
> main-thread JSObject, that would be totally unsafe.

Is it safe to use one of the JSContexts in CycleCollectedJSRuntime::Get()->Runtime()?
Flags: needinfo?(bobbyholley)
(In reply to Wei-Cheng Pan [:wcp] [:legnaleurc] from comment #18)
> Is it safe to use one of the JSContexts in
> CycleCollectedJSRuntime::Get()->Runtime()?

I don't understand this question. CycleCollectedJSRuntime::Get() will return a different runtime depending on what thread you're on. The object inside nsScriptErrorWithStack is always from the main-thread runtime.

The ToString method should do:

MOZ_ASSERT(NS_IsMainThread());
AutoJSAPI jsapi;
if (!jsapi.Init(mStack))
    return NS_ERROR_FAILURE;
JSContext* cx = jsapi.cx();

Or something like that.
Flags: needinfo?(bobbyholley)
Fixed JSContext and refactor it to reuse nsScriptErrorBase::ToString.
Attachment #8663536 - Attachment is obsolete: true
Attachment #8664696 - Flags: review?(bobbyholley)
Comment on attachment 8664696 [details] [diff] [review]
print-stack-trace-in-the-console-service-v2.patch

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

r=me with those fixes. Thanks!

::: js/xpconnect/src/nsScriptErrorWithStack.cpp
@@ +96,5 @@
> +    MOZ_ASSERT(NS_IsMainThread());
> +    AutoJSAPI jsapi;
> +    if (!jsapi.Init(mStack)) {
> +        return NS_ERROR_FAILURE;
> +    }

Please move this part below the nsScriptErrorBase part, and make the AutoJSAPI + FormatStackString call conditional on mStack being non-null (it might be null, if it's been cycle-collected).

@@ +102,5 @@
> +    nsCString message;
> +    nsresult rv = nsScriptErrorBase::ToString(message);
> +    if (rv != NS_OK) {
> +        return rv;
> +    }

You should do NS_FAILED instead of a direct comparison. However in this case, I'd prefer:

NS_ENSURE_SUCCESS(rv, rv);
Attachment #8664696 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/e0e53c6083c5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → FxOS-S8 (02Oct)
Thanks for fixing this! \o/

Does this mean that the stack will now appear under the unhandled JS exception by default? Or do we need to turn this on via some other flag/switch?
Flags: needinfo?(21) → needinfo?(wpan)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #25)
> Thanks for fixing this! \o/
> 
> Does this mean that the stack will now appear under the unhandled JS
> exception by default? Or do we need to turn this on via some other
> flag/switch?

Yes it does not need any pref, and it will appears in logcat for B2G.

Just note that not all unhandled exceptions have valid JS stack at the moment,
so some of them (e.g. exceptions from jsm) still have no stack in the log.
Flags: needinfo?(wpan)
(In reply to Wei-Cheng Pan [:wcp] [:legnaleurc] from comment #26)
> Just note that not all unhandled exceptions have valid JS stack at the
> moment,
> so some of them (e.g. exceptions from jsm) still have no stack in the log.

Perhaps we'll need a new bug for this..?
Flags: needinfo?(wpan)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #27)
> (In reply to Wei-Cheng Pan [:wcp] [:legnaleurc] from comment #26)
> > Just note that not all unhandled exceptions have valid JS stack at the
> > moment,
> > so some of them (e.g. exceptions from jsm) still have no stack in the log.
> 
> Perhaps we'll need a new bug for this..?

Yes, but I don't know how to describe the bug.
The problem is what I said in comment 8.
I tried to print stacks JS_SetPendingException, but I always crash in MaybeAutoCompartment.
Flags: needinfo?(wpan)
Blocks: 1208641
I filed bug 1208641.
You need to log in before you can comment on or make changes to this bug.