179268 - Fix abuse of QueryInterface in toolkit.jar

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

Two cases:
1. Use of try { .QueryInterface } catch instead of instanceof
2. Useless .QueryInterface (already correct interface)

Comment 1

[[e-mail]]

Um, now where did I put that patch?

Comment 2

[Hixie (not reading bugmail)]

This component is retired and should not be used.

->XP Apps.

Comment 3

[[e-mail]]

Attachment: Proposed patch

Comment 4

[timeless]

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

[[e-mail]]

>+	   // 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

[[e-mail]]

Attachment: Whoops!

+  if (!param instanceof Components.interfaces.nsIDialogParamBlock)
should be
+  if (!(param instanceof Components.interfaces.nsIDialogParamBlock))

Comment 7

[[e-mail]]

Comment on attachment 109103 [details] [diff] [review]
Whoops!

Gotta keep that queue topped up ;-)

Comment 8

[Boris Zbarsky [:bzbarsky]]

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

[[e-mail]]

>>--- 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

[Boris Zbarsky [:bzbarsky]]

> 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

[[e-mail]]

It looks as if the if (!aObject.message) return; is a mistake.

Comment 12

[timeless]

*** Bug 232630 has been marked as a duplicate of this bug. ***

Comment 13

[[e-mail]]

Attachment: consoleBindings.xml [Checkin: Comment 23]

Comment 14

[[e-mail]]

Attachment: Other stuff

Comment 15

[Boris Zbarsky [:bzbarsky]]

So... if the consoleservice is holding a ref to us, why would our destructor get
called?

Comment 16

[[e-mail]]

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

[Boris Zbarsky [:bzbarsky]]

I'd rather we did not make assumptions like that....  especially because that
assumes that there are no gc cycles around.

Comment 18

[[e-mail]]

Actually it occurs to me that the current code already has a reference loop:
console service -> observer -> binding -> console service

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

[timeless]

i'm fairly certain we leak (until shutdown), i mentioned it in house recently :).

Comment 21

[Serge Gautherie (:sgautherie)]

Neil,
Are you still working on this ?

Comment 22

[[e-mail]]

No, I assume I've fixed all the interesting cases by now.

Comment 23

[Serge Gautherie (:sgautherie)]

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

[Serge Gautherie (:sgautherie)]

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 ... ?