877760 - XPConnect always logs an error when a component doesn't implement a method

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Dave Townsend [:mossop]]

If a method isn't implemented in a JS component then calling it will throw an exception and log an error to the error console.

This code example runable in a browser scratchpad logs an error even though the thrown exception is caught and ignored:

Components.utils.import("resource://gre/modules/Services.jsm");

var component = {
  QueryInterface: function(iid) {
    if (iid.equals(Components.interfaces.nsIChannel))
      return this;
  }
}

Services.obs.addObserver(function(subject) {
  Services.obs.removeObserver(arguments.callee, "foobar");

  try {
    subject.QueryInterface(Components.interfaces.nsIChannel);
    if (("open" in subject) && ((typeof subject.open) == "function"))
      subject.open();
  }
  catch (e) { }
}, "foobar", false);

Services.obs.notifyObservers(component, "foobar", null);

Comment 1

[Gabor Krizsanits (INACTIVE)]

So the problem here is that when someone wants to implement an interface from javascript, it's really cumbersome to add a bunch of dummy function that we don't really need to implement. As the example shows there is just no way to avoid spamming on the error console even if we are catching the exception.

This doc https://developer.mozilla.org/en-US/docs/Exception_logging_in_JavaScript says that we always throw from xpconnect/xpidl but since what we have here is a js implemented component, I don't think we should enforce that practice here. Any reason why we must log here, or can I change that? (exception thrown: NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED)

Comment 2

[Bobby Holley (:bholley)]

I think Blake probably has a better idea of what to do here.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Weird... It looks like this exception gets reported twice even on the console without the try catch block. If it's caught then once.

Comment 4

[Gabor Krizsanits (INACTIVE)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #3)
> Weird... It looks like this exception gets reported twice even on the
> console without the try catch block. If it's caught then once.

Exception logging is weird. It's done in nsXPCWrappedJSClass::CheckForException. Here is what happens. Exception thrown and logged from subject.open(); and then passed from the native context to the js context, where the js code might or might not catches it. If the try/catch part is commented out in this example, it's not getting caught and the exception is passed from the js context to the other native context: nsObserverList::NotifyObservers which will detect that an xpc exception occured and re-logs it again. This way the same exception might be logged to the error console multiple times.

Comment 5

[Gabor Krizsanits (INACTIVE)]

Attachment: XPConnect always logs an error when a component doesn't implement a method v1

So we could make an exception case for this...

Comment 6

[Gabor Krizsanits (INACTIVE)]

Attachment: Make sure xpconnect exceptions logged only once v1

And I don't think it makes sense to log the same exception twice. I need to test this, just wanted to ask your opinion in the meanwhile if there is any reason to log the same exception more than once.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 756565 [details] [diff] [review]
XPConnect always logs an error when a component doesn't implement a method v1

Based on my reading of the bug, this is the most conservative solution, so we should definitely get this in and see if anybody complains.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 756568 [details] [diff] [review]
Make sure xpconnect exceptions logged only once v1

I think we should do this in another bug (and bholley should probably review it). It feels a little odd to me to have this back channel on nsIScriptError, but I guess nobody else implements it, so it's not that much of a leaky abstraction, but XPConnect remembering that it's already reported an error does make sense to avoid double reporting (of course, this won't entirely prevent this problem since other code can also log exceptions).

Comment 9

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=11d4d0812bd1

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8425335f52f

Comment 10

[Gabor Krizsanits (INACTIVE)]

(In reply to Blake Kaplan (:mrbkap) from comment #8)
> I think we should do this in another bug (and bholley should probably review
> it).

Bug 881647

Comment 11

[[e-mail]]

Comment on attachment 756565 [details] [diff] [review]
XPConnect always logs an error when a component doesn't implement a method v1

>                 if (reportable && e_result == NS_ERROR_NO_INTERFACE &&
>                     !strcmp(anInterfaceName, "nsIInterfaceRequestor") &&
>                     !strcmp(aPropertyName, "getInterface")) {
>                     reportable = false;
>                 }
>+
>+                // More special case, see bug 877760.
>+                if (e_result != NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED) {
>+                    reportable = false;
>+                }
Is this even correct? We want to disable reporting when the result was that there was no function of that name, but you've used != here.

Comment 12

[Gabor Krizsanits (INACTIVE)]

(In reply to [e-mail] from comment #11)
> Is this even correct? We want to disable reporting when the result was that
> there was no function of that name, but you've used != here.

No, it's not. I fixed it in the patch I pushed to try/inbound, just forgot to add a comment here it seems. Sorry about that, it was a copy paste mistake from a previous version where the != made sense.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8425335f52f