404749 - errors in js modules are not reported

Status: RESOLVED DUPLICATE of bug 415498

(Core :: XPConnect, defect, P3)

Description

[Sergey Yanovich]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.8) Gecko/20071004 Iceweasel/2.0.0.8 (Debian-2.0.0.8-1)
Build Identifier: 1.9b1

This has regressed after 1.9a8, the most probable candidate is a fix for bug 353737.

Reproducible: Always

Steps to Reproduce:
1.set pref("javascript.options.showInConsole, true)
2.add a line 'throw "error";' to the end of any function known to be called in any js module
3.launch with --jsconsole

Actual Results:  
nothing happens

Expected Results:  
uncaught exception "error" is logged to js console

Comment 1

[Sergey Yanovich]

Linking probable regression source.

Comment 2

[Sergey Yanovich]

(In reply to comment #1)
> Linking probable regression source.
> 

A more probable regression source is bug 393627. Looks like new behavior is intentional.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

bug 353737 has no effect on Firefox. The only behavior change from bug 393627 *should* be on |throw Components.results.NS_...|. If that's not the case, then this is a bug.

Comment 4

[Sergey Yanovich]

I see a I said: 'throw "error";' is ignored. I really need this lacking functionality. I will try and reverse the patch for bug 393627 to see if it helps.

What if I create a patch with a pref to block new behavior?

I plan to read the pref once in XPCJSRunTime constructor, store it there, and later poll from XPCWrappedJSClass::CheckForException(...) using mJSRunTime member pointer.

Comment 5

[Sergey Yanovich]

I can see all js XPCOM errors/exceptions with patch for bug 393627 reverted.

Comment 6

[Sergey Yanovich]

Should we mark this as a duplicate of bug 393627 and reopen the latter?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

I don't see how this is a duplicate of that bug. That bug appears to be the opposite of this one, except that mrbkap said that its patch should only have affected throwing error codes, while you're reporting that it also affected throwing "error". That seems like the behavior change we should be focusing on.

Comment 8

[Sergey Yanovich]

The patch for bug 393627 has prevented reporting of most js errors by allowing only 5 nsresult values to pass a filter. The patch aimed at hiding a flow of exception arising from calls of 'nsIHandlerService::getTypeFromExtension'.

Fixing this bug would most likely restore that flow of js console errors, which means bug 393627 would need a new fix. So we can start to think about a new fix for it from the beginning.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #8)
> The patch for bug 393627 has prevented reporting of most js errors by allowing
> only 5 nsresult values to pass a filter. The patch aimed at hiding a flow of
> exception arising from calls of 'nsIHandlerService::getTypeFromExtension'.

No, it didn't. The errors in xpc_IsReportableErrorCode are the errors thrown when the JS component threw something that isn't directly translatable into an nsresult, such as a string or an object. In such cases, to avoid losing information, we still do want to want to report the error. All other errors can be turned into nsresults with no loss of information.

> Fixing this bug would most likely restore that flow of js console errors, which
> means bug 393627 would need a new fix. So we can start to think about a new fix
> for it from the beginning.

So this doesn't hold. Fixing this bug will entail figuring out why throwing a regular string (which I thought should translate to NS_ERROR_XPC_JS_THREW_STRING) is getting suppressed. The vast majority of other numeric things thrown don't need to be reported.

Comment 10

[Sergey Yanovich]

I see the point.

In this case, maybe we can use a slightly different approach. Instead of allowing a handful of values to pass the filter, we can establish a special NS_ERROR_xxx (like NS_ERROR_FILTERED_FAILURE) to be thrown uncaught.

For example, I still need to see when a native component called from js returns an error. If all numeric exceptions are suppressed, it won't be possible.

Comment 11

[Sergey Yanovich]

Attachment: patch blocks only 3 nsresult values

This patch uses approach from comment #10

Comment 12

[Sergey Yanovich]

Attachment: patch blocks only 3 nsresult values v2

Fixed typo:

s/ENSURE_TRUE(rv, .../ENSURE_SUCCESS(rv, .../

Comment 13

[Sergey Yanovich]

Attachment: patch blocks only 3 nsresult values v3

After playing with the previous patch, I discovered, that there is no need to change c++ code of nsExternalHelperAppService.

The new exeption must also go to xpc.msg, though its message will never be displayed :)

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 289943 [details] [diff] [review]
patch blocks only 3 nsresult values v3

This doesn't look like what we want to be doing, since it replaces a variety of error codes with a single one that all JS component methods faced with this situation must throw, which reduces expressiveness significant, and makes JS components responsible for behaving differently if they think they might be called by C++ code, which violates the XPCOM principle that components don't have to know what language their callers are written in (and vice-versa).

The right solution is to do what Blake said in comment 9: figure out why string exceptions are being suppressed, which is a bug (only error codes that can be turned into nsresults are supposed to be suppressed).

Comment 15

[Sergey Yanovich]

(In reply to comment #14)
> (From update of attachment 289943 [details] [diff] [review])
> This doesn't look like what we want to be doing, since it replaces a variety of
> error codes with a single one that all JS component methods faced with this
> situation must throw, which reduces expressiveness significant, and makes JS
> components responsible for behaving differently if they think they might be
> called by C++ code, which violates the XPCOM principle that components don't
> have to know what language their callers are written in (and vice-versa).

I thought, that it doesn't really matter who is the caller. The idea was to explicitly hide a *specific* exception from js console.

Furthermore, throwing an exception in a normal (expected) situation is not a sound practice. First of all, because real C++ exceptions are really expensive. Most often, there is a way for the caller to prevent an exception by verifying resource availability, data integrity and so on.

Currently we hide *all* exceptions and *all* errors, which are thrown/occur in js modules. That is at least counter-intuitive. Please consider that no failure in a C++ component which is called from a js one is, and will be logged. That makes it extremely difficult to debug *all* js components, despite having such a sophisticated tool as jsconsole at hands.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #15)
> I thought, that it doesn't really matter who is the caller. The idea was to
> explicitly hide a *specific* exception from js console.

The idea is to hide exceptions that are error codes when the caller is C++ code.


> Furthermore, throwing an exception in a normal (expected) situation is not a
> sound practice.

There seems to be some disagreement about this for the case of a JavaScript XPCOM component that wants to return an error code.


> First of all, because real C++ exceptions are really expensive.

We aren't using real C++ exceptions, so that's not an issue.


> Most often, there is a way for the caller to prevent an exception by verifying
> resource availability, data integrity and so on.

Sure, but occasionally there isn't, as in the nsHandlerService case that makes the bug more noticeable.


> Currently we hide *all* exceptions and *all* errors, which are thrown/occur in
> js modules. That is at least counter-intuitive.

That sounds bad.  The exception should still get thrown to the error console if the caller is JavaScript and doesn't catch the exception itself.


> Please consider that no failure
> in a C++ component which is called from a js one is, and will be logged. That
> makes it extremely difficult to debug *all* js components, despite having such
> a sophisticated tool as jsconsole at hands.

Right.  That sounds like a bug we should fix.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

I think this was really just bug 401735.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 289943 [details] [diff] [review]
patch blocks only 3 nsresult values v3

I don't think this should be necessary.

Comment 19

[Sergey Yanovich]

(In reply to comment #16)
> The idea is to hide exceptions that are error codes when the caller is C++
> code.

I have filed a patch to bug 415498 that does that, while still exposing unhandled XPCOM errors from C++. Switching duplicate.

> There seems to be some disagreement about this for the case of a JavaScript
> XPCOM component that wants to return an error code.
> 
> We aren't using real C++ exceptions, so that's not an issue.

I've spent a day in js/src jungles, and my impression from js_Invoke and js_Interpret on this issue is that JS exceptions are at least by order of magnitude more expensive than C++ ones. 'throw Components.results.NS_ERROR_FAILURE' is not even close in efficiency to 'return NS_ERROR_FAILURE'. So it may probably be a good performance-related idea to rewrite all JS components without throwing in normal path of execution.
 
> > Currently we hide *all* exceptions and *all* errors, which are thrown/occur in
> > js modules. That is at least counter-intuitive.
> 
> That sounds bad.  The exception should still get thrown to the error console if
> the caller is JavaScript and doesn't catch the exception itself.

This can also be achieved. We may check that exception is thrown from the top frame to hide it, and log it otherwise. But in the view of JS exception costs, it isn't worth the effort.

> > Please consider that no failure
> > in a C++ component which is called from a js one is, and will be logged. That
> > makes it extremely difficult to debug *all* js components, despite having such
> > a sophisticated tool as jsconsole at hands.
> 
> Right.  That sounds like a bug we should fix.

This is fixed in an alternative patch to bug 415498. Everyone's invited to comment there also.