393627 - JS XPCOM component exception handled by native code shows up in error console

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

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

My patch for bug 393492 adds a method to the nsHandlerService JS XPCOM component that throws an exception under certain circumstances.  The native callers of that method see the exception as a return value, understand it, and proceed normally when they encounter it.

Nevertheless, the exception shows up in the error console, and its location is reported to be the line in the front-end JS code that prompted the native call.

For example, at one point the new prefpane being added in bug 377784 appends a listitem element to a richlistbox.  The listitem element is bound to an XBL binding that adds an anonymous image child and sets its src attribute to a URL with the moz-icon: scheme, prompting the app to try to load that URL.

That load calls nsExternalHelperAppService at some point in the load process, and nsExternalHelperAppService then calls nsHandlerService::retrieve to see if the user has specified a handler for the scheme.  nsHandlerService::retrieve throws NS_ERROR_NOT_AVAILABLE, which shows up in the Error Console like this:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'Component is not available' when calling method: [nsIHandlerService::retrieve]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://browser/content/preferences/handlers.js :: anonymous :: line 90"  data: no]
************************************************************

handlers.js line 90 is just the line that appends the listitem element to the richlistbox:

      this._list.appendChild(item);

Since the exception is expected and handled by native code, it seems like it shouldn't show up on the error console.

Comment 1

[Boris Zbarsky [:bzbarsky]]

The exception is reported by the JS engine before control even returns to native code, so there's not much the native code or error console can do here.

If you're trying to somehow use an API that uses status codes to propagate information (which is a terrible design, fwiw), you need to use Components.returnCode.

Comment 2

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

Reopening and moving to XPConnect per discussion on IRC with bz and mrbkap.  bz summarized the problem succinctly: "this does make it impossible for a C++ caller to handle the exception".

Comment 3

[Brendan Eich [:brendan]]

I'm focused on XPConnect elimination for Mozilla 2, sooner the better. So I'm not the guy to help much here. But mrbkap is a softie ;-).

/be

Comment 4

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

Erm, this really should be in XPConnect, from what I understand.

Comment 5

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

duplicate of bug 374499?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Yes, except this bug has more info...

Comment 8

[Nickolay_Ponomarev]

> If you're trying to somehow use an API that uses status codes to propagate
> information (which is a terrible design, fwiw), you need to use
> Components.returnCode.

...which doesn't work (bug 287107)

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

I'll get on this as soon as I can.

Comment 10

[Karsten Düsterloh]

I get at least about half a hundred of these errors on trunk SM startup, and many  more when running Mailnews. This is annoying as hell, since the real errors almost drown in this mess... :-(

Comment 11

[Philip Chee]

As a workaround use the Console² extension and the "Hide duplicates" option.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

mrbkap: do you think you'd have cycles to look at this?

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

This is kind of ugly, but the idea is to not report exceptions that are simply thrown numbers. We do still want to report thrown JS object exceptions, as those can carry additional information that would be lost otherwise. The exception codes hard coded into the reportable check came from searching xpcconvert for what types of exceptions it throws for various situations.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, ok, you're all big whiners

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Johnny, ping?

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 282924 [details] [diff] [review]
Proposed fix

pong. Sorry for the delay here. r+sr=jst

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 282924 [details] [diff] [review]
Proposed fix

This should ease error console pain by reducing the number of exceptions reported to it.

Comment 19

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 282924 [details] [diff] [review]
Proposed fix

a=drivers for M9 landing

Comment 20

[Reed Loden [:reed]]

Checked this in for mrbkap since he wasn't sure when he would have time to do it.

Checking in js/src/xpconnect/src/xpcconvert.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcconvert.cpp,v  <--  xpcconvert.cpp
new revision: 1.116; previous revision: 1.115
done
Checking in js/src/xpconnect/src/xpcwrappedjsclass.cpp;
/cvsroot/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp,v  <--  xpcwrappedjsclass.cpp
new revision: 1.108; previous revision: 1.107
done

Comment 21

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007102503 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

V.Fixed,
per bug 374499 comment 29 (actually bug 384693 comment 16),
and bug 397088 comment 9.

Comment 22

[Sergey Yanovich]

Bug 404749 is a follow-up to this one.

I definitely agree with Myk Melez, that such a flood of errors is not normal. But I don't think that a correct approach to address that is used.

Errors go to jsconsole *only* if 'throw Components.error.NS_*;' goes to another javascript component! See QueryInterface of any js XPCOM. If you ask for a wrong interface from c++, no error is logged.

I am fighting this

Error: [Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no]

by providing a sensible default mime type. I will upload a patch in a moment.

Comment 23

[Sergey Yanovich]

Attachment: fix source of error messages

this patch provides a sensible default mime type instead of throwing an error.

Comment 24

[Boris Zbarsky [:bzbarsky]]

The C++ part of that patch is not acceptable.

Comment 25

[Sergey Yanovich]

(In reply to comment #24)
> The C++ part of that patch is not acceptable.
> 

If we cannot determine file type using all available resources, let's think it is a binary executable. Since that is potentially the most dangerous file type, I don't see any security implications.

What am I missing?

Comment 26

[Boris Zbarsky [:bzbarsky]]

The fact that the extension is not "all available resources".  There are callers that fall back on other methods of determining the type if there is no type mapping for this extension.  In particular, they may have a lot more information than just the extension.

For example, your change would make nsUnknownDecoder::SniffURI return true any time the URI has an extension.  That would have the wonderful effect that a file that start with "<?xml" but doesn't have an extension we recognize as XML would end up as application/octet-stream instead of the text/xml it ends up as now.

Similarly, a file named README loaded from local disk and containing only text would end up as application/octet-stream, not text/plain.

And that's just nsUnknownDecoder.  I haven't even tried looking at _all_ consumers of the API in question.

So we need a way to indicate "no mapping for this extension" in this API.  Whether we do that via an exception or an explicit return code is a different matter; I don't particularly like throwing exceptions to communicate information.

Comment 27

[Sergey Yanovich]

I see now.

application/x-unknown-content-type looks like a good choice (defined in 'netwerk/mime/public/nsMimeTypes.h'):
#define UNKNOWN_CONTENT_TYPE                "application/x-unknown-content-type"

BTW, I grepped for API consumers. nsJARChannel and nsBinHexDecoder are already assigning UNKNOWN_CONTENT_TYPE by default. nsIconChannel is assigning "application/octet-stream".

I noticed, that neither client is checking return value, but all check the returned string for being empty. Maybe that can be declared as an invariant for this piece of API?

Comment 28

[Sergey Yanovich]

(In reply to comment #27)
> I noticed, that neither client is checking return value, but all check the
> returned string for being empty. Maybe that can be declared as an invariant for
> this piece of API?

I mean to return NS_OK, and truncated _retval if there is no match.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> application/x-unknown-content-type looks like a good choice

Not really, unless you fix all consumers to deal.

Channels will assign application/x-unknown-content-type if they want their data to be run through nsUnknownDecoder.

> I mean to return NS_OK, and truncated _retval if there is no match.

That might work.  Again, you'd need to check all consumers (including the ones that call it indirectly, etc) to make sure that's ok.  But I see no problem with that change.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

I dispute that there's anything wrong with the current API.  This is an exceptional condition (the caller asked for something which doesn't exist), it _should_ throw an exception.  Using some sort of in-band indicator as the various proposals here discuss would be an icky hack compared to throwing an exception.

Comment 31

[Sergey Yanovich]

(In reply to comment #30)
> I dispute that there's anything wrong with the current API.  This is an
> exceptional condition (the caller asked for something which doesn't exist), it
> _should_ throw an exception.  Using some sort of in-band indicator as the
> various proposals here discuss would be an icky hack compared to throwing an
> exception.

I would fully agree with this, if there were a single possible source of information, but there is not.

'no type' is a legitimate result for the client, not an exceptional. As an analogy, 'SELECT 1 WHERE 1=2;' SQL statement returns an empty result set, but doesn't trigger an error.

Furthermore, thrown exceptions are uncaught in at least on place atm. That means, at that place(s) exception is not expected for incorrect data.

I still agree, that exception is already implemented, so another solution is to fix the client code, that is causing the flood of exceptions as is. I will experiment a bit with this. The suspect is 'getFromTypeAndExtension' call in 'uriloader/exthandler/nsHandlerService.js:1022'.

Comment 32

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

(In reply to comment #22)
> Errors go to jsconsole *only* if 'throw Components.error.NS_*;' goes to 
> another javascript component! See QueryInterface of any js XPCOM. If you ask
> for a wrong interface from c++, no error is logged.

Right, that's intentional, because C++ code "catches" those exceptions (where "catches" means "checks the return code and branches appropriately" in the C++ context), and exceptions should only appear on the error console if they aren't caught by any caller in the caller chain.


> I am fighting this
> 
> Error: [Exception... "'Component is not available' when calling method:
> [nsIHandlerService::getTypeFromExtension]"  nsresult: "0x80040111
> (NS_ERROR_NOT_AVAILABLE)"  location: "<unknown>"  data: no]
> 
> by providing a sensible default mime type.

The exception thrown by nsIHandlerService::getTypeFromExtension is just one (prominent) example of the pattern exposing this bug, so making that method always return a type does not address the underlying issue, which is that exceptions that are getting caught are showing up on the error console as if they hadn't been caught because they happen to have been caught by C++ code.


(In reply to comment #26)
> So we need a way to indicate "no mapping for this extension" in this API. 
> Whether we do that via an exception or an explicit return code is a different
> matter; I don't particularly like throwing exceptions to communicate
> information.

I don't care either way, but throwing exceptions is currently the recommended approach, so we should change the recommendation if we change this behavior.

Comment 33

[Sergey Yanovich]

(In reply to comment #32)
> (In reply to comment #22)
> > Errors go to jsconsole *only* if 'throw Components.error.NS_*;' goes to 
> > another javascript component! See QueryInterface of any js XPCOM. If you ask
> > for a wrong interface from c++, no error is logged.
> 
> Right, that's intentional, because C++ code "catches" those exceptions (where
> "catches" means "checks the return code and branches appropriately" in the C++
> context), and exceptions should only appear on the error console if they aren't
> caught by any caller in the caller chain.

I mean that if 'throw ...' goes to a c++ caller *without* this bug's patch, there is still no error logged. I routinely ask my js XPCOMs for interfaces they don't support. They reply by 'throw Components.results.NS_ERROR_NO_INTERFACE', but that is never logged.

Consequently, there must be a call from js module to nsIHandlerService::getTypeFromExtension()  somewhere in the client code, that is not wrapped in try{}catch{}, and therefore is causing the flood.

Comment 34

[Sergey Yanovich]

(In reply to comment #33)
> I mean that if 'throw ...' goes to a c++ caller *without* this bug's patch,
> there is still no error logged. I routinely ask my js XPCOMs for interfaces
> they don't support. They reply by 'throw
> Components.results.NS_ERROR_NO_INTERFACE', but that is never logged.
> 
> Consequently, there must be a call from js module to
> nsIHandlerService::getTypeFromExtension()  somewhere in the client code, that
> is not wrapped in try{}catch{}, and therefore is causing the flood.

Well, I am wrong here. Throwing Components.results.NS_ERROR_NO_INTERFACE from any other place except QI is logged.

Comment 35

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

(In reply to comment #34)
> (In reply to comment #33)
> > Consequently, there must be a call from js module to
> > nsIHandlerService::getTypeFromExtension()  somewhere in the client code, that
> > is not wrapped in try{}catch{}, and therefore is causing the flood.
> 
> Well, I am wrong here. Throwing Components.results.NS_ERROR_NO_INTERFACE from
> any other place except QI is logged.

Right.  I think QueryInterface is special-cased somewhere.

Comment 36

[Sergey Yanovich]

Comment on attachment 289663 [details] [diff] [review]
fix source of error messages

leaving this bug alone. new patch supplied to bug 404749

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #31)
>
> 'no type' is a legitimate result for the client, not an exceptional. As an
> analogy, 'SELECT 1 WHERE 1=2;' SQL statement returns an empty result set, but
> doesn't trigger an error.
>

After thinking this over some, I'll buy that a SELECT-style of API would be more suitable here.  I'm not totally convinced it's worth the time to change at this point, but if we do decide to do so, it should get its own new bug.

Comment 38

[Sergey Yanovich]

(In reply to comment #37)
> After thinking this over some, I'll buy that a SELECT-style of API would be
> more suitable here.  I'm not totally convinced it's worth the time to change at
> this point, but if we do decide to do so, it should get its own new bug.

I'll sign up for this, so that we can revert this patch and fix bug 405769. Should I change the title of bug 405769, or should I file a new bug?

Comment 39

[Dave Townsend [:mossop] (PTO until 6/3)]

Just because the nsIHandlerService case might change doesn't mean that this fix should be backed out. There are other valid situations where we should not be reporting these errors like bug 374499

Comment 40

[Sergey Yanovich]

(In reply to comment #39)
> Just because the nsIHandlerService case might change doesn't mean that this fix
> should be backed out. There are other valid situations where we should not be
> reporting these errors like bug 374499

I agree, that such situations are probably unavoidable. But for me, the applied solution looks worse than the problem itself. We want to suppress thrown and unhandled js exceptions, but we actually suppress all errors and exceptions with a registered nsresult code.

Maybe we need a more elaborate approach, like checking origin frame of exception, and suppressing logging only if origin frame is the current frame. If JSException doesn't have origin frame attribute, it well worths adding it.

In other words, one way or another, I would like this patch backed out.

Comment 41

[Dave Townsend [:mossop] (PTO until 6/3)]

(In reply to comment #40)
> I agree, that such situations are probably unavoidable. But for me, the applied
> solution looks worse than the problem itself. We want to suppress thrown and
> unhandled js exceptions, but we actually suppress all errors and exceptions
> with a registered nsresult code.

I don't agree with this. Unless I'm mistaken we don't want to suppress thrown and unhandled js exceptions. We do want to suppress js thrown exceptions with a particular nsresult code. It takes special effort to throw these which suggests that it is effectively an expected result.

From what I can tell this is what the patch does.

Comment 42

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think we should revert this fix. There is no reason to put the error in an error-console just because we're crossing a JS/C++ boundary. We don't log errors that go between JS components, or errors that go between C++ components, we only log errors that bubble up to top level without being handled. So why should boundaries between the two languages cause logging if it is later handled.

Comment 43

[Sergey Yanovich]

(In reply to comment #41)
> From what I can tell this is what the patch does.

(In reply to comment #42)
> I don't think we should revert this fix. There is no reason to put the
> error in an error-console just because we're crossing a JS/C++ boundary.

I believe, this should be logged (case 1):

function a() { b(); }
function b() { throw Component.result.NS_ERROR_UNEXPECTED; }

But this should not (case 2):

function a() { try { b(); } catch (e) { throw e.result; } }
function b() { throw Component.result.NS_ERROR_UNEXPECTED; }

This bug's fix caused both be suppressed. I think, that is a regression.

I am sorry to cause cross-posting, but the chances to be heard are higher here. Since the bug 405769 is filed to address this, everyone is invited to comment there :)

Comment 44

[Sylvain Pasche]

Apparently this also regressed the display in Error Console of some content errors (bug 415498).

Comment 45

[Jeffrey Baker]

Has this regressed?  I'm now getting "[Exception... "'Component is not available' when calling method: [nsIHandlerService::getTypeFromExtension]" nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)" location: "<unknown>" data: no]" with Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008041104 Minefield/3.0pre which I didn't get with Firefox 3.0 Beta 5.  It's not a very helpful message ....

Comment 46

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Partially, yes. Please see bug 415498 for discussion.

Comment 47

[Martijn Wargers (dead)]

So should this bug be reopened for that? Or should a new bug be filed? Or was a new bug filed for it already?

Comment 48

[Ben Turner (not reading bugmail, use the needinfo flag!)]

No, the behavior we have currently is the best compromise we can muster at this time (i.e. it's by design).