(Back out) Standardize QueryInterface without throw

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
15 years ago
11 years ago

People

(Reporter: timeless, Assigned: philor)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

15 years ago
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.
Reporter

Comment 1

15 years ago
Reporter

Updated

15 years ago
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+
Reporter

Updated

15 years ago
Attachment #148551 - Flags: superreview?(darin)

Comment 3

15 years ago
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+
Reporter

Updated

15 years ago
Whiteboard: [seeking moa]

Comment 4

15 years ago
+    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
:)
Reporter

Comment 5

15 years ago
see url

Comment 6

15 years ago
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.

Comment 10

15 years ago
(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.

Comment 11

15 years ago
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.
Reporter

Comment 13

15 years ago
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

Comment 15

14 years ago
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.

Comment 17

14 years ago
(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.

Comment 21

13 years ago
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
Reporter

Updated

11 years ago
Component: XP Apps: GUI Features → UI Design
Assignee

Comment 22

11 years ago
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)
Assignee

Updated

11 years ago
Component: UI Design → General
Product: SeaMonkey → Core
QA Contact: general
Whiteboard: [seeking moa]
Attachment #356367 - Flags: superreview?(jonas) → superreview+
Assignee

Updated

11 years ago
OS: Windows XP → All
Hardware: x86 → All
Summary: Standardize QueryInterface without throw → (Back out) Standardize QueryInterface without throw
Assignee

Comment 23

11 years ago
Attachment #356371 - Flags: superreview?(bugzilla)
Attachment #356371 - Flags: review?(timeless)
Reporter

Updated

11 years ago
Attachment #356371 - Flags: review?(timeless) → review+
Reporter

Updated

11 years ago
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+
Assignee

Comment 25

11 years ago
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]
Assignee

Comment 26

11 years ago
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]
Assignee

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Closed: 11 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
Assignee

Comment 30

11 years ago
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.