Closed
Bug 514280
Opened 15 years ago
Closed 10 years ago
NS_GET_IID can be used on classes which don't actually declare an IID
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: benjamin, Assigned: neil)
References
(Depends on 1 open bug)
Details
Attachments
(8 files, 4 obsolete files)
(deleted),
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
benjamin
:
review+
neil
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
neil
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
neil
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
neil
:
checkin+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
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>’
Comment 1•15 years ago
|
||
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.
Reporter | ||
Comment 2•15 years ago
|
||
Comment 3•15 years ago
|
||
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.
Reporter | ||
Comment 4•15 years ago
|
||
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...
Comment 5•15 years ago
|
||
Can we please discuss this in another bug, specifically about the cycle collector?
Reporter | ||
Comment 6•15 years ago
|
||
Attachment #398234 -
Attachment is obsolete: true
Attachment #400036 -
Flags: superreview?(bzbarsky)
Attachment #400036 -
Flags: review?(bzbarsky)
Updated•15 years ago
|
Attachment #400036 -
Attachment is patch: true
Attachment #400036 -
Attachment mime type: application/octet-stream → text/plain
Updated•15 years ago
|
Attachment #400036 -
Flags: superreview?(bzbarsky)
Attachment #400036 -
Flags: superreview+
Attachment #400036 -
Flags: review?(bzbarsky)
Attachment #400036 -
Flags: review+
Comment 7•15 years ago
|
||
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.
Assignee | ||
Comment 8•12 years ago
|
||
I don't know whether it's worth splitting this out, just to get things moving?
Assignee | ||
Comment 9•12 years ago
|
||
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);}
Assignee | ||
Comment 10•11 years ago
|
||
Assignee | ||
Comment 11•11 years ago
|
||
Well, opt windows; there are lots of people using nsCOMPtr<ConcreteClass> which only compiles opt with the patch.
Assignee | ||
Comment 12•11 years ago
|
||
Some of the tests fail though, that's probably because I didn't fix up the bogus code well enough.
Assignee | ||
Comment 13•11 years ago
|
||
This only affects debug builds, because nsCOMPtr has extra checks in that case.
Attachment #8388888 -
Flags: review?(benjamin)
Assignee | ||
Comment 14•11 years ago
|
||
(If preferred the nsID.h change could be moved to a later patch.)
Attachment #634636 -
Attachment is obsolete: true
Attachment #8388891 -
Flags: review?(benjamin)
Assignee | ||
Updated•11 years ago
|
Attachment #8388112 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Attachment #8388888 -
Flags: review?(benjamin) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #8388891 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 8388888 [details] [diff] [review]
Mechanical nsCOMPtr to nsRefPtr conversions
https://hg.mozilla.org/integration/mozilla-inbound/rev/a01f97c1ed02
Attachment #8388888 -
Flags: checkin+
Assignee | ||
Comment 16•11 years ago
|
||
Comment on attachment 8388891 [details] [diff] [review]
Remove GetIID
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a015b45d808
Attachment #8388891 -
Flags: checkin+
Assignee | ||
Updated•11 years ago
|
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
Assignee | ||
Comment 18•11 years ago
|
||
Comment on attachment 8388888 [details] [diff] [review]
Mechanical nsCOMPtr to nsRefPtr conversions
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d805dccc357
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 8388891 [details] [diff] [review]
Remove GetIID
https://hg.mozilla.org/integration/mozilla-inbound/rev/57165fdfa87f
Comment 20•11 years ago
|
||
Assignee | ||
Comment 21•11 years ago
|
||
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)
Assignee | ||
Comment 22•11 years ago
|
||
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)
Assignee | ||
Comment 23•11 years ago
|
||
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)
Reporter | ||
Updated•11 years ago
|
Attachment #8393532 -
Flags: review?(benjamin) → review+
Reporter | ||
Updated•11 years ago
|
Attachment #8395472 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 24•11 years ago
|
||
Comment on attachment 8393532 [details] [diff] [review]
Fix bogus interface maps
https://hg.mozilla.org/integration/mozilla-inbound/rev/d91f49580ec8
Attachment #8393532 -
Flags: checkin+
Assignee | ||
Comment 25•11 years ago
|
||
Comment on attachment 8395472 [details] [diff] [review]
Fix misdefined IIDs
https://hg.mozilla.org/integration/mozilla-inbound/rev/4509cc145dfc
Attachment #8395472 -
Flags: checkin+
Assignee | ||
Comment 27•11 years ago
|
||
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)
Reporter | ||
Updated•11 years ago
|
Attachment #8400491 -
Flags: review?(benjamin) → review+
Comment 28•11 years ago
|
||
Assignee | ||
Comment 29•10 years ago
|
||
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)
Reporter | ||
Comment 30•10 years ago
|
||
Comment on attachment 8419249 [details] [diff] [review]
Workarounds for dependent bugs
gogo!
Attachment #8419249 -
Flags: review?(benjamin) → review+
Comment 31•10 years ago
|
||
Neil: I think you must have meant a different bug than bug 907352.
Assignee | ||
Comment 32•10 years ago
|
||
(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.
Assignee | ||
Comment 33•10 years ago
|
||
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
Assignee | ||
Comment 34•10 years ago
|
||
Comment 35•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•