Avoid the risk of unexpected NSS shutdowns

VERIFIED FIXED in psm2.2

Status

Core Graveyard
Security: UI
P1
normal
VERIFIED FIXED
17 years ago
2 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

1.0 Branch
psm2.2
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

17 years ago
David Baron found this problem. David, please correct if I repeat incorrectly
what Brian told me.

The bug David found can lead to NSS being shut down too early, or too often.

The reason is the PSM code that defines the nsNSSComponent module. This same
component is listed for multiple contract IDs.

Currently, nsNSSComponent is designed to be a singleton. If it is created
multiple times, we run into big problems.

While the code that explicitly asks for the nsNSSComponent uses GetService, thus
accessing the singleton if it has been already created, some of the other
contracts (e.g. the cert handlers) do not use this concept. Instead they use
CreateInstace, which leads to the problems.

Brian Ryner made the following suggestion: "My suggestion would be to split out
nsNSSComponent into separate classes. Right now it's doing way too many things,
and then some of those classes would not need to be singletons."

For details, look at file nsNSSModule.cpp. Several classes use
NS_NSSCOMPONENT_CID / nsNSSComponentConstructor.
I described the problem in a little more detail in attachment 55670 [details] [diff] [review] on bug
75947, in the comment in nsNSSModule.cpp near the end of the patch.
(Assignee)

Comment 2

17 years ago
From the mentioned patch, a comment for the contract IDs in nsNSSModule.cpp.

+  // XXXldb Isn't the |nsNSSComponent| object supposed to be a singleton?
+  // These contract IDs are used with do_CreateInstance in nsURILoader.cpp, so
+  // their use will create additional instances of the nsNSSComponent object
+  // unless we modify nsNSSComponentConstructor to cache its singleton.  (Will
+  // it then work correctly if it being used simultaneously by multiple users
+  // for this purpose?)

Updated

17 years ago
Priority: -- → P1
Target Milestone: --- → 2.2
(Assignee)

Comment 3

17 years ago
In my opinion it is a bug that NS_NSSCOMPONENT_CID is registered to implement
the different contracts starting with NS_CONTENT_HANDLER_CONTRACTID_PREFIX, used
by nsURILoader.

nsURIILoader tries to create an instance for nsIContentHandler. However, neither
nsNSSComponent (which is currently registered for doing that), nor any other
class in security does implement this interface.

I suggest that we remove these four registrations from the code as they have no
effect. I believe that nsURILoader will try to instantiate that class, use
QueryInterface, but fail because of a lack of support for the requested interface.

The real mime type handling for downloading certs is handled by PSMContentListener.

I will attach a patch to remove that code.
(Assignee)

Comment 4

17 years ago
Created attachment 57164 [details] [diff] [review]
Suggested fix

Together with the patch in bug 75947 this will eliminate all additional
registrations for class nsNSSComponent, fixing this bug.
(Assignee)

Updated

17 years ago
Blocks: 75947
(Assignee)

Comment 5

17 years ago
Javi, while we talked about this patch, you suggested to try out whether
obtaining certificates from Verisign still works. I tested it, it works.

Can you please review?

Comment 6

17 years ago
Should 10977 block this?
(Assignee)

Comment 7

17 years ago
Javi, I assume you meant bug 109777 ? I don't think this blocking is necessary.

Bug 109777 blocks bug 75947. I think this is sufficient, as the code removed by
this patch does currently not have any positive effect (as the registered class
does not implement nsIContentHandler), only the negative effect of causing
additional instances of nsNSSComponent being created.
Status: NEW → ASSIGNED

Comment 8

17 years ago
Comment on attachment 57164 [details] [diff] [review]
Suggested fix

r=javi (since there is a new bug to ensure cert downloading works.)
(Assignee)

Updated

17 years ago
Blocks: 99566
Comment on attachment 57164 [details] [diff] [review]
Suggested fix

sr=blizzard
Attachment #57164 - Flags: superreview+
(Assignee)

Comment 10

17 years ago
checked in => fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 11

17 years ago
Verified per kaie's comment.
Status: RESOLVED → VERIFIED

Updated

14 years ago
Component: Security: UI → Security: UI
Product: PSM → Core

Updated

10 years ago
Version: psm2.1 → 1.0 Branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.