Last Comment Bug 179268 - Fix abuse of QueryInterface in toolkit.jar
: Fix abuse of QueryInterface in toolkit.jar
Status: RESOLVED FIXED
: perf, polish
Product: SeaMonkey
Classification: Client Software
Component: UI Design (show other bugs)
: Trunk
: x86 Windows 98
-- minor (vote)
: ---
Assigned To: neil@parkwaycc.co.uk
: John Morrison
:
Mentors:
: 232630 (view as bug list)
Depends on: 282253
Blocks: 344955
  Show dependency treegraph
 
Reported: 2002-11-09 12:40 PST by neil@parkwaycc.co.uk
Modified: 2008-06-04 09:53 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed patch (6.35 KB, patch)
2002-11-11 08:54 PST, neil@parkwaycc.co.uk
timeless: review+
Details | Diff | Splinter Review
Whoops! (6.35 KB, patch)
2002-12-12 01:31 PST, neil@parkwaycc.co.uk
bzbarsky: superreview-
Details | Diff | Splinter Review
consoleBindings.xml [Checkin: Comment 23] (3.65 KB, patch)
2004-06-19 11:23 PDT, neil@parkwaycc.co.uk
timeless: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Other stuff (3.69 KB, patch)
2004-06-19 13:55 PDT, neil@parkwaycc.co.uk
no flags Details | Diff | Splinter Review

Description User image neil@parkwaycc.co.uk 2002-11-09 12:40:50 PST
Two cases:
1. Use of try { .QueryInterface } catch instead of instanceof
2. Useless .QueryInterface (already correct interface)
Comment 1 User image neil@parkwaycc.co.uk 2002-11-09 12:43:05 PST
Um, now where did I put that patch?
Comment 2 User image Hixie (not reading bugmail) 2002-11-11 08:36:10 PST
This component is retired and should not be used.

->XP Apps.
Comment 3 User image neil@parkwaycc.co.uk 2002-11-11 08:54:43 PST
Created attachment 105834 [details] [diff] [review]
Proposed patch
Comment 4 User image timeless 2002-11-11 11:32:17 PST
Comment on attachment 105834 [details] [diff] [review]
Proposed patch

Sun Nov 10 22:16:30 2002
@@ -130,24 +130,13 @@
+	   // Try to QI it to a script error to get more info
change the comment

-	     if (!this.showChromeErrors && scriptError.sourceName.substr(0, 9)
== "chrome://")
+	     if (this.showChromeErrors || !/^chrome:/.test(aObject.sourceName))

technically these two aren't equivalent (chrome: != chrome://), but i think
that change is ok.

-	       var msg =
aObject.QueryInterface(Components.interfaces.nsIConsoleMessage);
-	       this.appendMessage(msg.message);

+	     this.appendMessage(aObject.message);
i think you need 
if (aObject instanceof Components.interfaces.nsIConsoleMessage)
  this.appendMessage(aObject.message);
else 
  this.appendMessage(aObject);

otherwise xpconnect might not expose the message field even if it does exist.
Comment 5 User image neil@parkwaycc.co.uk 2002-11-12 03:33:27 PST
>+	   // Try to QI it to a script error to get more info
>change the comment
Well, instanceof does try to QI it... what did you have in mind?

>-	     if (!this.showChromeErrors && scriptError.sourceName.substr(0, 9)
== "chrome://")
>+	     if (this.showChromeErrors || !/^chrome:/.test(aObject.sourceName))
>technically these two aren't equivalent (chrome: != chrome://), but i think
>that change is ok.
So I was too lazy to write \/\/ ...

>-	       var msg =
aObject.QueryInterface(Components.interfaces.nsIConsoleMessage);
>-	       this.appendMessage(msg.message);
>+	     this.appendMessage(aObject.message);
>i think you need 
>if (aObject instanceof Components.interfaces.nsIConsoleMessage)
>  this.appendMessage(aObject.message);
>else
>  this.appendMessage(aObject);
>otherwise xpconnect might not expose the message field even if it does exist.
Did you see the first line of the method?
if (!aObject.message) return;
(the idl defines the callback parameter as an nsIConsoleMessage)
Comment 6 User image neil@parkwaycc.co.uk 2002-12-12 01:31:12 PST
Created attachment 109103 [details] [diff] [review]
Whoops!

+  if (!param instanceof Components.interfaces.nsIDialogParamBlock)
should be
+  if (!(param instanceof Components.interfaces.nsIDialogParamBlock))
Comment 7 User image neil@parkwaycc.co.uk 2003-08-10 13:59:34 PDT
Comment on attachment 109103 [details] [diff] [review]
Whoops!

Gotta keep that queue topped up ;-)
Comment 8 User image Boris Zbarsky [:bz] (still a bit busy) 2003-08-10 14:31:04 PDT
Comment on attachment 109103 [details] [diff] [review]
Whoops!

>--- embedding/components/ui/progressDlg/nsProgressDlg.js
>+++ embedding/components/ui/progressDlg/nsProgressDlg.js	

This file no longer exists.

> xpfe/components/console/resources/content/consoleBindings.xml	

>+          // Try to QI it to a script error to get more info

Is this comment still correct?	Perhaps "check whether it's a script error"?

>+          if (aObject instanceof Components.interfaces.nsIScriptError) {

>+              this.appendError(aObject);

appendError expects to get an nsIScriptError object (it will access properties
that live on the nsIScriptError interface).  Are you relying on the fact that
the QI from the instanceof will cache the properties of that interface on
aObject?  If so, please document that, for it is not clear from this code.

>+            this.appendMessage(aObject.message);

What timeless said -- if no one has tried to QI aObject to nsIConsoleMessage,
then .message will be null or undefined, even if the object has a message
(because the property's existence will not be cached on the object).

>+++ xpfe/global/resources/content/selectDialog.js

I have to admit that I'm not sure why you made this change.  We are trying to
access a property of the nsIDialogParamBlock interface off window.arguments[0].
 QIing it to that interface seems like the right way to go; why is using
instanceof better here?

>+++ xpfe/global/resources/content/nsDragAndDrop.js

>-          transArray.AppendElement(trans.QueryInterface(Components.interfaces.nsISupports));
>+          transArray.AppendElement(trans);

Is this correct?  This does not do an implicit QI, does it?  Are you sure that
the consumers of transArray will never do pointer comparisons on the stuff you
put in it?
Comment 9 User image neil@parkwaycc.co.uk 2003-08-11 02:15:13 PDT
>>--- embedding/components/ui/progressDlg/nsProgressDlg.js
>>+++ embedding/components/ui/progressDlg/nsProgressDlg.js	
>This file no longer exists.
Such is life :-/

>>+            this.appendMessage(aObject.message);
>What timeless said -- if no one has tried to QI aObject to nsIConsoleMessage,
>then .message will be null or undefined, even if the object has a message
>(because the property's existence will not be cached on the object).
It's quite difficult to call a console listener without passing it an
nsIConsoleMessage... if you're that worried then check should be done earlier.

Actually now that I think about it, the binding should be its own listener. But
that's a separate bug.

>>+++ xpfe/global/resources/content/selectDialog.js
>I have to admit that I'm not sure why you made this change.  We are trying to
>access a property of the nsIDialogParamBlock interface off window.arguments[0].
>QIing it to that interface seems like the right way to go; why is using
>instanceof better here?
Well, you could QI it, but the issue is that you can't use if (x.QI(C.I.y))
because x.QI never returns null, it throws an exception if it fails.

>>+++ xpfe/global/resources/content/nsDragAndDrop.js
>>-         
transArray.AppendElement(trans.QueryInterface(Components.interfaces.nsISupports));
>>+          transArray.AppendElement(trans);
>Is this correct?  This does not do an implicit QI, does it?  Are you sure that
>the consumers of transArray will never do pointer comparisons on the stuff you
>put in it?
It should do if necessary (XPConnect might already have the nsISupports
pointer); it certainly won't try anything silly like casting it.
Comment 10 User image Boris Zbarsky [:bz] (still a bit busy) 2003-08-11 09:00:01 PDT
> if you're that worried

I'm worried about creating fragile code that depends on random side-effects that
happened half a file away in code that can change very much independently from
this.  At the very least, clearly document the far-reaching assumptions you are
making at the point when you make them (and possibly at the point where the code
your code depends on lives).

> Well, you could QI it, but the issue is that you can't use if (x.QI(C.I.y))
> because x.QI never returns null, it throws an exception if it fails.

So you catch the exception.  That still gives clearer code (put all the use of
C.I.y inside the try block, etc).
Comment 11 User image neil@parkwaycc.co.uk 2003-08-11 14:34:22 PDT
It looks as if the if (!aObject.message) return; is a mistake.
Comment 12 User image timeless 2004-06-15 07:12:42 PDT
*** Bug 232630 has been marked as a duplicate of this bug. ***
Comment 13 User image neil@parkwaycc.co.uk 2004-06-19 11:23:27 PDT
Created attachment 151216 [details] [diff] [review]
consoleBindings.xml
[Checkin: Comment 23]
Comment 14 User image neil@parkwaycc.co.uk 2004-06-19 13:55:07 PDT
Created attachment 151232 [details] [diff] [review]
Other stuff
Comment 15 User image Boris Zbarsky [:bz] (still a bit busy) 2005-02-06 16:33:17 PST
So... if the consoleservice is holding a ref to us, why would our destructor get
called?
Comment 16 User image neil@parkwaycc.co.uk 2005-02-06 16:52:02 PST
It occurs to me that this patch precedes your "XBL constructor and destructor
should fire on node creation/destruction" bug, which meant that I was able to
assume that the destructor would fire when the window was closed.
Comment 17 User image Boris Zbarsky [:bz] (still a bit busy) 2005-02-06 17:20:46 PST
I'd rather we did not make assumptions like that....  especially because that
assumes that there are no gc cycles around.
Comment 18 User image neil@parkwaycc.co.uk 2005-02-07 01:08:01 PST
Actually it occurs to me that the current code already has a reference loop:
console service -> observer -> binding -> console service
Comment 19 User image Boris Zbarsky [:bz] (still a bit busy) 2005-02-07 09:28:05 PST
Comment on attachment 151216 [details] [diff] [review]
consoleBindings.xml
[Checkin: Comment 23]

OK.. sr=bzbarsky, but it's not clear to me how (or whether) we avoid leaking
this stuff....
Comment 20 User image timeless 2005-02-08 19:33:46 PST
i'm fairly certain we leak (until shutdown), i mentioned it in house recently :).
Comment 21 User image Serge Gautherie (:sgautherie) 2008-06-02 09:38:56 PDT
Neil,
Are you still working on this ?
Comment 22 User image neil@parkwaycc.co.uk 2008-06-04 07:42:20 PDT
No, I assume I've fixed all the interesting cases by now.
Comment 23 User image Serge Gautherie (:sgautherie) 2008-06-04 09:43:22 PDT
Comment on attachment 151216 [details] [diff] [review]
consoleBindings.xml
[Checkin: Comment 23]

{{
1.19	neil%parkwaycc.co.uk	2005-02-07 09:39	 	Clean up use of QueryInterface in consoleBindings.xml b=179268 r=timeless sr=bz
}}
Comment 24 User image Serge Gautherie (:sgautherie) 2008-06-04 09:53:11 PDT
Comment on attachment 151232 [details] [diff] [review]
Other stuff

*CPP file/code still there.
*Toolkit has the same JS code as Xpfe had.

Is this patch to be obsoleted or ... ?

Note You need to log in before you can comment on or make changes to this bug.