Closed Bug 786008 Opened 12 years ago Closed 12 years ago

Add a debug check to make sure that QueryInterface implementations always handle nsISupports

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18

People

(Reporter: ehsan.akhgari, Assigned: Luqman)

Details

(Whiteboard: [mentor=ehsan][lang=c++])

Attachments

(1 file, 4 obsolete files)

Add a debug check to make sure that QueryInterface implementations always handle nsISupports
2.46 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review 
All QI implementations need to be able to handle nsISupports.  One way to check that is to modify NS_IMPL_QUERY_TAIL_GUTS, NS_IMPL_QUERY_TAIL_INHERITING and NS_IMPL_QUERY_TAIL_USING_AGGREGATOR such that they MOZ_ASSERT to make sure that the iid is not equal to nsISupport's iid, in other words, nsISupports must be handled by something else before the NS_NOINTERFACE case in those macros are reached.

Once we have that patch, one needs to submit it to the try server to fix the existing places in the code base which make this mistake.
CCing a few more people who might be interested in tackling this.
Comment on attachment 655775 [details] [diff] [review]
Add a debug check to make sure that QueryInterface implementations always handle nsISupports

Thanks Luqman! I just realized that I was wrong in my description; we don't return early from the QI tables as I thought. The asserts should be moved inside the !foundInterface cases, then you can fire off a try build and see what happens.
Assignee: nobody → laden
Comment on attachment 655779 [details] [diff] [review]
Add a debug check to make sure that QueryInterface implementations always handle nsISupports

Review of attachment 655779 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot, Luqman for taking this!

::: xpcom/glue/nsISupportsImpl.h
@@ +665,5 @@
>  #define NS_IMPL_QUERY_TAIL_GUTS                                               \
>      foundInterface = 0;                                                       \
>    nsresult status;                                                            \
>    if ( !foundInterface )                                                      \
> +    MOZ_ASSERT(!aIID.Equals(NS_GET_IID(nsISupports)));                        \

You need to add braces around the if block, as the try server will tell you.  :-)

Also, please add a comment there saying why this is needed.
Got it, this is C++ not python
Attachment #655779 - Attachment is obsolete: true
Ok, so confirmed that this compiled locally (linux 64), pushed it to try https://tbpl.mozilla.org/?tree=Try&rev=e2c902bbbc9d

When I tried running the build it crashed:

    Assertion failure: !aIID.Equals((::nsISupports::COMTypeInfo<int>::kIID)), at /scratch/laden/mozilla-central/netwerk/base/src/nsFileStreams.cpp:334
    Program received signal SIGSEGV, Segmentation fault.
    nsFileInputStream::QueryInterface (this=0x7fffdeb5d2e0, aIID=..., aInstancePtr=<optimized out>) at /scratch/laden/mozilla-central/netwerk/base/src/nsFileStreams.cpp:334
    334	    NS_INTERFACE_MAP_END_INHERITING(nsFileStreamBase)

which makes sense since they don't invoked the NS_INTERFACE_MAP_ENTRY_AMBIGUOUS magic. The fix I think would be NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, foo) except that I don't know what the foo should be. The first interface specified to NS_INTERFACE_MAP_ENTRY ?
Attachment #655810 - Attachment is obsolete: true
nsFileStreamBase QIs to nsISupports.

In the NS_IMPL_QUERY_TAIL_INHERITING case, you shouldn't be asserting anything, since it's perfectly fine to have the baseclass do the nsISupports stuff.

For that matter, I'm not sure it makes sense to assert anything in the aggregation case either, for the same reason.
As per Boris' comments, assert in only the NS_IMPL_QUERY_TAIL_GUTS case.
Attachment #655843 - Attachment is obsolete: true
(In reply to Boris Zbarsky (:bz) from comment #10)
> nsFileStreamBase QIs to nsISupports.
> 
> In the NS_IMPL_QUERY_TAIL_INHERITING case, you shouldn't be asserting
> anything, since it's perfectly fine to have the baseclass do the nsISupports
> stuff.
> 
> For that matter, I'm not sure it makes sense to assert anything in the
> aggregation case either, for the same reason.

Hmm, yeah on the second thought I think you're right.  If the base class (or aggregated) version of QI doesn't handle nsISupports, the assertion will trigger before it returns.  :-)
(Sorry for not thinking of that from the beginning!)
Looks green to me. Who's a good reviewer for this?
(In reply to Josh Matthews [:jdm] from comment #15)
> Looks green to me. Who's a good reviewer for this?

I'm gonna say me!  ;-)
Comment on attachment 656053 [details] [diff] [review]
Add a debug check to make sure that QueryInterface implementations always handle nsISupports

This looks good.  The indentation needs to be adjusted though, but I'll do that myself before landing.  Thanks, Luqman!
Attachment #656053 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/94038167af79
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Interestingly, the fact that this passes tests means we're not QIing concrete nsAccessNode, say, to nsISupports...
(In reply to comment #20)
> Interestingly, the fact that this passes tests means we're not QIing concrete
> nsAccessNode, say, to nsISupports...

Hmm, that's a good point.  I don't know how we would make this check more comprehensive though, without making QIs a lot slower in debug builds.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: