576680 - "Conditional jump or move depends on uninitialised value(s) / nsPrefBranch::GetIntPref(char const*, int*) (nsPrefBranch.cpp:247)"

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Got this several times valgrind'ing test-ipc.xul.  Not sure if fennectrolysis has the same bug, will see what's up.

==11476== Conditional jump or move depends on uninitialised value(s)
==11476==    at 0x5C6200E: nsPrefBranch::GetIntPref(char const*, int*) (nsPrefBranch.cpp:247)
==11476==    by 0x6D00F08: nsNavHistory::LoadPrefs() (nsNavHistory.cpp:2115)
==11476==    by 0x6CF3E9F: nsNavHistory::Init() (nsNavHistory.cpp:428)
==11476==    by 0x6CF2B12: nsNavHistory::GetSingleton() (nsNavHistory.cpp:376)
==11476==    by 0x6D63ED3: nsNavHistoryConstructor(nsISupports*, nsID const&, void**) (nsPlacesModule.cpp:15)
==11476==    by 0x70C24D6: mozilla::GenericFactory::CreateInstance(nsISupports*, nsID const&, void**) (GenericFactory.cpp:48)
==11476==    by 0x7126EFB: nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) (nsComponentManager.cpp:1148)
==11476==    by 0x7127C63: nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) (nsComponentManager.cpp:1510)
==11476==    by 0x70B0E80: CallGetService(char const*, nsID const&, void**) (nsComponentManagerUtils.cpp:94)
==11476==    by 0x70B13C1: nsGetServiceByContractID::operator()(nsID const&, void**) const (nsComponentManagerUtils.cpp:278)
==11476==    by 0x6B2FA00: nsCOMPtr<nsINavHistoryService>::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) (nsCOMPtr.h:1252)
==11476==    by 0x6B2F0C0: nsCOMPtr<nsINavHistoryService>::nsCOMPtr(nsGetServiceByContractID) (nsCOMPtr.h:627)
==11476==

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is caused by

    nsresult rv;
    PRInt32 retval;
    cpc->SendGetIntPref(nsDependentCString(getPrefName(aPrefName)), &retval, &rv);
    if (NS_SUCCEEDED(rv))
      *_retval = retval;
    return rv;

where apparently the |cpc->SendGetIntPref()| is failing because of a closed channel.  Not a huge deal since the content process would be _exit()ing soon thereafter, but I think it's worth initializing rv with NS_ERROR_NOT_AVAILABLE here (and nearby) if only to shut valgrind up.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Initialize |rv| so that valgrind doesn't complain (though it's technically unnecessary) and add a helper function to cut some LoC

Comment 3

[[e-mail]]

Comment on attachment 455805 [details] [diff] [review]
Initialize |rv| so that valgrind doesn't complain (though it's technically unnecessary) and add a helper function to cut some LoC

> #ifdef MOZ_IPC
>-    if (XRE_GetProcessType() == GeckoProcessType_Content) {
>-      mozilla::dom::ContentProcessChild *cpc = 
>-        mozilla::dom::ContentProcessChild::GetSingleton();
>-      NS_ASSERTION(cpc, "Content Protocol is NULL!");
>-
>+    if (ContentProcessChild* cpc = GetContentProcessChild()) {
>       nsAutoString prefValue;
>       cpc->SendGetPrefLocalizedString(nsDependentCString(getPrefName(aPrefName)), 
>                                       &prefValue, &rv);
>       if (NS_FAILED(rv)) return rv;

We should init 'rv' here too, right? It'll be set to NS_OK from the prior call.

Should we be checking the return value of the Send* methods instead? I initially thought they aborted on failure but I guess they just return false in the closed channel case :/

r=dwitte either way.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #3)
> (From update of attachment 455805 [details] [diff] [review])
> > #ifdef MOZ_IPC
> >-    if (XRE_GetProcessType() == GeckoProcessType_Content) {
> >-      mozilla::dom::ContentProcessChild *cpc = 
> >-        mozilla::dom::ContentProcessChild::GetSingleton();
> >-      NS_ASSERTION(cpc, "Content Protocol is NULL!");
> >-
> >+    if (ContentProcessChild* cpc = GetContentProcessChild()) {
> >       nsAutoString prefValue;
> >       cpc->SendGetPrefLocalizedString(nsDependentCString(getPrefName(aPrefName)), 
> >                                       &prefValue, &rv);
> >       if (NS_FAILED(rv)) return rv;
> 
> We should init 'rv' here too, right? It'll be set to NS_OK from the prior call.
> 

Good call.

> Should we be checking the return value of the Send* methods instead? I
> initially thought they aborted on failure but I guess they just return false in
> the closed channel case :/
> 

Yeah, they currently return false until we process the channel-error notification (and then are supposed to _exit()).  The contract for subprocess code is that return values from Send*() don't need to be checked; your code is perfectly fine.  We might change the disconnect implementation, but that's not something to worry about.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/4f8b85f53eba