David Baron :dbaron: Reporter
	Description • 12 years ago

This is probably more than one issue in a single bug, but I'm not quite sure how to split things up right now, or exactly what should be happening.

So while debugging bug 842476, which turned out to be a leak due to leaking an nsXPCException that got stuck on the XPCJSRuntime, I realized that it seems like a pretty bad problem that the exception in question *wasn't being reported at all*.  See bug 842476 for the exceptions passing through XPCWrappedJS.

The underlying problem is simple:  NS_ScriptErrorReporter has code to suppress reporting of exceptions if there's JS higher on the stack (calling JS_DescribeScriptedCaller).  This code is different from the suppression code in nsXPCWrappedJSClass::CheckForException (which I think is more reasonable, modulo that it defers to NS_ScriptErrorReporter when it calls JS_ReportPendingException, though I think it still might toss away things we want reported).

On top of that, js_ErrorFromException doesn't deal usefully with exceptions that are proxies, which happened in this case, maybe because of the interactions with the browser-element code?

Maybe shades of bug 482137, but different.

	David Baron :dbaron: Reporter
	Comment 1 • 12 years ago

This testcase shows a very simple example of why NS_ScriptErrorReporter is over-suppressing.  (I didn't verify what's happening in the debugger in this testcase, though; it just was created to be similar to what I saw in the debugger debugging the actual problem in bug 842476.)

If you load this testcase with the Web Console open, it executes a function that calls a nonexistent function.  The error is not reported.

Clicking on the paragraph invokes the same javascript differently, and thus does report the exception.

	David Baron :dbaron: Reporter
	Comment 2 • 12 years ago

So this patch touches three files:

 A. the mochitest change backs out the fix for the exception

 B. the jsexn.cpp change unwraps proxies (probably the wrong way, and I have no idea if there are major security risks to what I did there)

 C. the NS_ScriptErrorReporter change disables the check for nesting inside JS


With changes A and C but not B, I see (instead of bug 842176 comment 12):

JavaScript error: chrome://global/content/BrowserElementChildPreload.js, line 278: TypeError: can't access dead object
JavaScript error: , line 0: Error:
[Child 29898] WARNING: NS_ENSURE_TRUE(mCallback) failed: file /home/dbaron/builds/ssd/mozilla-central/mozilla/content/base/src/nsFrameMessageManager.cpp, line 417
JavaScript error: chrome://global/content/BrowserElementChildPreload.js, line 278: TypeError: can't access dead object
JavaScript error: , line 0: Error:
NOTE: child process received `Goodbye', closing down


With all the changes, I see:


JavaScript error: chrome://global/content/BrowserElementChildPreload.js, line 278: can't access dead object
JavaScript error: http://mochi.test:8888/tests/dom/browser-element/mochitest/file_browserElement_SetVisibleFrames_Outer.html, line 25: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "can't access dead object" {file: "chrome://global/content/BrowserElementChildPreload.js" line: 278}]' when calling method: [nsIPrompt::alert]
[Child 30559] WARNING: NS_ENSURE_TRUE(mCallback) failed: file /home/dbaron/builds/ssd/mozilla-central/mozilla/content/base/src/nsFrameMessageManager.cpp, line 417
JavaScript error: chrome://global/content/BrowserElementChildPreload.js, line 278: can't access dead object
JavaScript error: http://mochi.test:8888/tests/dom/browser-element/mochitest/file_browserElement_SetVisibleFrames_Inner.html?child1, line 13: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "can't access dead object" {file: "chrome://global/content/BrowserElementChildPreload.js" line: 278}]' when calling method: [nsIPrompt::alert]
NOTE: child process received `Goodbye', closing down

	David Baron :dbaron: Reporter
	Comment 3 • 12 years ago

Oh, and with changes A and B but not C, I see:

[Child 30723] WARNING: NS_ENSURE_TRUE(mCallback) failed: file /home/dbaron/builds/ssd/mozilla-central/mozilla/content/base/src/nsFrameMessageManager.cpp, line 417
JavaScript error: http://mochi.test:8888/tests/dom/browser-element/mochitest/file_browserElement_SetVisibleFrames_Inner.html?child1, line 13: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "can't access dead object" {file: "chrome://global/content/BrowserElementChildPreload.js" line: 278}]' when calling method: [nsIPrompt::alert]
NOTE: child process received `Goodbye', closing down

(the first exception disappears completely; the second is reported)


With only change A, I see:

[Child 31142] WARNING: NS_ENSURE_TRUE(mCallback) failed: file /home/dbaron/builds/ssd/mozilla-central/mozilla/content/base/src/nsFrameMessageManager.cpp, line 417
JavaScript error: , line 0: Error:
NOTE: child process received `Goodbye', closing down

(the first exception disappears completely; the second is reported uselessly -- this wasn't enough to clue me in to the problem being exceptions when I started debugging bug 842476)

	David Baron :dbaron: Reporter
	Comment 4 • 12 years ago

I'm interested in thoughts from Boris or others about what *ought* to be happening here.

	Bobby Holley (:bholley)
	Comment 5 • 12 years ago

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment 
>  B. the jsexn.cpp change unwraps proxies (probably the wrong way, and I have
> no idea if there are major security risks to what I did there)

Yeah, you should probably use UnwrapObjectChecked here.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 6 • 12 years ago

What "should" happen here is that we should read the mind of the person looking at the error console and... No, seriously, this has been changed back and forth countless times over the past few years. Slightly less impossible (but only barely) would be a solution that detected whether or not the already-running JS code is "related" to the code being called and if not, reporting before returning to it. Of course, knowing "related" is basically an unsolvable problem and there are cases (events, mostly) where both related and unrelated calls happen through the same bottleneck.

I'd expect the patch for this bug to push a null context (or call JS_SaveFrameChain) somewhere since that has been the traditional hack used to cause more errors to be reported. I'm curious what the C++ and JS stacks leading to NS_ScriptErrorReporter returning early look like.

	David Baron :dbaron: Reporter
	Comment 7 • 12 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #6)
> What "should" happen here is that we should read the mind of the person
> looking at the error console and... No, seriously, this has been changed
> back and forth countless times over the past few years. Slightly less
> impossible (but only barely) would be a solution that detected whether or
> not the already-running JS code is "related" to the code being called and if
> not, reporting before returning to it. Of course, knowing "related" is
> basically an unsolvable problem and there are cases (events, mostly) where
> both related and unrelated calls happen through the same bottleneck.
> 
> I'd expect the patch for this bug to push a null context (or call
> JS_SaveFrameChain) somewhere since that has been the traditional hack used
> to cause more errors to be reported. I'm curious what the C++ and JS stacks
> leading to NS_ScriptErrorReporter returning early look like.

FWIW, the current approach here seems like the sort of approach that's easy to get 90% right and hard to get 100% right.  I suspect that forcing anybody who wants to propagate a JS exception across C++ to pass around something that explicitly holds that JS exception, and reporting everything else, might make it easier to get the last 10% done, despite being harder for the first 90%.

	David Baron :dbaron: Reporter
	Comment 12 • 12 years ago

And, to be clear, I posted the patch because it explains why the exceptions weren't being reported (or reported usefully), not because I thought it was something we should land.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 12 years ago

> very simple testcase showing why NS_ScriptErrorReporter over-suppresses

For what it's worth, that testcase will be fixed by bug 835643.

The way we're doing this in WebIDL land is the following:

1)  By default, calls from C++ into JS report the exception when returning back into
    C++.
2)  The caller can suppress this behavior and instead ask that the exception be
    propagated back to it.  In this case it becomes the caller's responsibility to
    report the exception.  There are asserts that ensure that it does so or at least
    tries to; these asserts will trigger even if there is no exception, so that they're
    actually useful.

The mechanism used is basically that of comment 7: the ErrorResult can hold a jsval for the exception.

as for which of #1 and #2 to do, the specs are expected to define it.  For example, for event dispatch it's #1 and for TreeWalker callbacks it's #2.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 12 years ago

Oh, and... it's not clear to me that we can get to that state for XPCWrappedJS without significant surgery on how JS-wrapped stuff is exposed in C++, right?

	David Baron :dbaron: Reporter
	Comment 15 • 12 years ago

That sounds good.  Given that fix, this bug still needs something done about the proxy issue, though.

For non-Web-exposed nsXPCWrappedJS, would reporting anything that doesn't look like an nsresult failure code work?  I don't see why you'd want to suppress reporting of errors that weren't explicitly nsresults at an XPCOM interface boundary.

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 12 years ago

> Given that fix, this bug still needs something done about the proxy issue, though.

Well, and for XPCWrappedJS consumers.  The fix for event listeners involves no longer using XPCWrappedJS.

> would reporting anything that doesn't look like an nsresult failure code work?

Hmm.  That might not be a bad idea, yeah.

	BMO Automation
	Comment 17 • 6 years ago

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 6 years ago

We should still try to get this fixed, assuming the issue is still present.