Comment 77 for bug 274065

Revision history for this message
In , Kai Engert (kaie) wrote :

     NS_ADDREF(inst); \
     rv = inst->_InitMethod(); \
     if(NS_SUCCEEDED(rv)) { \
         rv = inst->QueryInterface(aIID, aResult); \
+ if (triggeredByNSSComponent) \
+ EnsureNSSInitialized(ensureCalledByNSSComponent); \
     } \
+ else \
+ EnsureNSSInitialized(ensureReset); \

Are you sure you always want to reset? What about:
+ if (triggeredByNSSComponent) \
+ EnsureNSSInitialized(ensureReset); \

Or is there a reason I don't see?
In my understanding, we can arrive here while NSS is initialized, but we failed to create some other object (maybe hash wrapper object etc.)

>-// We must ensure that the nsNSSComponent has been loaded before
>-// creating any other components.
>-static void EnsureNSSInitialized(PRBool triggeredByNSSComponent)
>-{
>- static PRBool haveLoaded = PR_FALSE;
>- if (haveLoaded)
>- return;
>-
>- haveLoaded = PR_TRUE;
>-
>- if (triggeredByNSSComponent) {
>- // We must prevent a recursion, as nsNSSComponent creates
>- // additional instances
>- return;
>- }
>-
>- nsCOMPtr<nsISupports> nssComponent
>- = do_GetService(PSM_COMPONENT_CONTRACTID);
>-}

I'm wondering about races for variable haveLoaded.
With the old code, the variable started at false, and whatever happened afterwards, it remained at true, so it wasn't necessary to use a lock/mutex.

I see a possible race with the new code, because of your new "reset" feature.

Let's avoid unnecessary changes to the variable, only change when it's about the nssComponent object.

>+// We must ensure that the nsNSSComponent has been loaded before
>+// creating any other components.
>+PRBool EnsureNSSInitialized(nssEnsureNSSInitializedOp op)
>+{
>+ static PRBool haveLoaded = PR_FALSE;
>+
>+ if (op == ensureReset) {
>+ haveLoaded = PR_FALSE;
>+ return PR_FALSE;
>+ }
>+
>+ if (haveLoaded)
>+ return PR_TRUE;
>+
>+ haveLoaded = PR_TRUE;

I propose to remove this line

>+
>+ if (op == ensureCalledByNSSComponent) {
>+ // We must prevent a recursion, as nsNSSComponent creates
>+ // additional instances

add
      haveLoaded = PR_TRUE;
here

>+ return PR_TRUE;
>+ }
>+
>+ nsCOMPtr<nsISupports> nssComponent
>+ = do_GetService(PSM_COMPONENT_CONTRACTID);
>+
>+ // Check if something didn't fail during nss init, if so,
>+ // uncheck the haveLoaded flag to try again later.
>+ if (!nssComponent)
>+ haveLoaded = PR_FALSE;

revert this to:

    if (nssComponent)
      haveLoaded = PR_TRUE;

>+
>+ return haveLoaded;
>+}

Does this make sense?