1208641 - Some unhandled DOM / XPC exceptions have no stacks

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

+++ This bug was initially created as a clone of Bug #877896 +++

As per bug 877896 comment 8 and bug 877896 comment 28, some unhandled JS exceptions (e.g. from jsm / system app) still have no stacks.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Fabrice, any clue?

Comment 2

[[:fabrice] Fabrice Desré]

Could this be because we discard source for certified apps and chrome code? Try flipping http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#654 to check that!

Comment 3

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

(In reply to [:fabrice] Fabrice Desré from comment #2)
> Could this be because we discard source for certified apps and chrome code?
> Try flipping
> http://mxr.mozilla.org/mozilla-central/source/b2g/app/b2g.js#654 to check
> that!

Just test this pref by inserting some code to reproduce some resolved bugs e.g.: bug 1146053
but it still can't retrieve the stack. (just shows
W/GeckoConsole(  207): [JavaScript Error: "NS_ERROR_NOT_IMPLEMENTED: SetNFCFocus for in-process mode is not yet supported" {file: "jar:file:///system/b2g/omni.ja!/components/BrowserElementParent.js" line: 998}]
)

Comment 4

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: extract-stack-from-dom-xpc-exception-part-1.patch

This should display most kinds of exceptions.

Another unsolved problem is error reported by AsyncErrorReporter. (e.g. from Promise or Worker)
I think it would be better to store stringified stack into AsyncErrorReporter.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8700568 [details] [diff] [review]
extract-stack-from-dom-xpc-exception-part-1.patch

I think Bobby was the last person to touch this. I don't understand the current status of the error reporting stuff well enough to review this.

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 8700568 [details] [diff] [review]
extract-stack-from-dom-xpc-exception-part-1.patch

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

::: js/xpconnect/src/nsXPConnect.cpp
@@ +248,5 @@
>      nsCOMPtr<nsIConsoleService> consoleService =
>        do_GetService(NS_CONSOLESERVICE_CONTRACTID);
>  
>      nsCOMPtr<nsIScriptError> errorObject;
> +    if (aStack) {

Can you explain why it's save to remove the check for mWindowID? According to the comment below, this is necessary to avoid leaks because we only clean up after documents.

Comment 7

[Bobby Holley (:bholley)]

By the way wcp, I really, _really_ appreciate your persistence in tackling these tricky XPConnect issues (most people just give up!). Sorry I haven't been able to be more helpful, I've just been busy with other things.

I'm guessing that there's something we can do to make this work for non-window globals, but we need to think about the leak implications. Nathan, can you explain why exactly we need to buffer these messages in a way that makes the ClearMessagesForWindowId call necessary?

Comment 8

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

(In reply to Bobby Holley (busy) from comment #6)
> Comment on attachment 8700568 [details] [diff] [review]
> Can you explain why it's save to remove the check for mWindowID? According
> to the comment below, this is necessary to avoid leaks because we only clean
> up after documents.

You are right, I didn't notice that.
I did that because some reports do not come with a valid window ID.

Can we just store those formatted stack string into nsScriptError?
Or this will cause serious security problem?.

Comment 9

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (busy) from comment #7)
> I'm guessing that there's something we can do to make this work for
> non-window globals, but we need to think about the leak implications.
> Nathan, can you explain why exactly we need to buffer these messages in a
> way that makes the ClearMessagesForWindowId call necessary?

Just a note: I'm not going to be able to do enough of a dive on this to figure out what's going on until after the new year.  Keeping the ni? so I remember to do that. :)

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8700568 [details] [diff] [review]
extract-stack-from-dom-xpc-exception-part-1.patch

Canceling review in the mean time.

Comment 11

[Nathan Froyd [:froydnj]]

(In reply to Bobby Holley (busy) from comment #7)
> I'm guessing that there's something we can do to make this work for
> non-window globals, but we need to think about the leak implications.
> Nathan, can you explain why exactly we need to buffer these messages in a
> way that makes the ClearMessagesForWindowId call necessary?

So we have a pref to control whether these messages get buffered.  This pref is essentially always true...except for B2G (see bug 848615 and bug 852018).

As to why the messages get buffered in the first place...I'm not sure.  The buffering dates back to the original import of the code into xpcom in 2000 (no bug associated with the commit, natch), and I can't locate the code prior to that.  So I don't know why we buffer things in the first place. :(

We could turn off the buffering.  We have a couple tests that apparently rely on the buffering, but they could flip the pref appropriately.  The only bit that appears to use the buffering is devtools's webconsole, but even that looks like it sets up console listeners, so it shouldn't really care about the buffered messages...Brian, do you know why we have console listeners and check the message buffering hereabouts:

http://mxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/utils.js#821

?  (I'm sure we also have addons that might complain if we stopped buffering, but one thing at a time.)

Comment 12

[Brian Grinstead [:bgrins]]

(In reply to Nathan Froyd [:froydnj] from comment #11)
> We could turn off the buffering.  We have a couple tests that apparently
> rely on the buffering, but they could flip the pref appropriately.  The only
> bit that appears to use the buffering is devtools's webconsole, but even
> that looks like it sets up console listeners, so it shouldn't really care
> about the buffered messages...Brian, do you know why we have console
> listeners and check the message buffering hereabouts:
> 
> http://mxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/
> utils.js#821
> 
> ?  (I'm sure we also have addons that might complain if we stopped
> buffering, but one thing at a time.)

The nsIConsoleService is storing error messages (not console API messages).  So it's how we display a page's JS errors and other messages sent from the platform in the web console - CSP violations, as one example.

But the webconsole actor doesn't get instantiated until the devtools are opened.  And when the actor initializes we need to display messages that happened before the tools were opened, which is the purpose of the call to Services.console.getMessageArray.  Then the next step in that function is that if it's not the Browser Console we need to do filtering based on the message's innerWindowID - making sure that it matches the window id of the current page or any subframes so that we don't show messages from Tab A in Tab B's webconsole.  This filtering also happens for messages coming through registerListener, which also gets called when the webconsole actor is initialized.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Could we please separate this out into two separate bugs?  One bug for handling DOMException/Exception at both the places in nsJSEnvironment where we currently do ExceptionStackOrNull (the system error reporter, as patched in this bug and ScriptErrorEvent::Run, which is what most window stuff goes through).  This will have immediate wins for web developers in terms of showing in the console stacks for most DOM stuff; now I understand why we didn't show them before!

Then we can have a second bug for figuring out the non-window-global situation.

I don't have a strong feeling for which one of those two this bug should be; given its summary it should probably focus on the non-window globals, I guess.  Wei-Cheng, any preference?

I definitely don't think we should block landing the web-developer-facing win on the non-window-global bits.

Comment 14

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #13)
> Could we please separate this out into two separate bugs?  One bug for
> handling DOMException/Exception at both the places in nsJSEnvironment where
> we currently do ExceptionStackOrNull (the system error reporter, as patched
> in this bug and ScriptErrorEvent::Run, which is what most window stuff goes
> through).  This will have immediate wins for web developers in terms of
> showing in the console stacks for most DOM stuff; now I understand why we
> didn't show them before!
> 
> Then we can have a second bug for figuring out the non-window-global
> situation.
> 
> I don't have a strong feeling for which one of those two this bug should be;
> given its summary it should probably focus on the non-window globals, I
> guess.  Wei-Cheng, any preference?
> 
> I definitely don't think we should block landing the web-developer-facing
> win on the non-window-global bits.

Sure, I'll update the patch so you can deal the buffering problem in another bug.

Comment 15

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: extract-stack-from-dom-xpc-exception.patch

Extract to a function.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8705484 [details] [diff] [review]
extract-stack-from-dom-xpc-exception.patch

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

Please find a way to add tests for this (a followup bug is ok) - this kind of thing is very easy to regress, unfortunately.

Thanks for working on this stuff!

r=bholley with comments addressed.

::: dom/base/nsJSEnvironment.cpp
@@ +216,5 @@
>  // us from triggering expensive full collections too frequently.
>  static int32_t sExpensiveCollectorPokes = 0;
>  static const int32_t kPokesBetweenExpensiveCollectorTriggers = 5;
>  
> +// The exception object may be a JS/DOM/XPC exception

Please change this comment to:

// This handles JS Exceptions (via ExceptionStackOrNull), as well as DOM and XPC Exceptions.
//
// Note that the returned object is _not_ wrapped into the compartment of cx.

@@ +218,5 @@
>  static const int32_t kPokesBetweenExpensiveCollectorTriggers = 5;
>  
> +// The exception object may be a JS/DOM/XPC exception
> +static JSObject*
> +FindExceptionStackOrNull(JSContext* cx, JS::HandleObject exceptionObject)

I would just call this "FindExceptionStack".

@@ +222,5 @@
> +FindExceptionStackOrNull(JSContext* cx, JS::HandleObject exceptionObject)
> +{
> +  JSAutoCompartment ac(cx, exceptionObject);
> +  JS::RootedObject stackObject(cx, ExceptionStackOrNull(cx, exceptionObject));
> +  if (!stackObject) {

Instead of doing this, please early-return in the case that stackObject is found. That will decrease the indentation level of the rest of this function.

@@ +226,5 @@
> +  if (!stackObject) {
> +    // It is not a JS Exception, try DOM Exception.
> +    RefPtr<Exception> exception;
> +    nsresult rv = UNWRAP_OBJECT(DOMException, exceptionObject, exception);
> +    if (NS_FAILED(rv)) {

It seems inconsistent to check |rv| for the first UNWRAP and check |exception| for the second. I'd switch this to:

if (!exception) {

Comment 17

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: extract-stack-from-dom-xpc-exception.patch

Comment 18

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: extract-stack-from-dom-xpc-exception.patch

Comment 19

[Boris Zbarsky [:bzbarsky]]

What's blocking this from landing?  This has a reviewed patch as of a month ago (though as I said above I think that patch should have gone into a separate bug, and I still think it should do that), and that patch fixes a problem I'm hitting pretty much every day...

Comment 20

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

I thought it will be better to have a test before landing this patch.

Should we just land the patch and close this?

Comment 21

[Boris Zbarsky [:bzbarsky]]

I think you should land the patch and not block on the test, yes.

As far as closing this, that's fine if you retitle it to clearly refer to DOMException/Exception from Window and file a followup on the actual problem with jsm/system-app....

Comment 22

[Alexandre Poirot [:ochameau]]

https://hg.mozilla.org/integration/fx-team/rev/68ae644667f2a398d10bac8941c4b7ae2193281a
Bug 1208641 - Extract stack from DOM/XPC exception. r=bholley

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/68ae644667f2