Closed Bug 514280 Opened 10 years ago Closed 6 years ago

NS_GET_IID can be used on classes which don't actually declare an IID

Categories

(Core :: XPCOM, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: benjamin, Assigned: neil)

References

(Depends on 2 open bugs)

Details

Attachments

(8 files, 4 obsolete files)

115.41 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
124.35 KB, patch
Details | Diff | Splinter Review
54.22 KB, patch
benjamin
: review+
neil
: checkin+
Details | Diff | Splinter Review
4.97 KB, patch
benjamin
: review+
neil
: checkin+
Details | Diff | Splinter Review
8.59 KB, patch
benjamin
: review+
neil
: checkin+
Details | Diff | Splinter Review
17.47 KB, patch
benjamin
: review+
neil
: checkin+
Details | Diff | Splinter Review
19.98 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
12.87 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
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?
Depends on: 515714
Depends on: 515949
Attachment #398234 - Attachment is obsolete: true
Attachment #400036 - Flags: superreview?(bzbarsky)
Attachment #400036 - Flags: review?(bzbarsky)
Attachment #400036 - Attachment is patch: true
Attachment #400036 - Attachment mime type: application/octet-stream → text/plain
Attachment #400036 - Flags: superreview?(bzbarsky)
Attachment #400036 - Flags: superreview+
Attachment #400036 - Flags: review?(bzbarsky)
Attachment #400036 - Flags: review+
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.
Attached patch Just the GetIID removal (obsolete) — Splinter Review
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);}
Attached patch Version that compiles on Windows (obsolete) — Splinter Review
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.
Attachment #8388888 - Flags: review?(benjamin)
Attached patch Remove GetIIDSplinter Review
(If preferred the nsID.h change could be moved to a later patch.)
Attachment #634636 - Attachment is obsolete: true
Attachment #8388891 - Flags: review?(benjamin)
Attachment #8388112 - Attachment is obsolete: true
Attachment #8388888 - Flags: review?(benjamin) → review+
Attachment #8388891 - Flags: review?(benjamin) → review+
Assignee: benjamin → neil
Status: NEW → ASSIGNED
Keywords: leave-open
Attached patch Fix misdefined IIDs (obsolete) — Splinter Review
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.
Attachment #8393528 - Flags: review?(benjamin)
All the places claiming to implement concrete types, which currently ends up implementing the interface the type inherits from, sometimes redundantly.
Attachment #8393532 - Flags: review?(benjamin)
Depends on: 986973
Depends on: 986975
Depends on: 986974
Depends on: 986976
Depends on: 986972
I accidentally omitted one misdefined IID from attachment 8393528 [details] [diff] [review].
Attachment #8393528 - Attachment is obsolete: true
Attachment #8393528 - Flags: review?(benjamin)
Attachment #8395472 - Flags: review?(benjamin)
Attachment #8393532 - Flags: review?(benjamin) → review+
Attachment #8395472 - Flags: review?(benjamin) → review+
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.
Attachment #8400491 - Flags: review?(benjamin)
Attachment #8400491 - Flags: review?(benjamin) → review+
Depends on: 998092
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.)
Attachment #8419249 - Flags: review?(benjamin)
Comment on attachment 8419249 [details] [diff] [review]
Workarounds for dependent bugs

gogo!
Attachment #8419249 - Flags: review?(benjamin) → review+
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
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/77b1f329c174
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.