317937 - Possible IID/nsCOMPtr abuses

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[[e-mail]]

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*

Comment 1

[Boris Zbarsky [:bzbarsky]]

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

Comment 2

[[e-mail]]

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

Comment 3

[[e-mail]]

Attachment: Mail/News fixes

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

Comment 4

[[e-mail]]

Attachment: Odds and ends

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

Comment 5

[Johnny Stenback (:jst)]

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

sr=jst

Comment 6

[Darin Fisher]

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

Comment 7

[[e-mail]]

Attachment: Updated for review comments

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

[Darin Fisher]

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?

Comment 9

[[e-mail]]

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

[Darin Fisher]

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.

Comment 11

[[e-mail]]

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

Comment 12

[[e-mail]]

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