497136 - crash when a javascript component components called from C++ is null.

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[arno renevier]

Attachment: testcase: put that file in your component directory to crash

Hi,
to reproduce, create a javascript component whose QueryInterface returns null and call it from C++.
Alternatively, you can overload an internal component like "@mozilla.org/widget/dragservice;1", so gecko will call it for you.
remove compreg.dat and launch your application.
It crashes

Comment 1

[Benjamin Smedberg]

This sounds like a "don't do that" moment to me! Convince this isn't WONTFIX.

Comment 2

[arno renevier]

Attachment: patch v1

That happens because returns NS_ERROR_SERVICE_NOT_FOUND which is actually a success code 
http://mxr.mozilla.org/mozilla-central/source/xpcom/components/nsIServiceManager.idl#98
Then, it is not detected in nsComponentManagerImpl::GetServiceByContractID 
http://mxr.mozilla.org/mozilla-central/source/xpcom/components/nsComponentManager.cpp#2262
My diff just replaces NS_ERROR_SERVICE_NOT_FOUND occurences with NS_ERROR_SERVICE_NOT_AVAILABLE

Comment 3

[arno renevier]

Sorry, I did not see comment 1 before sending my diff. Fell free to review- it or mark it as wontfix if you think that's appropriate. Anyway, I think that NS_ERROR_SERVICE_NOT_FOUND thing is strange.

Comment 4

[Benjamin Smedberg]

Comment on attachment 382327 [details] [diff] [review]
patch v1

Could you also please remove all mention of NS_ERROR_SERVICE_NOT_FOUND?

Comment 5

[arno renevier]

Attachment: patch v1.1

removes all references to NS_ERROR_SERVICE_NOT_FOUND

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 383443 [details] [diff] [review]
patch v1.1

This is what bsmedberg asked for, sr=dveditz

If you don't have check-in privs please add the checkin-needed keyword.

Comment 7

[Dão Gottwald [:dao]]

the patch fails to apply

Comment 8

[Wayne Mery (:wsmwk)]

Is this now obsolete

Comment 9

[Masatoshi Kimura [:emk]]

This patch should be rebased and landed. See bug 1502774.

Comment 10

[Masatoshi Kimura [:emk]]

Attachment: Replace NS_ERROR_SERVICE_NOT_FOUND with NS_ERROR_SERVICE_NOT_AVAILABLE

rebased

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6773759c86ec
Replace NS_ERROR_SERVICE_NOT_FOUND with NS_ERROR_SERVICE_NOT_AVAILABLE. r=benjamin sr=dveditz

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/6773759c86ec

Comment 13

[(no longer active)]

Wait...  This changes the behavior in very subtle ways, exactly as I described in https://bugzilla.mozilla.org/show_bug.cgi?id=1502774#c14.  I think this should at least have received a review from Nathan before landing.  I can definitely see this patch breaking things.  What do you think, Masatoshi?

Comment 14

[Masatoshi Kimura [:emk]]

Comment on attachment 9021473 [details] [diff] [review]
Replace NS_ERROR_SERVICE_NOT_FOUND with NS_ERROR_SERVICE_NOT_AVAILABLE

Hmm... The patch might have been a bit too old to carry over the review flags. OK, requesting review.

This patch will change the behavior only when nsIClassFactory::CreateInstance returns a nullptr with a success code. Basically it is impossible these days since we dropped support for legacy addons.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 9021473 [details] [diff] [review]
Replace NS_ERROR_SERVICE_NOT_FOUND with NS_ERROR_SERVICE_NOT_AVAILABLE

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

I think Ehsan is correct that this could have interesting effects, but I also think not having legacy-style addons minimizes the potential breakage here.  The followup below should tell us if we were doing things incorrectly.

::: xpcom/components/nsComponentManager.cpp
@@ +1044,5 @@
>    if (factory) {
>      rv = factory->CreateInstance(aDelegate, aIID, aResult);
>      if (NS_SUCCEEDED(rv) && !*aResult) {
>        NS_ERROR("Factory did not return an object but returned success!");
> +      rv = NS_ERROR_SERVICE_NOT_AVAILABLE;

This seems like a reasonable change.  I think we should do a followup to make this code (and other instances) look something like:

rv = factory->CreateInstance(...);
// Maybe even MOZ_DIAGNOSTIC_ASSERT.
if (NS_SUCCEEDED(rv)) {
  MOZ_ASSERT(*aResult, "Successful CreateInstance should have returned an object");
  if (!*aResult) {
    rv = NS_ERROR_SERVICE_NOT_AVAILABLE;
  }
}

to more loudly tell people they are doing something wrong (and to more loudly catch any instances of this problem that we are having!).

Comment 16

[(no longer active)]

Thanks for taking a look, Nathan.  Now I feel better about the patch.  :-)

Comment 17

[Masatoshi Kimura [:emk]]

(In reply to Nathan Froyd [:froydnj] from comment #15)
> This seems like a reasonable change.  I think we should do a followup to
> make this code (and other instances) look something like:
> 
> rv = factory->CreateInstance(...);
> // Maybe even MOZ_DIAGNOSTIC_ASSERT.
> if (NS_SUCCEEDED(rv)) {
>   MOZ_ASSERT(*aResult, "Successful CreateInstance should have returned an
> object");
>   if (!*aResult) {
>     rv = NS_ERROR_SERVICE_NOT_AVAILABLE;
>   }
> }
> 
> to more loudly tell people they are doing something wrong (and to more
> loudly catch any instances of this problem that we are having!).

Filed bug 1504039 as a follow-up.