Closed Bug 986976 Opened 6 years ago Closed 6 years ago

do_QueryInterface abuse in various .mm files

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: neil, Assigned: smichaud)

References

Details

Attachments

(1 file, 1 obsolete file)

The code

nsCOMPtr<nsPrintSettingsX> printSettingsX(do_QueryInterface(aPS));

which appears several times in various .mm files is an abuse of do_QueryInterface, since nsPrintSettingsX is not an interface, so what happens is that aPS gets reinterpreted as an nsPrintSettingsX*.

You may want to add an IID to nsPrintSettingsX which would allow you to write:

nsRefPtr<nsPrintSettingsX> printSettingsX(do_QueryObject(aPS));
Blocks: 514280
How embarrassing :-(

> nsCOMPtr<nsPrintSettingsX> printSettingsX(do_QueryInterface(aPS));

These were originally as follows, until bug 520494 eliminated the nsIPrintSettingsX interface:

nsCOMPtr<nsIPrintSettingsX> printSettingsX(do_QueryInterface(aPS));

I'll come up with a patch.
> what happens is that aPS gets reinterpreted as an nsPrintSettingsX*

I'm surprised there isn't a compiler error, or at least a warning.  Their absence is probably itself a bug.
Indeed, I discovered the problem trying to make it a compiler error (see bug 514280). I don't know if it's possible to make it a compiler warning though.
Attached patch Possible fix (obsolete) — Splinter Review
Were you looking for something like this?
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #8396049 - Flags: review?(neil)
Comment on attachment 8396049 [details] [diff] [review]
Possible fix

This seems fine although I don't think you needed to expand the NS_IMPL_ISUPPORTS_INHERITED1 macro (a peruse through the tree shows it expanded in the case of ambiguous nsISupports inheritance or having to support class info).

I checked that compiles in conjunction with the rest of my changes from bug 514280 (my try push didn't run any tests though because it includes some workarounds for the related dependent bugs just to get the code to compile.)
Attachment #8396049 - Flags: review?(neil) → review+
> I don't think you needed to expand the NS_IMPL_ISUPPORTS_INHERITED1

I came to the same conclusion overnight.  I'll land the patch without the expansion.

Thanks.
Don't expand the NS_IMPL_ISUPPORTS_INHERITED1 macro.  Carrying over Neil's r+.
Attachment #8396049 - Attachment is obsolete: true
Attachment #8396474 - Flags: review+
Comment on attachment 8396474 [details] [diff] [review]
Fix rev1 (don't expand NS_IMPL_ISUPPORTS_INHERITED1)

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f924b14ed31
https://hg.mozilla.org/mozilla-central/rev/1f924b14ed31
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.