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

RESOLVED FIXED in mozilla18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Ehsan, Assigned: Luqman)

Tracking

Trunk
mozilla18
x86
macOS
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

Reporter

Description

7 years ago
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
Reporter

Comment 6

7 years ago
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.
Assignee

Comment 7

7 years ago
Got it, this is C++ not python
Attachment #655779 - Attachment is obsolete: true
Assignee

Comment 9

7 years ago
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 ?
Assignee

Updated

7 years ago
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.
Assignee

Comment 11

7 years ago
As per Boris' comments, assert in only the NS_IMPL_QUERY_TAIL_GUTS case.
Attachment #655843 - Attachment is obsolete: true
Reporter

Comment 13

7 years ago
(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.  :-)
Reporter

Comment 14

7 years ago
(Sorry for not thinking of that from the beginning!)
Looks green to me. Who's a good reviewer for this?
Reporter

Comment 16

7 years ago
(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!  ;-)
Reporter

Comment 17

7 years ago
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+
Reporter

Comment 18

7 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/94038167af79
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/94038167af79
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Interestingly, the fact that this passes tests means we're not QIing concrete nsAccessNode, say, to nsISupports...
Reporter

Comment 21

7 years ago
(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.