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;
NS_ ADDREF( inst); \ InitMethod( ); \ NS_SUCCEEDED( rv)) { \ rface(aIID, aResult); \ Component) \ lized(ensureCal ledByNSSCompone nt); \ lized(ensureRes et); \
rv = inst->_
if(
rv = inst->QueryInte
+ if (triggeredByNSS
+ EnsureNSSInitia
} \
+ else \
+ EnsureNSSInitia
Are you sure you always want to reset? What about: Component) \ lized(ensureRes et); \
+ if (triggeredByNSS
+ EnsureNSSInitia
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 lized(PRBool triggeredByNSSC omponent) Component) { nsISupports> nssComponent PSM_COMPONENT_ CONTRACTID) ;
>-// creating any other components.
>-static void EnsureNSSInitia
>-{
>- static PRBool haveLoaded = PR_FALSE;
>- if (haveLoaded)
>- return;
>-
>- haveLoaded = PR_TRUE;
>-
>- if (triggeredByNSS
>- // We must prevent a recursion, as nsNSSComponent creates
>- // additional instances
>- return;
>- }
>-
>- nsCOMPtr<
>- = do_GetService(
>-}
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 lized(nssEnsure NSSInitializedO p op)
>+// creating any other components.
>+PRBool EnsureNSSInitia
>+{
>+ 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
>+ SSComponent) {
>+ if (op == ensureCalledByN
>+ // We must prevent a recursion, as nsNSSComponent creates
>+ // additional instances
add
haveLoaded = PR_TRUE;
here
>+ return PR_TRUE; nsISupports> nssComponent PSM_COMPONENT_ CONTRACTID) ;
>+ }
>+
>+ nsCOMPtr<
>+ = do_GetService(
>+
>+ // 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?