Closed
Bug 1208641
Opened 9 years ago
Closed 9 years ago
Some unhandled DOM / XPC exceptions have no stacks
Categories
(Firefox OS Graveyard :: Gaia, defect)
Tracking
(firefox47 fixed)
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: gkw, Assigned: wcpan)
References
(Blocks 1 open bug)
Details
(Keywords: foxfood)
Attachments
(1 file, 3 obsolete files)
(deleted),
patch
|
wcpan
:
review+
|
Details | Diff | Splinter Review |
+++ 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 2•9 years ago
|
||
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!
Flags: needinfo?(fabrice)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(wpan)
Reporter | ||
Updated•9 years ago
|
No longer blocks: orangfuzz
Keywords: b2g-testdriver,
unagi
Assignee | ||
Comment 3•9 years ago
|
||
(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}] )
Flags: needinfo?(wpan)
Assignee | ||
Comment 4•9 years ago
|
||
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.
Attachment #8700568 -
Flags: review?(mrbkap)
Comment 5•9 years ago
|
||
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.
Attachment #8700568 -
Flags: review?(mrbkap) → review?(bobbyholley)
Comment 6•9 years ago
|
||
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•9 years ago
|
||
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?
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 8•9 years ago
|
||
(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•9 years ago
|
||
(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•9 years ago
|
||
Comment on attachment 8700568 [details] [diff] [review] extract-stack-from-dom-xpc-exception-part-1.patch Canceling review in the mean time.
Attachment #8700568 -
Flags: review?(bobbyholley)
Comment 11•9 years ago
|
||
(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.)
Flags: needinfo?(nfroyd) → needinfo?(bgrinstead)
Comment 12•9 years ago
|
||
(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.
Flags: needinfo?(bgrinstead)
Comment 13•9 years ago
|
||
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.
Flags: needinfo?(wpan)
Assignee | ||
Comment 14•9 years ago
|
||
(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.
Flags: needinfo?(wpan)
Assignee | ||
Comment 15•9 years ago
|
||
Extract to a function.
Attachment #8700568 -
Attachment is obsolete: true
Attachment #8705484 -
Flags: review?(bobbyholley)
Reporter | ||
Updated•9 years ago
|
Assignee: nobody → wpan
Status: NEW → ASSIGNED
Comment 16•9 years ago
|
||
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) {
Attachment #8705484 -
Flags: review?(bobbyholley) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Attachment #8705484 -
Attachment is obsolete: true
Attachment #8709317 -
Attachment is obsolete: true
Attachment #8709319 -
Flags: review+
Comment 19•9 years ago
|
||
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...
Flags: needinfo?(wpan)
Assignee | ||
Comment 20•9 years ago
|
||
I thought it will be better to have a test before landing this patch. Should we just land the patch and close this?
Flags: needinfo?(wpan)
Comment 21•9 years ago
|
||
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....
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 22•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/68ae644667f2a398d10bac8941c4b7ae2193281a Bug 1208641 - Extract stack from DOM/XPC exception. r=bholley
Updated•9 years ago
|
Keywords: checkin-needed
Summary: Some unhandled JS exceptions (e.g. from jsm / system app) have no stacks → Some unhandled DOM / XPC exceptions have no stacks
Updated•6 years ago
|
Blocks: dbg-stacks
You need to log in
before you can comment on or make changes to this bug.
Description
•