Fix abuse of QueryInterface in toolkit.jar

RESOLVED FIXED

Status

SeaMonkey
UI Design
--
minor
RESOLVED FIXED
15 years ago
9 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: neil@parkwaycc.co.uk)

Tracking

({perf, polish})

Trunk
x86
Windows 98
perf, polish
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

15 years ago
Um, now where did I put that patch?
This component is retired and should not be used.

->XP Apps.
Assignee: waterson → neil
Component: XP Miscellany → XP Apps
(Assignee)

Comment 3

15 years ago
Created attachment 105834 [details] [diff] [review]
Proposed patch
(Assignee)

Updated

15 years ago
Keywords: patch, perf, polish, review

Comment 4

15 years ago
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.
Attachment #105834 - Flags: review+

Updated

15 years ago
QA Contact: brendan → jrgm
(Assignee)

Comment 5

15 years ago
>+	   // 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)
Status: NEW → ASSIGNED
(Assignee)

Updated

15 years ago
Attachment #105834 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 6

15 years ago
Created attachment 109103 [details] [diff] [review]
Whoops!

+  if (!param instanceof Components.interfaces.nsIDialogParamBlock)
should be
+  if (!(param instanceof Components.interfaces.nsIDialogParamBlock))
Attachment #105834 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #105834 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 7

14 years ago
Comment on attachment 109103 [details] [diff] [review]
Whoops!

Gotta keep that queue topped up ;-)
Attachment #109103 - Flags: superreview?(bz-vacation)
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?
Attachment #109103 - Flags: superreview?(bz-vacation) → superreview-
(Assignee)

Comment 9

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

Comment 11

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

Comment 12

13 years ago
*** Bug 232630 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 13

13 years ago
Created attachment 151216 [details] [diff] [review]
consoleBindings.xml
[Checkin: Comment 23]
(Assignee)

Updated

13 years ago
Attachment #151216 - Flags: review?(timeless)
(Assignee)

Comment 14

13 years ago
Created attachment 151232 [details] [diff] [review]
Other stuff
Attachment #109103 - Attachment is obsolete: true
Product: Core → Mozilla Application Suite

Updated

12 years ago
Attachment #151216 - Flags: review?(timeless) → review+
(Assignee)

Updated

12 years ago
Attachment #151216 - Flags: superreview?(bzbarsky)
So... if the consoleservice is holding a ref to us, why would our destructor get
called?
(Assignee)

Comment 16

12 years ago
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.
I'd rather we did not make assumptions like that....  especially because that
assumes that there are no gc cycles around.
(Assignee)

Comment 18

12 years ago
Actually it occurs to me that the current code already has a reference loop:
console service -> observer -> binding -> console service
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....
Attachment #151216 - Flags: superreview?(bzbarsky) → superreview+

Comment 20

12 years ago
i'm fairly certain we leak (until shutdown), i mentioned it in house recently :).
Depends on: 282253
Blocks: 344955
Neil,
Are you still working on this ?
(Assignee)

Comment 22

9 years ago
No, I assume I've fixed all the interesting cases by now.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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
}}
Attachment #151216 - Attachment description: consoleBindings.xml → consoleBindings.xml [Checkin: Comment 23]
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 ... ?
You need to log in before you can comment on or make changes to this bug.