Closed Bug 243621 Opened 17 years ago Closed 12 years ago

(Back out) Standardize QueryInterface without throw

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: timeless, Assigned: philor)

References

()

Details

Attachments

(3 files, 1 obsolete file)

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.
Attachment #148551 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #148551 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #148551 - Flags: superreview?(darin)
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
Attachment #148551 - Flags: superreview?(darin) → superreview+
Whiteboard: [seeking moa]
+    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.
(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.
Product: Core → Mozilla Application Suite
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
Component: XP Apps: GUI Features → UI Design
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.
Assignee: timeless → philringnalda
Attachment #148551 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #356367 - Flags: superreview?(jonas)
Attachment #356367 - Flags: review?(timeless)
Component: UI Design → General
Product: SeaMonkey → Core
QA Contact: general
Whiteboard: [seeking moa]
Attachment #356367 - Flags: superreview?(jonas) → superreview+
OS: Windows XP → All
Hardware: x86 → All
Summary: Standardize QueryInterface without throw → (Back out) Standardize QueryInterface without throw
Attachment #356371 - Flags: superreview?(bugzilla)
Attachment #356371 - Flags: review?(timeless)
Attachment #356371 - Flags: review?(timeless) → review+
Attachment #356367 - Flags: review?(timeless) → review+
Also note bug 452288 - comment 11 in this bug seems to reflect it.
Attachment #356371 - Flags: superreview?(bugzilla) → superreview+
Comment on attachment 356367 [details] [diff] [review]
mozilla-central backout [checked in]

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

http://hg.mozilla.org/comm-central/rev/5d42e53ff9b1
Attachment #356371 - Attachment description: comm-central backout → comm-central backout [checked in]
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
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
Attachment #148551 - Attachment description: draft → draft [Checkin: See comment 17+22]
Attachment #148551 - Attachment is obsolete: false
Attachment #148758 - Attachment description: add ignoreNoInterface pref to venkman → add ignoreNoInterface pref to venkman [Moved to bug 452288]
Attachment #148758 - Attachment is obsolete: true
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."
You need to log in before you can comment on or make changes to this bug.