I audited the code by prepending a private static IID accessor to the NS_DECL_ISUPPORTS macro, which causes a compile error when someone tries to use NS_GET_IID on a concrete class. Some classes appear to legitimately have IIDs for their own reasons so I renamed the original macro for their continued use. These classes are: PrincipalHolder, XPCVariant, nsProxyInfo, nsDocLoader, nsProxyEventClass, nsProxyEventObject, WindowStateHolder, CNavDTD, ViewWrapper Mail/News Back End: # nsMsgFilter claims to use nsIMsgFilter's IID for no apparent reason # nsMsgTxn does not have an IID but nsMessenger uses do_QueryInterface to statically cast an nsITransaction* to an nsMsgTxn* # nsStatusBarBiffManager does not have an IID but nsMsgBiffManager uses an nsCOMPtr<nsStatusBarBiffManager> # CopyListener has an IID but only so that nsMsgCopy can use an nsCOMPtr<CopyListener> when it should be updated to an nsRefPtr # nsMsgCopy claims to use nsIMsgCopy's IID # QuotingOutputStreamListener does not have an IID but nsMsgQuoteListener tries it use it to statically cast an nsIStreamListener* to an QuotingOutputStreamListener* # nsMsgCompFields does not have an IID but nsMsgComposeAndSend tries to use an nsCOMPtr<nsMsgCompFields> when it should be updated to an nsRefPtr # nsMsgThread does not have an IID but claims to implement it ? nsImapMailCopyState has an IID, but I couldn't tell if static casts would be unsafe, so maybe this class belongs in the "legitimate" list Layout: # nsPresContext has an IID but only so that nsAccessibilityService can use do_QueryInterface to statically cast an nsISupports* to a nsPresContext* (the method would actually prefer an nsIPresShell* or nsIWeakReference* parameter) # nsMediaList does not have an IID but CSSImportRuleImpl uses do_QueryInterface to statically cast an nsIDOMMediaList* to an nsMediaList* Netwerk: # nsJARURI has an IID but only uses it internally so I'm guessing it should be following nsStandardURL's method instead # mozTXTToHTMLConv claims to use mozITXTToHTMLConv's IID for no apparent reason (indeed it is even commented with an XXX as potentially unnecessary) DocShell: # nsRefreshTimer does not have an IID but nsDocShell uses do_QueryInterface to statically cast an nsITimerCallback* to an nsRefreshTimer*

> # nsJARURI has an IID but only uses it internally so I'm guessing it should be > following nsStandardURL's method instead I did it this way on purpose, since the nsStandardURL method doesn't allow me to use type-safe stuff like do_QI and so on. It seems like we should file separate bugs on separate issued here (eg the accessibility service thing)...

(In reply to comment #1) >># nsJARURI has an IID but only uses it internally so I'm guessing it should be >>following nsStandardURL's method instead >I did it this way on purpose, since the nsStandardURL method doesn't allow me >to use type-safe stuff like do_QI and so on. nsRefPtr doesn't let you do that anyway, so no loss there. Oh, and nsDocumentEncoder claims to use nsIDocumentEncoder's IID.

I also removed the CopyListener's apparent self-reference cycle.

Feel free to suggest other diff options/reviewers/ways to fix the code.

Comment on attachment 204312 [details] [diff] [review] Odds and ends sr=jst

Comment on attachment 204312 [details] [diff] [review] Odds and ends >Index: mozilla/accessible/public/nsIAccessibilityService.idl >@@ -61,17 +61,17 @@ interface nsIAccessibilityService : nsIA > nsIAccessible createHTMLLIAccessible(in nsISupports aFrame, in nsISupports aBulletFrame, in AString aBulletText); > nsIAccessible createHTMLCheckboxAccessible(in nsISupports aFrame); > nsIAccessible createHTMLCheckboxAccessibleXBL(in nsIDOMNode aNode); >- nsIAccessible createHTMLComboboxAccessible(in nsIDOMNode aNode, in nsISupports aPresShell); >+ nsIAccessible createHTMLComboboxAccessible(in nsIDOMNode aNode, in nsIWeakReference aPresShell); > nsIAccessible createHTMLGenericAccessible(in nsISupports aFrame); > nsIAccessible createHTMLGroupboxAccessible(in nsISupports aFrame); > nsIAccessible createHTMLHRAccessible(in nsISupports aFrame); > nsIAccessible createHTMLImageAccessible(in nsISupports aFrame); > nsIAccessible createHTMLLabelAccessible(in nsISupports aFrame); >- nsIAccessible createHTMLListboxAccessible(in nsIDOMNode aNode, in nsISupports aPresShell); >+ nsIAccessible createHTMLListboxAccessible(in nsIDOMNode aNode, in nsIWeakReference aPresShell); > nsIAccessible createHTMLObjectFrameAccessible(in nsObjectFrame aFrame); > nsIAccessible createHTMLRadioButtonAccessible(in nsISupports aFrame); > nsIAccessible createHTMLRadioButtonAccessibleXBL(in nsIDOMNode aNode); >- nsIAccessible createHTMLSelectOptionAccessible(in nsIDOMNode aNode, in nsIAccessible aAccParent, in nsISupports aPresShell); >+ nsIAccessible createHTMLSelectOptionAccessible(in nsIDOMNode aNode, in nsIAccessible aAccParent, in nsIWeakReference aPresShell); > nsIAccessible createHTMLTableAccessible(in nsISupports aFrame); > nsIAccessible createHTMLTableCellAccessible(in nsISupports aFrame); > nsIAccessible createHTMLTableCaptionAccessible(in nsIDOMNode aDOMNode); Changes to any interface require a UUID change. >Index: mozilla/modules/libjar/nsJARURI.cpp >- if (aIID.Equals(NS_GET_IID(nsJARURI))) >+ if (aIID.Equals(kThisImplCID)) Why are you making this change? What is wrong with the use of: >- NS_DECLARE_STATIC_IID_ACCESSOR(NS_THIS_JARURI_IMPL_CID) >-NS_DEFINE_STATIC_IID_ACCESSOR(nsJARURI, NS_THIS_JARURI_IMPL_CID) for a class like nsJARURI? Those macros are used with hand written interfaces, so why can't they be used with concrete classes? I find that this way of QI'ing to a private implementation class is very handy and somewhat clean IMO. >Index: mozilla/layout/style/nsCSSRules.cpp >- mMedia = do_QueryInterface(mediaList); >+ mMedia = NS_STATIC_CAST(nsMediaList*, NS_STATIC_CAST(nsIDOMMediaList*, mediaList.get())); nit: break long lines at 80 chars

* Changed nsIAccesibilityService's IID * Removed nsJARURI changes * Wrapped long line in nsCSSRules.cpp If you use NS_DECLARE_STATIC_IID_ACCESSOR then someone might be tempted to use other macros such as NS_INTERFACE_MAP_ENTRY which would fail.

Comment on attachment 204683 [details] [diff] [review] Updated for review comments >Index: mozilla/layout/base/nsPresContext.h >-// hack to make egcs / gcc 2.95.2 happy >-class nsPresContext_base : public nsIObserver >-{ >-public: >- NS_DECLARE_STATIC_IID_ACCESSOR(NS_IPRESCONTEXT_IID) >-}; So, you have found that this hack is no longer necessary? Or, are you just breaking support for gcc 2.95.2?

(In reply to comment #8) >(From update of attachment 204683 [details] [diff] [review]) >>Index: mozilla/layout/base/nsPresContext.h > >>-// hack to make egcs / gcc 2.95.2 happy >>-class nsPresContext_base : public nsIObserver >>-{ >>-public: >>- NS_DECLARE_STATIC_IID_ACCESSOR(NS_IPRESCONTEXT_IID) >>-}; >So, you have found that this hack is no longer necessary? Or, are >you just breaking support for gcc 2.95.2? Both, probably; compare http://lxr.mozilla.org/mozilla1.7/source/view/public/nsIView.h#128 http://lxr.mozilla.org/mozilla1.8/source/view/public/nsIView.h#102 where I didn't remove the IID ;-)

Comment on attachment 204683 [details] [diff] [review] Updated for review comments ok, well... if someone complains about this breaking the GCC 2.95.2 build, then i guess we can put it back.

(In reply to comment #10) >(From update of attachment 204683 [details] [diff] [review]) >ok, well... if someone complains about this breaking the GCC 2.95.2 build, then >i guess we can put it back. ecgs complains if you have a class with its own IID that multiply inherits from nsISupports because then it can't unambiguously resolve ::nsISupports::GetIID.

Fix checked in, modulo bustage fix (bug 318750).