QueryInterface impls in JS that use throw get annoying when venkman is running because an exception occurs. QueryInterface failing is not an exceptional case. xpconnect exposes a better way to implement the QI method.

Comment on attachment 148551 [details] [diff] [review] draft [Checkin: See comment 17+22] r=me where applicable, but where you renamed theUID to aUID I think you might as well have renamed it to aIID.

Comment on attachment 148551 [details] [diff] [review] draft [Checkin: See comment 17+22] I presume this is a patch w/ whitespace changes stripped, right? because there are indentation problems with lines that you did not appear to change. is there an xml-rpc "owner" that should look over your non-trivial changes to that module? sr=darin

+ Components.returnCode = Components.results.NS_ERROR_NO_INTERFACE; + return null; Does this Components.returnCode thing work in Mozilla 1.0? (I can't seem to find any definition of it except http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpcjsruntime.cpp#52 which is less than useful.) (In reply to comment #3) > (From update of attachment 148551 [details] [diff] [review]) > I presume this is a patch w/ whitespace changes stripped, right? You can actually tell from the diff lines: diff -uwp -r1.304 browser/base/content/browser.js :)

see url

How does this cooperate with js callers into QueryInterface? Did you check if these rely on the exception being thrown? Even on mozdev. Shaver, does this have any implication on your mono work?

This seems strange to me too, XPCOM defines QueryInterface to throw when failing and though this seems like a bad idea breaking such a core contract seems even worse. Wouldn't it be better to hack venkman to ignore throws inside of QI implementations?

I very much dislike the Components.resultCode setting. What does it mean to set it and then return non-null? (It means that the return value is ignored, which is not at all what people will expect.) I think this is a venkman usability/heuristic enhancement request, plain and simple.

silver, http://www.mozilla.org/scriptable/components_object.html#_returnCode describes this

(In reply to comment #5) > see url (In reply to comment #9) > silver, http://www.mozilla.org/scriptable/components_object.html#_returnCode > describes this Thanks, though that page says it should be throwing to indicate failure normally. :) The page is also almost unreadable here (light grey text on white!), since it sets bgcolor but not text - who do I kick/bug about that? As for the patch, the ChatZilla changes look good to me, assuming this goes ahead in the end.

Here's an untested hack that adds a pref named "ignoreNoInterface" to venkman. If it works, it could be trivially added to the UI somewhere. To enable the hack, start venkman and type ``/pref ignoreNoInterface on''.

Comment on attachment 148758 [details] [diff] [review] add ignoreNoInterface pref to venkman [Moved to bug 452288] That's what I'm talking about. Even better might be to only suppress when in methods named QueryInterface, but that's probably not worth it.

QI methods have a tendency to be anonymous.

(In reply to comment #6) >How does this cooperate with js callers into QueryInterface? I'm not aware of JS code calling QueryInterface on JS objects. Wrapped JS objects throw an exception as expected.

This bug isn't resolved as FIXED (yet) so now I wonder if Components.returnCode should be used instead of a throw for QueryInterface/getClassObject? I guess that's the norm now that the 'draft' patch was applied, right?

AFAICT the draft patch was never checked in, and I hope it seems like a bad solution to me.

(In reply to comment #16) > AFAICT the draft patch was never checked in, and I hope it seems like a bad > solution to me. I always thought that venkman was fixed, but it seems that 'draft' patch went in anyway: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-05-14+17%3A01&maxdate=2004-05-17+17%3A03&cvsroot=%2Fcvsroot

grmbl.. i would really like to see that backed out.

I too think that the 'new' way is ugly and non-JS like. Note that with bug 287107 it gets particularly ugly.

If someone writes a backout patch i'll happily sr it.

FWIW, I really only intended that Components.returnCode would be used for success codes. Failure is *supposed* to be indicated by throwing. I meant what I said in... http://www.mozilla.org/scriptable/components_object.html#_returnCode

On the bright side, landing in browser and toolkit in 2004 was too much trouble, so only part of the patch actually did land. On the not-so-bright side, it looks like someone at Google actually read xpcom/sample/nsSample.js, so it metastasized throughout safebrowsing.

Also note bug 452288 - comment 11 in this bug seems to reflect it.

Comment on attachment 356367 [details] [diff] [review] mozilla-central backout [checked in] http://hg.mozilla.org/mozilla-central/rev/4cae4182a42d

Comment on attachment 356371 [details] [diff] [review] comm-central backout [checked in] http://hg.mozilla.org/comm-central/rev/5d42e53ff9b1

Comment on attachment 356371 [details] [diff] [review] comm-central backout [checked in] It looks like comm-central would have 3+1 more occurrences to fix: http://mxr.mozilla.org/comm-central/search?string=Components%5C.returnCode&regexp=on&case=on&find=%2FnsLDAPDataSource%5C.js%24 http://mxr.mozilla.org/comm-central/search?string=Components%5C.returnCode&regexp=on&case=on&find=%2FcalProviderUtils%5C.jsm%24

Comment on attachment 356367 [details] [diff] [review] mozilla-central backout [checked in] It looks like mozilla-central might have 1+1+12 more occurrences to fix: http://mxr.mozilla.org/mozilla-central/search?string=Components%5C.returnCode&regexp=on&case=on

Then, there would be the 2 (cvs) ChatZilla occurrences: http://mxr.mozilla.org/comm-central/search?string=Components%5C.returnCode&regexp=on&case=on&find=%2Fchatzilla-service%5C.js%24 (That's all for m-c and c-c.)

Sounds like you got confused by the subtle difference between "Back out using returnCode instead of throwing in QueryInterface implementations" and "Satan wrote returnCode and anyone who uses loses their immortal soul, we must stamp it out wherever it occurs."

Then, I filed bug 474120, bug 474121, bug 474122, bug 474124 and bug 474125.