8581 - missing GetIID() methods on interfaces

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[buster]

missing GetIID() methods causes unpredictable bugs when using interfaces and
nsCOMPtr.

From Bruce Mitchener Jr.'s original note on the layout newsgroup:

Hello.

I've been going through the tree and making sure that interface classes
have |GetIID()| implementations as nsCOMPtr requires.

A number of files in mozilla/layout are missing their |GetIID()|
implementations.  Which of these shouldn't have them for some reason or
other?  The only valid case that I can see is nsIFrame which isn't
reference counted and so you wouldn't want an nsCOMPtr<nsIFrame>.

layout/base/public:
nsIDocumentContainer
nsIDocumentObserver
nsIDocumentViewer (has nsCOMPtr usage)
nsIFrame
nsIFrameImageLoader
nsIFrameReflow
nsIFrameUtil
nsILayoutDebugger
nsINameSpace (has nsCOMPtr usage)
nsINameSpaceManager (has nsCOMPtr usage)
nsIPageSequenceFrame
nsIReflowCommand (has nsCOMPtr usage)
nsISpaceManager
nsIStyleFrameConstruction
nsIStyleSheetLinkingElement

layout/events/public:
nsIEventStateManager (has nsCOMPtr usage)

layout/html/content/public:
nsIFocusableContent
nsIForm
nsISelectElement

layout/html/forms/public:
nsIComboboxControlFrame
nsIFormControlFrame
nsIFormManager
nsIListControlFrame

layout/html/style/public:
nsICSSParser (has nsCOMPtr usage)

layout/xml/document/public:
nsIXMLContentSink
nsIXMLDocument (has nsCOMPtr usage)


I can add the |GetIID()| implementations to all of these if they should
all get them.  Please advise.

Thanks!

 - Bruce

Comment 1

[buster]

Bruce did tremendous work to track this down.  I hope the various owners of
these interfaces can fix the problem right away where appropriate, and document
any interfaces where it isn't appropriate and why.  Rick, I suggest M8 for a
target since the bugs that derive from this can be quite hard to track down.

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I found another missing GetIID() when I did a fix for bug 6544.

http://lxr.mozilla.org/mozilla/source/layout/html/document/src/nsIHTMLDocument.h

Comment 3

[buster]

This bug has gone nowhere in the past several weeks.  I think scc went through
and made a pass at verifying all interfaces had GetIID.  Maybe you could check
with him and see if this bug is already fixed?  adding scott to cc list.

Comment 4

[Scott Collins]

It is now a compile time error to omit |GetIID()| from classes that need it.  So,

... I would guess this bug is (mostly) fixed.  Every case like |nsIFoo| isa |

nsISupports| (and so must implement |nsIFoo::GetIID|) will be caught.  Cases

where |CallQueryInterface()| or |nsCOMPtr| are not used may not be caught.  Rarer

cases like |nsIBar| isa |nsIFoo| isa |nsISupports| will allow |nsIBar| to compile

(wrongly) without its own |GetIID()| if it picks up |nsIFoo::GetIID()|.  This is

harder to catch, and it's not really good COM style or normal COM procedure to

use inheritance this way ... caveat emptor.  Anyway, Mac, Windows, and egcs won't

compile if you forget (assuming an |nsCOMPtr| or call to |CallQueryInterface|,

etc. is involved), on some of the older Unixes it may continue to build.  So use

a primary platform for confirmation.

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

nsIContentSink is missing GetIID().

Comment 6

[rickg]

This bug has been marked "future" because the original netscape engineer 
working on this is over-burdened. If you feel this is an error, that you or 
another known resource will be working on this bug,or if it blocks your work 
in some way -- please attach your concern to the bug for reconsideration. 

Comment 7

[Christopher Hoess (gone)]

into layout mainstream, although this is probably completely invalid at this point.

Comment 8

[Boris Zbarsky [:bzbarsky]]

The following interfaces are still missing GetIID, of the ones listed above:

nsIDocumentContainer

Good thing it's not used anywhere.  Bug 193543 filed on that.

This bug is fixed.