Closed Bug 497136 Opened 10 years ago Closed 10 months ago

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

Categories

(Core :: XPCOM, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 --- wontfix
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- fixed

People

(Reporter: arno, Assigned: arno)

References

(Depends on 1 open bug)

Details

(Keywords: crash, testcase, Whiteboard: [sg:dos])

Attachments

(3 files, 1 obsolete file)

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
This sounds like a "don't do that" moment to me! Convince this isn't WONTFIX.
Severity: normal → critical
Keywords: crash
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Attached patch patch v1 (obsolete) — Splinter Review
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
Assignee: nobody → arno
Attachment #382327 - Flags: review?(benjamin)
Blocks: 235875
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.
Attachment #382327 - Flags: review?(benjamin) → review+
Comment on attachment 382327 [details] [diff] [review]
patch v1

Could you also please remove all mention of NS_ERROR_SERVICE_NOT_FOUND?
Attached patch patch v1.1Splinter Review
removes all references to NS_ERROR_SERVICE_NOT_FOUND
Attachment #382327 - Attachment is obsolete: true
Attachment #383443 - Flags: review?(benjamin)
Keywords: testcase
Whiteboard: [sg:dos]
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.
Attachment #383443 - Flags: review?(benjamin) → superreview+
Keywords: checkin-needed
the patch fails to apply
Keywords: checkin-needed
Is this now obsolete
This patch should be rebased and landed. See bug 1502774.
See Also: → 1502774
Pushed by VYV03354@nifty.ne.jp:
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
https://hg.mozilla.org/mozilla-central/rev/6773759c86ec
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
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?
Flags: needinfo?(VYV03354)
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.
Flags: needinfo?(VYV03354)
Attachment #9021473 - Flags: review?(nfroyd)
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!).
Attachment #9021473 - Flags: review?(nfroyd) → review+
Thanks for taking a look, Nathan.  Now I feel better about the patch.  :-)
Depends on: 1504039
(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.
You need to log in before you can comment on or make changes to this bug.