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

Um, now where did I put that patch?

This component is retired and should not be used. ->XP Apps.

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.

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

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

Comment on attachment 109103 [details] [diff] [review] Whoops! Gotta keep that queue topped up ;-)

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?

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

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

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

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

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.

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

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

Neil, Are you still working on this ?

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

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