Closed Bug 877760 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: XPConnect, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: mossop, Assigned: gkrizsanits)

Details

Attachments

(2 files)

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);
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)
I think Blake probably has a better idea of what to do here.
Flags: needinfo?(mrbkap)
Weird... It looks like this exception gets reported twice even on the console without the try catch block. If it's caught then once.
(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.
So we could make an exception case for this...
Attachment #756565 - Flags: review?(mrbkap)
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.
Attachment #756568 - Flags: feedback?(mrbkap)
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.
Attachment #756565 - Flags: review?(mrbkap) → review+
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).
Attachment #756568 - Flags: feedback?(mrbkap) → feedback+
Flags: needinfo?(mrbkap)
(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 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.
(In reply to neil@parkwaycc.co.uk 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.
https://hg.mozilla.org/mozilla-central/rev/d8425335f52f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.