Currently NS_GET_IID can be used on classes which don't actually declare an IID: class nsIFoo : public nsISupports { virtual nsresult DoSomething() = 0; }; nsCOMPtr<nsIFoo> foo = do_QueryInterface(bar); In this case we inherit the IID of nsISupports, which can be really confusing. I have a patch in the works which fixes this; it has the following side-effects: * you can't use nsCOMPtr<ConcreteClass> any more (unless the concrete class has a pseudo-IID). * I had to remove the static GetIID method from interfaces... this normally doesn't matter and is a good thing anyway. Producing this patch found the following bugs: * nsXPCOMCycleCollectionParticipant has extra virtual methods which are used by the cycle collector, but it doesn't have an IID. This is really sucky! * nsComponentManager holds onto the categorymanager with a concrete pointer but doesn't actually check that it has the right type. Dumb. The patch produces the following compile-time error: ../../dist/include/nsCOMPtr.h: In constructor ‘nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsReceiver]’: ../../dist/include/nsCOMPtr.h:520: instantiated from ‘void nsCOMPtr<T>::Assert_NoQueryNeeded() [with T = nsReceiver]’ ../../dist/include/nsCOMPtr.h:556: instantiated from ‘nsCOMPtr<T>::nsCOMPtr(T*) [with T = nsReceiver]’ ../../../src/xpcom/tests/TestPipes.cpp:187: instantiated from here ../../dist/include/nsCOMPtr.h:572: error: ‘kIID’ is not a member of ‘nsIRunnable::COMTypeInfo<nsReceiver, 0>’

Fixing this would make deCOMtamination easier. If you have a (non-scriptable interface) class that you suspect is only QIed from, not to, you could delete its IID and the compiler would confirm that.

I think it's worth separating the cycle collector changes here into their own bug. I think most of those callers can safely cast nsCycleCollectionParticipant to nsXPCOMCycleCollectionParticipant; however I don't think isScanSafe can, but that's debug-only and probably nobody ever uses that debugging method. I think we may want to maintain the distinctions you remove, and I think what's in the patch you have will probably cause compilation failures in XPConnect, which is where the other implementations of nsCycleCollectionParticipant that are not nsXPCOMCycleCollectionParticipant live.

Why is that cast safe? xpconnect at least seems to implement nsScriptObjectTracer without implementing nsXPCOMCycleCollectionParticipant. Obviously the patch here is incompletely, but in xpconnect and elsewhere I just made everything inherit from nsXPCOMCycleCollectionParticipant...

Can we please discuss this in another bug, specifically about the cycle collector?

Comment on attachment 400036 [details] [diff] [review] make NS_GET_IID on a class without a declared IID fail, rev. 2 >+++ b/accessible/src/base/nsAccessibleEventData.h >+inline already_AddRefed<nsAccEvent> >+nsAccEvent::GetAccEventPtr(nsIAccessibleEvent *aAccEvent) Can this not use CallQueryInterface? >+++ b/accessible/src/xul/nsXULTreeGridAccessible.cpp >-NS_INTERFACE_MAP_STATIC_AMBIGUOUS(nsXULTreeGridRowAccessible) Why is this change OK? Because it doesn't declare an IID and no on QIs to it? >+++ b/docshell/base/nsDocShell.cpp >@@ -941,17 +941,17 @@ NS_IMETHODIMP nsDocShell::GetInterface(c >- nsCOMPtr<nsIPresShell> shell; >+ nsRefPtr<nsIPresShell> shell; Should probably stay nsCOMPtr. > nsDocShell::GetCharset(char** aCharset) >- nsCOMPtr<nsIPresShell> presShell; >+ nsRefPtr<nsIPresShell> presShell; And here. With those nits, looks great.

I don't know whether it's worth splitting this out, just to get things moving?

Comment on attachment 634636 [details] [diff] [review] Just the GetIID removal >- static const nsIID& GetIID() {return COMTypeInfo<int>::kIID;} The other approach is to make it non-static and template on this. const nsIID& GetIID() {return NS_GetIID(this);} tempalate<class T> const nsIID& NS_GetIID() {return NS_GET_TEMPLATE_IID(T);}

Well, opt windows; there are lots of people using nsCOMPtr<ConcreteClass> which only compiles opt with the patch.

Some of the tests fail though, that's probably because I didn't fix up the bogus code well enough.

This only affects debug builds, because nsCOMPtr has extra checks in that case.

(If preferred the nsID.h change could be moved to a later patch.)

Comment on attachment 8388888 [details] [diff] [review] Mechanical nsCOMPtr to nsRefPtr conversions https://hg.mozilla.org/integration/mozilla-inbound/rev/a01f97c1ed02

Comment on attachment 8388891 [details] [diff] [review] Remove GetIID https://hg.mozilla.org/integration/mozilla-inbound/rev/2a015b45d808

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/cfe51017482e for OSX build bustage: https://tbpl.mozilla.org/php/getParsedLog.php?id=36293318&tree=Mozilla-Inbound

Comment on attachment 8388888 [details] [diff] [review] Mechanical nsCOMPtr to nsRefPtr conversions https://hg.mozilla.org/integration/mozilla-inbound/rev/1d805dccc357

Comment on attachment 8388891 [details] [diff] [review] Remove GetIID https://hg.mozilla.org/integration/mozilla-inbound/rev/57165fdfa87f

https://hg.mozilla.org/mozilla-central/rev/1d805dccc357 https://hg.mozilla.org/mozilla-central/rev/57165fdfa87f

It will becomes a compile error to use an IID before defining it (rather than just declaring it) so I'm moving the affected declarations to after the definitions. In some cases (particularly nsIPresShell), the IID is actually defined using the wrong class name, and nsINativeMenuService forgot to even declare its IID. Meanwhile nsIParanoidFragmentContentSink should just die.

All the places claiming to implement concrete types, which currently ends up implementing the interface the type inherits from, sometimes redundantly.

I accidentally omitted one misdefined IID from attachment 8393528 [details] [diff] [review].

Comment on attachment 8393532 [details] [diff] [review] Fix bogus interface maps https://hg.mozilla.org/integration/mozilla-inbound/rev/d91f49580ec8

Comment on attachment 8395472 [details] [diff] [review] Fix misdefined IIDs https://hg.mozilla.org/integration/mozilla-inbound/rev/4509cc145dfc

https://hg.mozilla.org/mozilla-central/rev/d91f49580ec8 https://hg.mozilla.org/mozilla-central/rev/4509cc145dfc

Most of these are uses of nsCOMPtr<Class> for objects where only a particular interface is needed, so rather than using nsRefPtr<Class> I chose nsCOMPtr<nsIInterface> instead (in particular this enabled some code simplification in DeviceStorageRequestParent.cpp). The exception is an nsRefPtr<AudioChannelAgent> mAgent that is initialised using do_CreateInstance which only works with interfaces, but fortunately an nsCOMPtr<nsIAudioChannelAgent> suffices for that use case.

Since the above patches landed a number of new abuses have landed, and one of the existing abuses has unfortunately regressed further due to bug 907352. So I thought that to avoid further regressions it might be better to land tweaks that suffice to get the specific abuses already reported to compile, but would make it difficult to add new abuses. (Had I done this before bug 907352 for instance, then perhaps that code would have been fixed by now.)

Comment on attachment 8419249 [details] [diff] [review] Workarounds for dependent bugs gogo!

Neil: I think you must have meant a different bug than bug 907352.

(In reply to Zack Weinberg from comment #31) > Neil: I think you must have meant a different bug than bug 907352. I did; to be precise, attachment #8411059 [details] [diff] [review] changed the types of the targets of two variables on which do_QueryObject had been abused. Before the change, it might have been possible to change the source type to match the variable thus avoiding the need for do_QueryObject.

Try history: https://tbpl.mozilla.org/?tree=Try&rev=b142e50ff11b was greenish on opt builds (except Android) https://tbpl.mozilla.org/?tree=Try&rev=26a50ad2916f was greenish except on Android https://tbpl.mozilla.org/?tree=Try&rev=20da5eaa9c48 was green (only tested on Android) This became the patch that was attached. Checked in with a comment fix: https://hg.mozilla.org/integration/mozilla-inbound/rev/c738f7348dea

https://tbpl.mozilla.org/?tree=Try&rev=8874abfbcbc0 https://hg.mozilla.org/integration/mozilla-inbound/rev/77b1f329c174