986975 - do_QueryInterface abuse in nsAndroidHandlerApp.cpp

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Download Manager, defect)

Description

[[e-mail]]

The code

nsCOMPtr<nsAndroidHandlerApp> aApp = do_QueryInterface(aHandlerApp);

which appears in nsAndroidHandlerApp.cpp is an abuse of do_QueryInterface, since nsAndroidHandlerApp is not an interface, so what happens is that aHandlerApp gets reinterpreted as an nsAndroidHandlerApp*.

The accessor methods on the nsIHandlerApp parameter should probably be used instead.

Comment 1

[[e-mail]]

The code

nsCOMPtr<nsMIMEInfoAndroid::SystemChooser> info = do_QueryInterface(aHandlerApp);

which appears in nsMIMEInfoAndroid.cpp is also an abuse of do_QueryInterface for similar reasons.

Comment 2

[:Paolo Amadini]

Attachment: Bug 986975 - do_QueryInterface abuse in nsAndroidHandlerApp.cpp.

This change is tested in detail as part of bug 1355585.

Review commit: https://reviewboard.mozilla.org/r/130316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/130316/

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8858369 [details]
Bug 986975 - do_QueryInterface abuse in nsAndroidHandlerApp.cpp.

https://reviewboard.mozilla.org/r/130316/#review134894

::: uriloader/exthandler/android/nsAndroidHandlerApp.cpp:66
(Diff revision 1)
> -  nsCOMPtr<nsAndroidHandlerApp> aApp = do_QueryInterface(aHandlerApp);
> -  *aRetval = aApp && aApp->mName.Equals(mName) &&
> -    aApp->mDescription.Equals(mDescription);
> +  *aRetval = false;
> +  if (!aHandlerApp) {
> +    return NS_OK;
> +  }
> +
> +  nsString name;

how long are these names usually? If on average it's small (less than 64 iirc), an AutoString could make sense

::: uriloader/exthandler/android/nsMIMEInfoAndroid.cpp:414
(Diff revision 1)
>    *aRetVal = false;
> +  if (!aHandlerApp) {
> +    return NS_OK;
> +  }
> +
> +  nsString name;

ditto

::: uriloader/exthandler/android/nsMIMEInfoAndroid.cpp:420
(Diff revision 1)
> +  nsString detailedDescription;
> +  aHandlerApp->GetName(name);
> +  aHandlerApp->GetDetailedDescription(detailedDescription);
> +
> +  *aRetVal = name.Equals(u"Android chooser") &&
> +             detailedDescription.Equals(u"Android's default handler application chooser");

these should probably use EqualsLiteral

Both strings should be moved to a #define or a static const char16_t*, so that there's no risk of modifying one instance and not the other one.
All the usage is in this same file.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd40784fb5b
do_QueryInterface abuse in nsAndroidHandlerApp.cpp. r=mak

Comment 5

[Carsten Book [:Tomcat]]

backed out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=93684714&repo=mozilla-inbound

Comment 6

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9588216af214
Backed out changeset 8bd40784fb5b for bustage

Comment 7

[:Paolo Amadini]

(In reply to Marco Bonardo [::mak] from comment #3)
> > +  *aRetVal = name.Equals(u"Android chooser") &&
> > +             detailedDescription.Equals(u"Android's default handler application chooser");
> 
> these should probably use EqualsLiteral

Looks like Android doesn't like EqualsLiteral. I'll re-land the original version with Equals.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d04ed8f11abe
do_QueryInterface abuse in nsAndroidHandlerApp.cpp. r=mak

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d04ed8f11abe