394114 - Interfaces missing from various QI implementations.

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[jag (Peter Annema)]

If nsIBar derives from nsIFoo and your QI has an entry for nsIBar, it should typically also have an entry for nsIFoo. With some creative grepping and a bit of perl I came up with a list of "offenders" of this rule. Note that this list may contain some false positives.

Format is "missing interface" : "class" in "file" ("derived interface")

imgIContainerObserver : nsGenConImageContent in content/base/src/nsGenConImageContent.cpp (nsIImageLoadingContent)
imgIContainerObserver : nsHTMLImageElement in content/html/content/src/nsHTMLImageElement.cpp (nsIImageLoadingContent)
imgIContainerObserver : nsHTMLInputElement in content/html/content/src/nsHTMLInputElement.cpp (nsIImageLoadingContent)
imgIContainerObserver : nsHTMLObjectElement in content/html/content/src/nsHTMLObjectElement.cpp (nsIImageLoadingContent)
imgIContainerObserver : nsHTMLSharedObjectElement in content/html/content/src/nsHTMLSharedObjectElement.cpp (nsIImageLoadingContent)
jsdIEphemeral : jsdContext in js/jsd/jsd_xpc.cpp (jsdIContext)
nsIContentSink : RobotSink in parser/htmlparser/robot/nsRobotSink.cpp (nsIRobotSink)
nsIContentSink : nsLoadSaveContentSink in content/xml/document/src/nsLoadSaveContentSink.cpp (nsIXMLContentSink)
nsIDOMEventListener : nsComboButtonListener in layout/forms/nsComboboxControlFrame.cpp (nsIDOMMouseListener)
nsIDOMEventListener : nsDOMParser in content/base/src/nsDOMParser.cpp (nsIDOMLoadListener)
nsIDOMEventListener : nsFileControlFrame::MouseListener in layout/forms/nsFileControlFrame.cpp (nsIDOMMouseListener)
nsIDOMEventListener : nsIsIndexFrame in layout/forms/nsIsIndexFrame.cpp (nsIDOMKeyListener)
nsIDOMEventListener : nsMathMLmactionFrame in layout/mathml/base/src/nsMathMLmactionFrame.cpp (nsIDOMMouseListener)
nsIDOMEventListener : nsMenuBarListener in layout/xul/base/src/nsMenuBarListener.cpp (nsIDOMFocusListener)
nsIDOMEventListener : nsPluginDOMContextMenuListener in layout/generic/nsObjectFrame.cpp (nsIDOMContextMenuListener)
nsIDOMEventListener : nsSliderMediator in layout/xul/base/src/nsSliderFrame.cpp (nsIDOMMouseListener)
nsIDOMEventListener : nsSplitterFrameInner in layout/xul/base/src/nsSplitterFrame.cpp (nsIDOMMouseListener)
nsIDOMEventListener : nsSyncLoader in content/base/src/nsSyncLoadService.cpp (nsIDOMLoadListener)
nsIDOMEventListener : nsXMLHttpRequest in content/base/src/nsXMLHttpRequest.cpp (nsIDOMLoadListener)
nsIDOMEventListener : nsXULPopupManager in layout/xul/base/src/nsXULPopupManager.cpp (nsIDOMKeyListener)
nsIDOMTreeWalker : inDeepTreeWalker in layout/inspector/src/inDeepTreeWalker.cpp (inIDeepTreeWalker)
nsIDOMUIEvent : nsDOMSVGZoomEvent in content/svg/content/src/nsDOMSVGZoomEvent.cpp (nsIDOMSVGZoomEvent)
nsIDocShellTreeNode : nsWebBrowser in embedding/browser/webBrowser/nsWebBrowser.cpp (nsIDocShellTreeItem)
nsIExceptionManager : nsExceptionService in xpcom/base/nsExceptionService.cpp (nsIExceptionService)
nsIFactory : nsPluginHostImpl in modules/plugin/base/src/nsPluginHostImpl.cpp (nsIPluginHost)
nsIFontCatalogEntry : nsFreeTypeFace in gfx/src/freetype/nsFreeType.cpp (nsITrueTypeFontCatalogEntry)
nsIInputStream : CPluginInputStream in plugin/oji/MRJ/plugin/Source/BackwardAdapter.cpp (nsIPluginInputStream)
nsILineIterator : nsTableRowGroupFrame in layout/tables/nsTableRowGroupFrame.cpp (nsILineIteratorNavigator)
nsIRequestObserver : EndListener in netwerk/streamconv/test/TestStreamConv.cpp (nsIStreamListener)
nsIRequestObserver : FaviconLoadListener in toolkit/components/places/src/nsFaviconService.cpp (nsIStreamListener)
nsIRequestObserver : InputTestConsumer in netwerk/test/TestUpload.cpp (nsIStreamListener)
nsIRequestObserver : QuotingOutputStreamListener in mailnews/compose/src/nsMsgCompose.cpp (nsIMsgQuotingOutputStreamListener)
nsIRequestObserver : RelatedLinksStreamListener in xpfe/components/related/src/nsRelatedLinksHandler.cpp (nsIStreamListener)
nsIRequestObserver : StreamToFile in parser/htmlparser/tests/grabpage/grabpage.cpp (nsIStreamListener)
nsIRequestObserver : TestConverter in netwerk/streamconv/test/Converters.cpp (nsIStreamConverter)
nsIRequestObserver : mozTXTToHTMLConv in netwerk/streamconv/converters/mozTXTToHTMLConv.cpp (mozITXTToHTMLConv)
nsIRequestObserver : nsDelAttachListener in mailnews/base/src/nsMessenger.cpp (nsIStreamListener)
nsIRequestObserver : nsFeedSniffer in browser/components/feeds/src/nsFeedSniffer.cpp (nsIStreamListener)
nsIRequestObserver : nsHTTPCompressConv in netwerk/streamconv/converters/nsHTTPCompressConv.cpp (nsIStreamConverter)
nsIRequestObserver : nsMsgMailboxParser in mailnews/local/src/nsParseMailbox.cpp (nsIStreamListener)
nsIRequestObserver : nsMsgSaveAsListener in mailnews/base/util/nsMsgMailNewsUrl.cpp (nsIStreamListener)
nsIRequestObserver : nsMsgSendLater in mailnews/compose/src/nsMsgSendLater.cpp (nsIMsgSendLater)
nsIRequestObserver : nsMsgTemplateReplyHelper in mailnews/compose/src/nsMsgComposeService.cpp (nsIStreamListener)
nsIRequestObserver : nsParseNewMailState in mailnews/local/src/nsParseMailbox.cpp (nsIStreamListener)
nsIRequestObserver : nsPluginByteRangeStreamListener in modules/plugin/base/src/nsPluginHostImpl.cpp (nsIStreamListener)
nsIRequestObserver : nsPluginCacheListener in modules/plugin/base/src/nsPluginHostImpl.cpp (nsIStreamListener)
nsIRequestObserver : nsSaveMsgListener in mailnews/base/src/nsMessenger.cpp (nsIStreamListener)
nsIRequestObserver : nsURLFetcher in mailnews/compose/src/nsURLFetcher.cpp (nsIStreamListener)
nsIRequestObserver : nsXFormsMessageElement in extensions/xforms/nsXFormsMessageElement.cpp (nsIStreamListener)
nsIRequestObserver : nsXPInstallManager in xpinstall/src/nsXPInstallManager.cpp (nsIStreamListener)
nsISelectionDisplay : nsTextInputSelectionImpl in layout/forms/nsTextControlFrame.cpp (nsISelectionController)
nsIStreamConverter : mozTXTToHTMLConv in netwerk/streamconv/converters/mozTXTToHTMLConv.cpp (mozITXTToHTMLConv)
nsIStreamListener : mozTXTToHTMLConv in netwerk/streamconv/converters/mozTXTToHTMLConv.cpp (mozITXTToHTMLConv)
nsIUrlClassifierDBService : nsUrlClassifierDBServiceWorker in toolkit/components/url_classifier/src/nsUrlClassifierDBService.cpp (nsIUrlClassifierDBServiceWorker)
nsIWebProgressListener : nsMsgComposeProgress in mailnews/compose/src/nsMsgComposeProgress.cpp (nsIMsgComposeProgress)
nsIXFormsControlBase : nsXFormsModelElement in extensions/xforms/nsXFormsModelElement.cpp (nsIXFormsContextControl)
nsIXPCTestParent : xpctestChild in js/src/xpconnect/tests/components/xpctest_child.cpp (nsIXPCTestChild)

Comment 1

[jag (Peter Annema)]

Attachment: List from comment 0

Comment 2

[timeless]

Attachment: jsdContext

Comment 3

[Damon Sicore (:damons)]

Comment on attachment 304701 [details] [diff] [review]
jsdContext

a1.9+=damons

Comment 4

[timeless]

Comment on attachment 304701 [details] [diff] [review]
jsdContext

mozilla/js/jsd/jsd_xpc.cpp 	1.86

Comment 5

[timeless]

Attachment: QIs

Comment 6

[jag (Peter Annema)]

timeless: awesome!

Can you make nsRobotSink use one of the NS_IMPL_ISUPPORTS macros? That and replacing the QI in NS_NewRobotSink with NS_IF_ADDREF should allow you to get rid of the NS_DEFINE_IIDs.

Comment 7

[timeless]

i don't think i can w/o converting http://mxr.mozilla.org/seamonkey/source/parser/htmlparser/robot/nsIRobotSink.h to idl (maybe I'm wrong). I didn't have the resources to perform proper testing for converting that to idl so i figured i'd deal w/ it later.

Comment 8

[timeless]

Attachment: untested rewrite of robotsink

Comment 9

[jag (Peter Annema)]

Comment on attachment 305977 [details] [diff] [review]
untested rewrite of robotsink

Ok, that was a bit more work than I expected. Let's move this to a new bug.

Comment 10

[jag (Peter Annema)]

See bug 419858 for that.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 305770 [details] [diff] [review]
QIs

>+#define NS_INTERFACE_TABLE_INHERITED10(Class, i1, i2, i3, i4, i5, i6, i7,     \
>+                                       i8, i9, i10)                           \
>+  NS_INTERFACE_TABLE_BEGIN                                                    \
>+    NS_INTERFACE_TABLE_ENTRY(Class, i1)                                       \
>+    NS_INTERFACE_TABLE_ENTRY(Class, i2)                                       \
>+    NS_INTERFACE_TABLE_ENTRY(Class, i3)                                       \
>+    NS_INTERFACE_TABLE_ENTRY(Class, i4)                                       \
>+    NS_INTERFACE_TABLE_ENTRY(Class, i5)                                       \
>+    NS_INTERFACE_TABLE_ENTRY(Class, i6)                                       \
>+    NS_INTERFACE_TABLE_ENTRY(Class, i7)                                       \
>+    NS_INTERFACE_TABLE_ENTRY(Class, i8)                                       \
>+    NS_INTERFACE_TABLE_ENTRY(Class, i9)                                       \
>+    NS_INTERFACE_TABLE_ENTRY(Class, i10)                                      \
>+  NS_INTERFACE_TABLE_END

Couldn't you change the caller to just use INTERFACE_MAP_ENTRYies

I hope there aren't any cases where leaving interface out from QI was intentional.

Comment 12

[Boris Zbarsky [:bzbarsky]]

> Couldn't you change the caller to just use INTERFACE_MAP_ENTRYies

You could, but the interface table might be smaller code...  That's why this code got converted to interface tables in the first place.

Comment 13

[jag (Peter Annema)]

Comment on attachment 305770 [details] [diff] [review]
QIs

>Index: mozilla/content/html/content/src/nsHTMLImageElement.cpp
>===================================================================
>@@ -192,12 +192,13 @@ NS_IMPL_RELEASE_INHERITED(nsHTMLImageEle
> 
> // QueryInterface implementation for nsHTMLImageElement
> NS_HTML_CONTENT_INTERFACE_TABLE_HEAD(nsHTMLImageElement, nsGenericHTMLElement)
>-  NS_INTERFACE_TABLE_INHERITED5(nsHTMLImageElement,
>+  NS_INTERFACE_TABLE_INHERITED6(nsHTMLImageElement,
>                                 nsIDOMHTMLImageElement,
>                                 nsIDOMNSHTMLImageElement,
>                                 nsIJSNativeInitializer,
>                                 imgIDecoderObserver,
>+                                nsIImageLoadingContent,
>+                                imgIContainerObserver)

Nit: put imgIContainerObserver right above imgIDecoderObserver. It looks like they order from base to derived.


>Index: mozilla/content/html/content/src/nsHTMLInputElement.cpp
>===================================================================
>@@ -410,16 +410,17 @@ NS_IMPL_RELEASE_INHERITED(nsHTMLInputEle
> // QueryInterface implementation for nsHTMLInputElement
> NS_HTML_CONTENT_CC_INTERFACE_TABLE_HEAD(nsHTMLInputElement,
>                                         nsGenericHTMLFormElement)
>+  NS_INTERFACE_TABLE_INHERITED10(nsHTMLInputElement,
>+                                 nsIDOMHTMLInputElement,
>+                                 nsIDOMNSHTMLInputElement,
>+                                 nsITextControlElement,
>+                                 nsIFileControlElement,
>+                                 nsIRadioControlElement,
>+                                 nsIPhonetic,
>+                                 imgIDecoderObserver,
>+                                 nsIImageLoadingContent,
>+                                 imgIContainerObserver,

Same here.


>Index: mozilla/content/html/content/src/nsHTMLObjectElement.cpp
>===================================================================
>@@ -168,16 +168,17 @@ NS_IMPL_RELEASE_INHERITED(nsHTMLObjectEl
> 
> NS_HTML_CONTENT_CC_INTERFACE_TABLE_HEAD(nsHTMLObjectElement,
>                                         nsGenericHTMLFormElement)
>+  NS_INTERFACE_TABLE_INHERITED10(nsHTMLObjectElement,
>+                                 nsIDOMHTMLObjectElement,
>+                                 imgIDecoderObserver,
>+                                 nsIRequestObserver,
>+                                 nsIStreamListener,
>+                                 nsIFrameLoaderOwner,
>+                                 nsIObjectLoadingContent,
>+                                 nsIImageLoadingContent,
>+                                 imgIContainerObserver,

And something similar here (maybe move imgIDecoderObserver down above nsIImageLoadingContent, and then imgIContainerObserver above both).


>Index: mozilla/content/html/content/src/nsHTMLSharedObjectElement.cpp
>===================================================================
>@@ -213,13 +213,14 @@ NS_IMPL_RELEASE_INHERITED(nsHTMLSharedOb
> NS_HTML_CONTENT_CC_INTERFACE_TABLE_AMBIGUOUS_HEAD(nsHTMLSharedObjectElement,
>                                                   nsGenericHTMLElement,
>                                                   nsIDOMHTMLAppletElement)
>+  NS_INTERFACE_TABLE_INHERITED9(nsHTMLSharedObjectElement,
>                                 imgIDecoderObserver,
>                                 nsIRequestObserver,
>                                 nsIStreamListener,
>                                 nsIFrameLoaderOwner,
>                                 nsIObjectLoadingContent,
>                                 nsIImageLoadingContent,
>+                                imgIContainerObserver,

Same here.


>Index: mozilla/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp
>===================================================================
>@@ -1012,7 +1012,9 @@ mozTXTToHTMLConv::~mozTXTToHTMLConv() 
> {
> }
> 
>-NS_IMPL_ISUPPORTS1(mozTXTToHTMLConv, mozITXTToHTMLConv)
>+NS_IMPL_ISUPPORTS2(mozTXTToHTMLConv,
>+                   mozITXTToHTMLConv,
>+                   nsIStreamConverter)

This is missing nsIStreamListener and nsIRequestObserver.


>Index: mozilla/netwerk/streamconv/converters/nsHTTPCompressConv.cpp
>===================================================================
>@@ -50,7 +50,10 @@
> #include "nsComponentManagerUtils.h"
> 
> // nsISupports implementation
>-NS_IMPL_ISUPPORTS2(nsHTTPCompressConv, nsIStreamConverter, nsIStreamListener)
>+NS_IMPL_ISUPPORTS3(nsHTTPCompressConv,
>+                   nsIStreamConverter,
>+                   nsIRequestObserver,
>+                   nsIStreamListener)

Swap the last two to follow order of inheritance. Or do base to derived like they do above.


>Index: mozilla/netwerk/streamconv/test/Converters.cpp
>===================================================================
>@@ -9,7 +9,10 @@
> // TestConverter
> //////////////////////////////////////////////////
> 
>-NS_IMPL_ISUPPORTS2(TestConverter, nsIStreamConverter, nsIStreamListener)
>+NS_IMPL_ISUPPORTS3(TestConverter,
>+                   nsIStreamConverter,
>+                   nsIRequestObserver,
>+                   nsIStreamListener)

Ditto.


>Index: mozilla/parser/htmlparser/robot/nsRobotSink.cpp

Dead file per bug 3004.

>Index: mozilla/toolkit/components/places/src/nsFaviconService.cpp
>===================================================================
>@@ -858,9 +858,10 @@ nsFaviconService::OptimizeFaviconImage(c
> }
> 
> 
>-NS_IMPL_ISUPPORTS4(FaviconLoadListener,
>+NS_IMPL_ISUPPORTS5(FaviconLoadListener,
>                    nsIRequestObserver,
>                    nsIStreamListener,
>+                   nsIRequestObserver,

Looks like this was added already for bug 392980 (see two lines up).



Looks like you missed these two in nsPluginHostImpl.cpp:
nsIRequestObserver : nsPluginByteRangeStreamListener in modules/plugin/base/src/nsPluginHostImpl.cpp (nsIStreamListener)
nsIRequestObserver : nsPluginCacheListener in modules/plugin/base/src/nsPluginHostImpl.cpp (nsIStreamListener)

And this one:
nsIUrlClassifierDBService : nsUrlClassifierDBServiceWorker in toolkit/components/url_classifier/src/nsUrlClassifierDBService.cpp (nsIUrlClassifierDBServiceWorker)


Could you attach a new patch with these nits picked and issues addressed?

Thanks for turning my list into a patch, timeless!

Comment 14

[timeless]

Attachment: wrong patch

Comment 15

[jag (Peter Annema)]

Comment on attachment 309794 [details] [diff] [review]
wrong patch

>Index: mozilla/content/html/content/src/nsHTMLObjectElement.cpp
>===================================================================
>+  NS_INTERFACE_TABLE_INHERITED10(nsHTMLObjectElement,
>+                                 nsIDOMHTMLObjectElement,
>+                                 nsIRequestObserver,
>+                                 nsIStreamListener,
>+                                 nsIFrameLoaderOwner,
>+                                 imgIContainerObserver,
>+                                 imgIDecoderObserver,
>+                                 nsIObjectLoadingContent,
>+                                 nsIImageLoadingContent,
>+                                 nsIInterfaceRequestor,
>+                                 nsIChannelEventSink)

Move nsIObjectLoadingContent above imgIContainerObserver please.


>Index: mozilla/content/html/content/src/nsHTMLSharedObjectElement.cpp
>===================================================================
>+  NS_INTERFACE_TABLE_INHERITED9(nsHTMLSharedObjectElement,
>                                 nsIRequestObserver,
>                                 nsIStreamListener,
>                                 nsIFrameLoaderOwner,
>+                                imgIContainerObserver,
>                                 nsIObjectLoadingContent,
>+                                imgIDecoderObserver,
>                                 nsIImageLoadingContent,
>                                 nsIInterfaceRequestor,
>                                 nsIChannelEventSink)

Ditto please. I guess my previous review wasn't as clear as I thought it was.


>Index: mozilla/toolkit/components/places/src/nsFaviconService.cpp
>===================================================================
>@@ -858,9 +858,10 @@ nsFaviconService::OptimizeFaviconImage(c
> }
> 
> 
>-NS_IMPL_ISUPPORTS4(FaviconLoadListener,
>+NS_IMPL_ISUPPORTS5(FaviconLoadListener,
>                    nsIRequestObserver,
>                    nsIStreamListener,
>+                   nsIRequestObserver,
>                    nsIInterfaceRequestor,
>                    nsIChannelEventSink)
> 

You still have a duplicated nsIRequestObserver entry. Feel free to remove theirs so this will have the same order as the rest.

r+sr=jag with those nits picked.

Comment 16

[Damon Sicore (:damons)]

Comment on attachment 309794 [details] [diff] [review]
wrong patch

a1.9+=damons

Comment 17

[Serge Gautherie (:sgautherie)]

A patch was checked in:
{{
2008-04-06 05:28	timeless%mozdev.org 	...  	Bug 394114 Interfaces missing from various QI implementations.
r=jag sr=jag a=dsicore
}}

Comment 18

[timeless]

this isn't absolutely identical to the attachment, but it is what i was trying to commit.

mozilla/layout/mathml/base/src/nsMathMLmactionFrame.cpp 	1.94
mozilla/layout/tables/nsTableRowGroupFrame.cpp 	3.408
mozilla/layout/generic/nsObjectFrame.cpp 	1.648
mozilla/content/base/src/nsXMLHttpRequest.cpp 	1.236
mozilla/toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp 	1.72
mozilla/browser/components/feeds/src/nsFeedSniffer.cpp 	1.21
mozilla/content/base/src/nsDOMParser.cpp 	1.63
mozilla/content/base/src/nsGenConImageContent.cpp 	1.11
mozilla/content/base/src/nsSyncLoadService.cpp 	1.68
mozilla/content/html/content/src/nsHTMLImageElement.cpp 	1.226
mozilla/content/html/content/src/nsHTMLInputElement.cpp 	1.477
mozilla/content/html/content/src/nsHTMLObjectElement.cpp 	1.109
mozilla/content/html/content/src/nsHTMLSharedObjectElement.cpp 	1.93
mozilla/content/svg/content/src/nsDOMSVGZoomEvent.cpp 	1.7
mozilla/content/xml/document/src/nsLoadSaveContentSink.cpp 	1.16
mozilla/embedding/browser/webBrowser/nsWebBrowser.cpp 	1.169
mozilla/extensions/xforms/nsXFormsMessageElement.cpp 	1.37
mozilla/extensions/xforms/nsXFormsModelElement.cpp 	1.162
mozilla/layout/inspector/src/inDeepTreeWalker.cpp 	1.21
mozilla/mailnews/base/util/nsMsgMailNewsUrl.cpp 	1.121
mozilla/mailnews/compose/src/nsMsgCompose.cpp 	1.560
mozilla/mailnews/compose/src/nsMsgComposeProgress.cpp 	1.15
mozilla/mailnews/compose/src/nsMsgComposeService.cpp 	1.143
mozilla/mailnews/compose/src/nsMsgSendLater.cpp 	1.121
mozilla/mailnews/compose/src/nsURLFetcher.cpp 	1.81
mozilla/mailnews/base/src/nsMessenger.cpp 	1.385
mozilla/layout/forms/nsComboboxControlFrame.cpp 	1.428
mozilla/layout/forms/nsFileControlFrame.cpp 	3.226
mozilla/layout/forms/nsIsIndexFrame.cpp 	1.100
mozilla/layout/forms/nsTextControlFrame.cpp 	3.289
mozilla/layout/xul/base/src/nsMenuBarListener.cpp 	1.90
mozilla/layout/xul/base/src/nsSliderFrame.cpp 	1.179
mozilla/layout/xul/base/src/nsSplitterFrame.cpp 	1.182
mozilla/layout/xul/base/src/nsXULPopupManager.cpp 	1.59
mozilla/mailnews/local/src/nsParseMailbox.cpp 	1.299
mozilla/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp 	1.90
mozilla/netwerk/streamconv/converters/nsHTTPCompressConv.cpp 	1.31
mozilla/netwerk/streamconv/test/Converters.cpp 	1.27
mozilla/netwerk/streamconv/test/TestStreamConv.cpp 	1.58
mozilla/netwerk/test/TestUpload.cpp 	1.23
mozilla/parser/htmlparser/tests/grabpage/grabpage.cpp 	1.35
mozilla/toolkit/components/places/src/nsFaviconService.cpp 	1.23
mozilla/plugin/oji/MRJ/plugin/Source/BackwardAdapter.cpp 	1.48
mozilla/xpcom/base/nsExceptionService.cpp 	1.22
mozilla/xpfe/components/related/src/nsRelatedLinksHandler.cpp 	1.84
mozilla/xpcom/glue/nsISupportsImpl.h 	3.56
mozilla/xpinstall/src/nsXPInstallManager.cpp 	1.163