Closed Bug 79153 Opened 23 years ago Closed 23 years ago

No indication that a key is being generated.

Categories

(Core :: Security: PSM, defect, P1)

Product:
Core
Component:
Security: PSM
Version:
1.0 Branch
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
psm2.2

People

(Reporter: junruh, Assigned: KaiE)

References

(
URL
)

Details

Attachments

(10 files, 5 obsolete files)

Screenshot from Communicator 4.x
2.45 KB, image/png
Details
Another Screenshot
5.08 KB, image/png
Details
Implementation
13.53 KB, patch
Details | Diff | Splinter Review
New implementation, status info message while generating
21.48 KB, patch
Details | Diff | Splinter Review
Modified patch, implementing 2) and 3), not yet solving 1)
22.52 KB, patch
Details | Diff | Splinter Review
New revision of patch, as suggested by Javi.
22.94 KB, patch
Details | Diff | Splinter Review
Updated patch as requested; fixed return stmts. in nsKeygenThread::Join()
23.02 KB, patch
Details | Diff | Splinter Review
Additional patch
964 bytes, patch
Details | Diff | Splinter Review
Patch to fix Mac.
596 bytes, patch
Details | Diff | Splinter Review
Updated patch (all-in-one), includes Javi's suggested addition
11.46 KB, patch
KaiE
: review+
blizzard
: superreview+
Details | Diff | Splinter Review 
1.) Visit the Verisign site for a trial cert.
2.) Select 2048K key size and click on the Accept button.
What is expected: A dialog stating that a key is being generated.
What happens: The 500 Mhz PC appears to be locked up for 20-30 seconds.
Target Milestone: --- → 2.0
->p1
Priority: -- → P1
Whiteboard: PDT
PDT+ as per Steve Elmeer 6/20/2001
Whiteboard: PDT → PDT+
Whiteboard: PDT+ → PDT+; eta 6/22
Removing from PDT+.

Fix is too involved.  Contacted Verisign. They're agreeable to place a warning
on a javascript pop up they use on the cert request page that the key gen will
take a long time.

target 2.1
Whiteboard: PDT+; eta 6/22 → PDT; eta 6/22
Target Milestone: 2.0 → 2.1
Removing PDT keyword.
Whiteboard: PDT; eta 6/22
Keywords: nsenterprise
In Communicator 4.x, this behaved differently.

Before the key generation starts, Communicator displays a message box giving
info to the user, with the option to cancel and access a longer explanation text.

I suggest we keep this behaviour, maybe adding a third box, right after the user
presses OK to generate, displaying the text "Please Wait - Key Generation in
Progress". However, depending on how much work it is, I might not add a cancel
mechanism and not display an icon in the message.

The text uses the phrase "Communicator is about to create...".. As we will use
this message in mutliple products (Mozilla/Netscape), I will change this to:
"This computer is about to create...".
Assignee: ddrinan → kai.engert
Attaching a patch implementing the suggested behaviour.
Instead of "Communicator" the text "Personal Security Manager" is used.


If you want to test without going to a CA (much easier), create a local file
with the following contents and open it with the browser:

<html>
<body>
<form>
<keygen name="testkey" challenge="none">
<input type="submit">
</form>
</body>

David, can you please review?
Keywords: patch, review
The problem I see with this patch is that it forces the user to click "OK" when
key generation occurs.  In Communicator, there was a "Cancel" button and that's
it.  We need to minimize the number of times a user has to click buttons before
receiving a certificate.

The new dialog should have a cancel button only and no "OK" button.
If I understand you correctly, you are suggesting the following behaviour:
- PSM displays the message "key generation in progress - please wait", with a
cancel button, and maybe a progress indicator.
- PSM starts immediately generating the key pair.
- If the user clicks on cancel while generation is in progress, it will be
interrupted.
- As soon as key generation is finished, the window disappears automatically and
the browser continues to submit the form.

The key generation should be done in a separate thread to make sure the user
interface is responsive.

Do we want that behaviour for 2.1?

I want to modify the suggestion. As there is no other dialog of that kind in
Mozilla, at least none I know of, we could move this status message to the
status bar.

Key generation should be moved to a different thread as suggested. If the user
presses the stop button to cancel loading the page, key generation will be
canceled, too.

Using this approach we can share the already available progress indicator used
while conneting to / loading a web page.

I don't think we'll gain much by putting key generation on a separate thread
since the UI thread will remain blocked waiting for the key generation to happen
(which means no UI events will get processed).

You can try this, but my initial reaction is that you'll just add over-head to
the problem without solving what you set out to solve.  :(

ddrinan looked at this a while back and we quickly came to the conclusion this
was a "hard" problem.  We might have to get some necko advice with regards to
the threading model in order to do what we want.
The above patch doesn't block until key generation is triggered. The UI is
repsonsive while the dialog is shown. However, this is on Linux and I don't know
how the other platforms behave.

If we can assume that this behaviour is identical on all platforms, I want to
suggest the following design, which indeed is some work to implement.

The idea is to show the dialog, having the text "Key Generation in progress" and
as suggested at most a cancel button, if any.

In the JS onLoad function of the dialog, we use XPConnect to fire a thread in
the background which starts generating the key pair. This should still not
block. In addition we can set the "wait" cursor.

If key generation is finished, the thread closes the dialog.

If the dialog closes, the nsKeygenHandler will check the state of the runnable
whether key generation is finished or not (depending on who closed the dialog,
the user or the thread). If it is finished, it takes the key and continues.
But if key generation is not yet finished, it assumes the user canceled, will
change the state of the Runnable to be in detached state (and should delete all
resources once it is finished - as we can't interrupt the call to
PK11_GenerateKeypair). This will mean there is some calculation going on in the
background while we are unable to cancel the thread, but the system will not
block and be responsive while the user continues using the application.

Above design is what I personally think might be the optimal implementation. The
only drawback is the amount of work required to do it.

We agreed that we don't want the user to press OK first. But if we don't want
that, we can't display a message on the screen, unless we move key generation to
a separate thread. If we try to show a window on the screen, the window will not
be displayed until we yield execution to the UI, effectively further delaying
the start of key generation.

If we want a solution without additional work, we could use the above patch,
which at least makes sure the user has seen a window explaining what is going to
happen, and this window will still be visible while key generation is in
progress, as the user interface blocks and is not removed from the screen until
we are finished with key generation.

As a first step, I have implemented this feature as an informational status
message only, without the ability to cancel. I fear canceling might be dangerous
unless we are able to generate the key in the off, without giving the key
generation thread the ability to directly access the security device, i.e.
security slot.

Javier, can you please review this patch?
kaie: when this is ready to land, make sure you buddy up with me so that I can
check in the corresponding Mac project changes.  You've added some new files
that will break the Mac build.
One overall note that I just remembered that I should've remembered before.

1) This implementation is not embedding friendly.  If an emdeder comes along
that doesn't have XUL, then KEYGEN will flat out not work for them.  That would
be a bad thing.  :(

Notes specific to the code:
1) In the directory mozilla/security/manager/ssl/src, we should be absolutely
positive that we want to fail if the UI module is not present.  It seems like we
used to allow an embedding project to generate a key pair, and with this patch
unless XUL is supported, that is no longer the case.  If we're going to make
that decision, we should be conscious of it.

2) The Mozilla standard is to have accessors for public variables.  So all the
variables that are public in nsKeygenThread, should become private/protected (I
don't have a preference) and have Get/Set accessors method like all the other
objects in Mozilla.

3) Use nsnull instead of NULL.  That's the convention in Mozilla code, so we
should stick to it at all times.

As we don't need to set the options independently of each other, I chose
to define a new type, a struct, which carries all the variables, and I
use only one accessor function to set this.

Will you accept the following code? Or do you think that tansferring a
reference to the thread, which will be modified by the thread, while
keeping the ownership, is not clean enough?

To solve comment 1) from Javi, we decided to add a switch to the code. The
switch checks whether the interface nsIGeneratingKeypairInfoDialogs is
implemented or not.

If it is, the new logic with status feedback is used.

If it is not, the old blocking behaviour is used.
Status: NEW → ASSIGNED
Javi, can you please review?
r=javi
Mass assigning QA to ckritzer.
QA Contact: junruh → ckritzer
sr=blizzard
Before checking in, please modify the following lines:
+<script src="chrome://global/content/strres.js" />
+<script src="pippki.js" />
+<script src="createCertInfo.js" />
+<script type="application/x-javascript" src="chrome://help/content/help.js" />

to conform with comments in Bug 88328
Patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Keywords: patch, review
Resolution: --- → FIXED
Okay, when I use the html code provided by Kai 
(http://tee.mcom.com/bugzilla/bugzilla79153.html), I get the alert box that a 
key is being generated...then the app crashes.

But, when I go to the verisign site and click the [Enroll Now] button, I get no 
key generation dialogue, and nothing appears to be happening - the app doesn't 
go to the site associated with that link.

I'm tempted to re-open this bug...I'll try a new profile first to see if there's 
any weirdness there.
I tried a fresh profile and the dialogue is now being generated.

Caveats:

1) There is no cancel button.  It is unclear in the latter part of the bug 
report after Kai mentions checking in a patch which will throw an informational 
dialogue whether or not a [Cancel] button should exist.

2) It does, however make Windows crash, Mac hang (nothing appears to be 
happening after 10+ minutes) and Linux was the only platform that actually 
generated the key without problem(s).

If the absence of the [Cancel] button is intentional design, then I would 
consider this bug fixed and will mark it as such.  Kai, could you comment?  
Thanks.

1) I have never seen the key generation crash. I used your test page, it works
for me. I tried both Linux and Window 2000. I'll contact you directly to see if
we can reproduce this on my machine.
2) If you want to test using the Verisign page, there are additional steps
required. First, go to the above URL. Then click on the green "Enroll Now"
image. In the next window, you first have to fill out the form, at least first,
last, email, challenge phrase, select 60 day test drive, and press the green
accept button. Now the key generation should start. It does on my system.
3) The cancel button is indeed missing by design. Unless we redesign the
internal code which generates the key, we'd risk key database corruption if we
supported canceling. If you think canceling is important, let's open a new bug
for that request.

Hey Kaie,

1) Crashes for me on Win98SE about 45sec after clicking the [Accept] button 
(following the steps you outlined in #2 above) - I'll check a Win2k box in the 
lab.
2) Yep, followed all these steps.
3) I don't feel that strongly about a [Cancel] button. If anyone else does, they 
can file a bug ;-)
Stack Signature
               KERNEL32.DLL + 0x1abea (0xbff8abea) 71353471 
 Bug ID
               79153 
 Trigger Time 
               2001-08-21 08:20:32 
 Email Address 
               ckritzer 
 User Comments 
               1) Use the html test page provided in bugzilla#79153 2) Click 
[Submit Query] button to generate key 3) CRASH! 
 Build ID
               2001082015 
 Product ID
               MozillaTrunk 
 Platform ID
               Win32 
 Trigger Reason
               Access violation 
 Stack Trace

KERNEL32.DLL + 0x1abea (0xbff8abea) 
nsKeygenFormProcessor::ProcessValue 
[d:\builds\seamonkey\mozilla\security\manager\ssl\src\nsKeygenHandler.cpp, line 
545] 
nsFormFrame::ProcessValue 
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormFrame.cpp, line 654] 
nsFormFrame::ProcessAsURLEncoded 
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormFrame.cpp, line 1215] 
nsFormFrame::OnSubmit 
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormFrame.cpp, line 708] 
nsHTMLFormElement::DoSubmitOrReset 
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLFormElement.cpp, 
line 518] 
nsHTMLFormElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLFormElement.cpp, 
line 462] 
PresShell::HandleEventInternal 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5653] 
PresShell::HandleEventWithTarget 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5623] 
nsFormControlHelper::DoManualSubmitOrReset 
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormControlHelper.cpp, line 
1001] 
nsHTMLButtonControlFrame::MouseClicked 
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsHTMLButtonControlFrame.cpp, 
line 367] 
nsHTMLInputElement::HandleDOMEvent 
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLInputElement.cpp, 
line 1265] 
PresShell::HandleEventInternal 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5653] 
PresShell::HandleEventWithTarget 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5623] 
nsEventStateManager::CheckForAndDispatchClick 
[d:\builds\seamonkey\mozilla\content\events\src\nsEventStateManager.cpp, line 
2466] 
nsEventStateManager::PostHandleEvent 
[d:\builds\seamonkey\mozilla\content\events\src\nsEventStateManager.cpp, line 
1552] 
PresShell::HandleEventInternal 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5674] 
PresShell::HandleEvent 
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5578] 
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 377] 
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 350] 
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 350] 
nsViewManager::DispatchEvent 
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 2058] 
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 68] 
nsWindow::DispatchEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 732] 
nsWindow::DispatchWindowEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 749] 
nsWindow::DispatchMouseEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4264] 
ChildWindow::DispatchMouseEvent 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4514] 
nsWindow::ProcessMessage 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3251] 
nsWindow::WindowProc 
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 997] 
KERNEL32.DLL + 0x363b (0xbff7363b) 
KERNEL32.DLL + 0x242e7 (0xbff942e7) 
0x00688b5a 
Okay, Kaie sent me a build (with a fix by Wan-Teh?)  that works just great, no
crashing, correct dialog, etc. so I'm going to check tomorrow's
(2001-08-22-xx-trunk) bits and hopefully get this bug verified.
Still crashes on 2001-08-22-06-trunk commercial build.

And it also never appears to generate a key on MacOS91 & MacOS_X (the
"Generating A Private Key" dialogue never goes away, and no key appears in the
url field)

Second Stack trace for Windows Crash:
Incident ID 34382362
Stack Signature KERNEL32.DLL + 0x1abea (0xbff8abea) 71353471
Bug ID
Trigger Time 2001-08-22 11:48:55
Email Address ckritzer@netscape.com
User Comments DANGITDANGITDANGIT
Build ID 2001082206
Product ID MozillaTrunk
Platform ID Win32
Trigger Reason Access violation
Stack Trace
KERNEL32.DLL + 0x1abea (0xbff8abea)
nsKeygenFormProcessor::ProcessValue
[d:\builds\seamonkey\mozilla\security\manager\ssl\src\nsKeygenHandler.cpp, line 580]
nsFormFrame::ProcessValue
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormFrame.cpp, line 654]
nsFormFrame::ProcessAsURLEncoded
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormFrame.cpp, line 1215]
nsFormFrame::OnSubmit
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormFrame.cpp, line 708]
nsHTMLFormElement::DoSubmitOrReset
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLFormElement.cpp,
line 518]
nsHTMLFormElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLFormElement.cpp,
line 462]
PresShell::HandleEventInternal
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5653]
PresShell::HandleEventWithTarget
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5623]
nsFormControlHelper::DoManualSubmitOrReset
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormControlHelper.cpp, line
1005]
nsHTMLButtonControlFrame::MouseClicked
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsHTMLButtonControlFrame.cpp,
line 367]
nsHTMLInputElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLInputElement.cpp,
line 1265]
PresShell::HandleEventInternal
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5653]
PresShell::HandleEventWithTarget
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5623]
nsEventStateManager::CheckForAndDispatchClick
[d:\builds\seamonkey\mozilla\content\events\src\nsEventStateManager.cpp, line 2466]
nsEventStateManager::PostHandleEvent
[d:\builds\seamonkey\mozilla\content\events\src\nsEventStateManager.cpp, line 1552]
PresShell::HandleEventInternal
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5674]
PresShell::HandleEvent
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5578]
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 377]
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 350]
nsView::HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 350]
nsViewManager::DispatchEvent
[d:\builds\seamonkey\mozilla\view\src\nsViewManager.cpp, line 2056]
HandleEvent [d:\builds\seamonkey\mozilla\view\src\nsView.cpp, line 68]
nsWindow::DispatchEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 732]
nsWindow::DispatchWindowEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 749]
nsWindow::DispatchMouseEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4264]
ChildWindow::DispatchMouseEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4514]
nsWindow::ProcessMessage
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3251]
nsWindow::WindowProc
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 997]
KERNEL32.DLL + 0x363b (0xbff7363b)
KERNEL32.DLL + 0x242e7 (0xbff942e7)
0x00688b5a 
The reason Mac keygen is failing is because the thread created to do the keygen
never runs so everything deadlocks.

This is the line where the thread gets created.  Is there anything else that
needs to be done to ensure the thread gets run on the Mac?  Or will the Mac's
threading model not allow us to create a thread from withing a call that happens
via xpconnect?

http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsKeygenThread.cpp#93
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
adding beard and sfraser to cc: list.  Please look at Javier's question regarding 
Mac thread creation

We aren't/weren't running a PR_PRIORITY_LOW thread?

r= kaie
Status: REOPENED → ASSIGNED
The NSPR thread scheduler will not run a low priority thread
if there is always a thread at a higher priority that is
runnable.  Even PR_Sleep(0) won't help because an NSPR thread
only yields to a runnable thread at the same or higher priority.
Does the patch work?

yes.  I ran it on my PowerBook with wtc looking over my shoulder.
sr=sfraser

a=asa on behalf of drivers.
Above patch checked in. However, I don't want to close the bug yet, as we still
see this crash on Windows 98.

I might have found the reason. I'm using a stack based instance, that is passed
on to the JavaScript dialog, and from there to the newly created thread.

I thought this would be no problem, because the caller calls Join on the newly
created thread, effectively waiting until it is finished. Therefore it is
guaranteed that the stack based instance will be destroyed *after* the thread is
finished.

But I ignored the fact that the JavaScript code will have a reference to this
instance, too. And the JavaScript code runs on the UI thread. So it might be
possible, the thread creator leaves its scope before the JavaScript releases its
reference.
If someone confirms that above statements make sense, we should add the new
patch that I'll attach right after this comment.
I can't test it, because I can't reproduce this patch on my system. On all my
systems, it runs with and without this patch. This looks like a thread race.
Wont't this line suffer from the same symptom you're trying to fix?  SetParams
doesn't make a copy of the parameters passed in, so you could have another
thread with access to the static variable as well.

In general, I agree that it's a bad idea to be passing local variables from one
method to a structure that could be used in another thread without the
appropriate ref counting mechanisms.

+  KeygenRunnable->SetParams(&gkp);
I rewrote parts of the patch, in the hope it will fix the crash that I'm still
not able to reproduce.

Getting rid of any stack based instances passed between interfaces/threads.

Releasing the reference to the status window is delayed until the thread
instance is destroyed.
Attaching the new diff - with 10 lines context to ease reviewing.

The 2001 08 24 03 build on WinME works fine for me at Verisign.  I generated a
2k-bit key without any problems.

I still can't reproduce this crash. It seems for me, my latest patch is not
required.

Therefore I set this to "fixed". If we continue to see the crash, I suggest to
open a new bug.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Reopening.

In the page used by Verisign, the key generation is triggered by a HTML tag
<KEYGEN> in the page. JUnruh showed me a page (used in Netscape CMS) where key
generation is triggered by a JavaScript function. I wasn't aware that there are
multiple code paths to do this.

For example: https://junruh.mcom.com:444/backup/DirUserEnrollSign1024.html

For this page, no indication is given while the key is generated.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
We'll fix this part in the next release.
Target Milestone: 2.1 → Future
I already created a patch which I will attach. It adds the status dialog for the
new code path.

I'm mixing this with my previous patch (not yet checked in), which makes the
code safer.

Since I worked on this bug the last time, I have been learning more about how
the Mozilla architecture works. I now fear that instantiating the nsKeygenThread
on the stack might not be a good idea at all. Instead, the perfect way would be
to move all the methods over to the interface, create a mapping to declare this
class as one implementing the interface, and use do_CreateInstance. This could
eliminate more risks, and not doing this might have been the cause for the crash
that ckritzer saw. (This additional change is not in the patch yet.)

Changing my prefered e-mail address.
Assignee: kai.engert → kaie
Status: REOPENED → NEW
Status: NEW → ASSIGNED
Attachment #49950 - Attachment is obsolete: true
Attachment #47037 - Attachment is obsolete: true
I think I finally found the reason that explains the crash we saw on Windows 98,
and also explains, why I was never able to reproduce it.

The constructor of nsKeygenThread does not init the XPCom reference counters,
the call to NS_INIT_ISUPPORTS() is missing. This means, the reference counter
has a random value. If it happens to be zero, we will crash.

This is the reason, why the current code, creating that instance stack based,
seemd to work. Obviously, in most cases the reference count was not zero, and
the destruction code was not reached. Only on that crashing Windows 98 it must
have contained zero.

I'm creating a new patch that completely eliminates all stack based instances
passed around. Bascially it is a mixture of the patches found above.

First, I will create a patch that only fixes the already checked in code, and I
will later create a separate patch for the other code path (key generation
triggered from JavaScript).

> The constructor of nsKeygenThread does not init the XPCom reference counters,
Purify would have found this. Has anyone run Purify over the security code?
Attachment #46859 - Attachment is obsolete: true
QA Contact: ckritzer → junruh
Only thing is that it appears there may be a dangling/leaking of a reference
potential to a PK11SlotInfo * by simply nulling out the slot pointer after
generating the key.  Add calls to PK11_ReferenceSlot and PK11_FreeSlot to get
rid of that possibility and r=javi.
marking nsenterprise-; will be reevaluated for nsenterprise in future release.
Keywords: nsenterprise-
Attachment #52570 - Attachment is obsolete: true
Attachment #52595 - Attachment is obsolete: true
Comment on attachment 54590 [details] [diff] [review]
Updated patch (all-in-one), includes Javi's suggested addition

r=javi
Attachment #54590 - Flags: review+
target PSM 2.2
Target Milestone: Future → 2.2
+  if (NS_FAILED(rv) || !KeygenRunnable) {

This seems a little odd to me.  If there's a failure you're still trying to
generate the key pair.  I would personally just return.  Is that expected to
happen in even a common edge case?
blizzard: The idea behind this: Maybe an embedding app doesn't implement the
required GUI and key generation feedback that we are using inside Mozilla.

We don't want to require that the feedback UI is implemented. If it is not
implemented, that's fine. In that case we generate the key pair as it was done
in the old code: with blocking UI.

Comment on attachment 54590 [details] [diff] [review]
Updated patch (all-in-one), includes Javi's suggested addition

sr=blizzard
Attachment #54590 - Flags: superreview+
Checked in to trunk.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Verified on 10/29 trunk.
Status: RESOLVED → VERIFIED
Product: PSM → Core
Version: psm2.0 → 1.0 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: