88230 - nsNSSComponent will crash is called when unitialized

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P1)

Description

[Conrad Carlen (not reading bugmail)]

I came across this in working on bug 86021 . It uses profile switching, to
which the NSSComponent listens, to do the job. Problem is, the profile mgr
window needs to be brought up after NSS has been shut down but before is has
been restarted. See
http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSComponent.cpp#746
. During this time at which it's unitialized, mouse movement will cause
RandomUpdate to be called and kaboom. So, the simple patch here checks the state
of mNSSInitilized before calling it. Then I noticed something really odd: that
flag is only set after a profile change but never in the normal couse of things.
Notice how in the destructor, it checks that flag before calling NSS_Shutdown.

Comment 1

[Conrad Carlen (not reading bugmail)]

Attachment: patch to check the init state

Comment 2

[Conrad Carlen (not reading bugmail)]

This bit:

+  nsCOMPtr<nsIObserverService>
observerService(do_GetService(NS_OBSERVERSERVICE_CONTRACTID, &rv));

has nothing to do with the problem reported here. I just noticed that the method
was returning an uninitialized rv :-/

Comment 3

[Javier Delgadillo]

Updating version and adding to cc list.

Comment 4

[Javier Delgadillo]

I'd rather move this block: 
-     
+  
+  mNSSInitialized = PR_TRUE;

to the end of the method.  That way it will only be set if all initialization
succeeded.  Other than that, patch looks good.

Comment 5

[Conrad Carlen (not reading bugmail)]

Javi - I wasn't sure about that myself. Say it fails and we don't set the flag
so we then try again - is that safe?

Comment 6

[Javier Delgadillo]

If we set the flag at the beginning and fail, then the very next time we try to
initialize NSS the very first check in the method will cause an error and that
means NSS will never be initialized.

But after looking at the code, it seems that we just waltz through, so the patch
will work for now, but if we ever (ahem) add error checking past that point,
then we could run into problems.

Bottom line, today that won't cause a problem, but if we modify the code one day
to do more error checking, then we run the risk of missing the fact that we'd
have to move the initialization of mNSSInitialized.

Comment 7

[Javier Delgadillo]

Attachment: Updated patch moving initialization of var to end of method.

Comment 8

[Javier Delgadillo]

Attachment: Patch updated with my comment. r=javi on this patch.

Comment 9

[Conrad Carlen (not reading bugmail)]

Thanks, Javi. Problem is, I can't check this one because it's not part of
SeaMonkeyAll. Can you do this?

Comment 10

[tor]

sr=tor

Comment 11

[Javier Delgadillo]

Patch checked in.

Comment 12

[John Unruh]

Verified per javi's comment.