82359 - Crash when hitting "Submit query" in Bugzilla - Trunk [@ nsKeygenFormProcessor::Init]

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[coen]

OS: win98
Build: 2001052304 talkback installer
Reproducible: Always

Comment 1

[coen]

Submitting other forms also crash. (IE google.com, altavista.com...)

Comment 2

[WD]

Confirmed on my WinMe system w/ 2001052304

Talkback ID's:
TB30811255W
TB30811227Q
TB30810859W

Comment 3

[Doron Rosenberg (IBM)]

kewords

Comment 4

[coen]

Build 2001052206 don't show this problem

Comment 5

[Phil Peterson]

Stack trace points to PSM keygen processor. cc ddrinan and javi

 Call Stack:     (Signature = nsKeygenFormProcessor::Init 96f83400)
     nsKeygenFormProcessor::Init
[d:\builds\seamonkey\mozilla\security\manager\ssl\src\nsKeygenHandler.cpp, line 185]
     nsKeygenFormProcessor::Create
[d:\builds\seamonkey\mozilla\security\manager\ssl\src\nsKeygenHandler.cpp, line 167]
     nsGenericFactory::CreateInstance
[d:\builds\seamonkey\mozilla\xpcom\components\nsGenericFactory.cpp, line 56]
     nsComponentManagerImpl::CreateInstance
[d:\builds\seamonkey\mozilla\xpcom\components\nsComponentManager.cpp, line 1206]
     nsComponentManager::CreateInstance
[d:\builds\seamonkey\mozilla\xpcom\components\nsRepository.cpp, line 82]
     nsServiceManagerImpl::GetService
[d:\builds\seamonkey\mozilla\xpcom\components\nsServiceManager.cpp, line 345]
     nsServiceManager::GetService
[d:\builds\seamonkey\mozilla\xpcom\components\nsServiceManager.cpp, line 560]
     nsGetServiceByCID::operator()
[d:\builds\seamonkey\mozilla\xpcom\components\nsServiceManager.cpp, line 46]
     nsCOMPtr_base::assign_from_helper
[d:\builds\seamonkey\mozilla\xpcom\base\nsCOMPtr.cpp, line 66]
     nsFormFrame::OnSubmit
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormFrame.cpp, line 738]
     nsHTMLFormElement::DoSubmitOrReset
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLFormElement.cpp,
line 523]
     nsHTMLFormElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLFormElement.cpp,
line 467]
     PresShell::HandleEventInternal
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5514]
     PresShell::HandleEventWithTarget
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5486]
     nsFormControlHelper::DoManualSubmitOrReset
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsFormControlHelper.cpp, line
998]
     nsHTMLButtonControlFrame::MouseClicked
[d:\builds\seamonkey\mozilla\layout\html\forms\src\nsHTMLButtonControlFrame.cpp,
line 363]
     nsHTMLInputElement::HandleDOMEvent
[d:\builds\seamonkey\mozilla\content\html\content\src\nsHTMLInputElement.cpp,
line 1249]
     PresShell::HandleEventInternal
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5514]
     PresShell::HandleEventWithTarget
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5486]
     nsEventStateManager::CheckForAndDispatchClick
[d:\builds\seamonkey\mozilla\content\events\src\nsEventStateManager.cpp, line 2464]
     nsEventStateManager::PostHandleEvent
[d:\builds\seamonkey\mozilla\content\events\src\nsEventStateManager.cpp, line 1550]
     PresShell::HandleEventInternal
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5535]
     PresShell::HandleEvent
[d:\builds\seamonkey\mozilla\layout\html\base\src\nsPresShell.cpp, line 5441]
     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 706]
     nsWindow::DispatchWindowEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 723]
     nsWindow::DispatchMouseEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4053]
     ChildWindow::DispatchMouseEvent
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 4298]
     nsWindow::ProcessMessage
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 3051]
     nsWindow::WindowProc
[d:\builds\seamonkey\mozilla\widget\src\windows\nsWindow.cpp, line 958]
     KERNEL32.DLL + 0x3613 (0xbff63613)  
     KERNEL32.DLL + 0x248f7 (0xbff848f7)  
     0x00688b5e  
     0x00058f64  

Comment 6

[David P. Drinan]

*** Bug 82367 has been marked as a duplicate of this bug. ***

Comment 7

[David P. Drinan]

This is PSM bug. This is caused by the fix for bug 77837. The fix for 77837 will 
be backed out now and this bug should then go away.

Comment 8

[David P. Drinan]

Assigning to ddrinan.

Comment 9

[Ferdinand]

*** Bug 82392 has been marked as a duplicate of this bug. ***

Comment 10

[Ferdinand]

*** Bug 82382 has been marked as a duplicate of this bug. ***

Comment 11

[Ferdinand]

*** Bug 82391 has been marked as a duplicate of this bug. ***

Comment 12

[Chris McAfee]

javi backed out changes for 77837, we need to test this again now
and mark fixed if it is so.

Comment 13

[Brendan Eich [:brendan]]

nsKeygenFormProcess::Init is buggy, it sorely needs super-review.  Why did it
crash?  Let's look at the method it calls three times, without bothering to
check return value:

http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSComponent.cpp#157

frees ptrv, then control flow falls into

http://lxr.mozilla.org/mozilla/source/security/manager/ssl/src/nsNSSComponent.cpp#162

and double-frees.  Plus, else-after-return non-sequitur, redundant and probably
unnecessary SetLength(0) calls that could be fused, and use of the deprecated
nsString instead of XPCOM PRUnichar** out param and nsXPIDLString in the caller.

Don't paper over these bugs by blaming 77837's patch.  It may have busted other
things, but the code that crashed here needs to be fixed.

/be

Comment 14

[David P. Drinan]

This no longer crashes after Javi backed out changes for 77837. I propose 
removing this as a blocker (so the tree can open) and leaving it open to fix the 
problems that brendan found with some of the code.

Comment 15

[Chris McAfee]

reducing to critical after talking with ddrinan.

Comment 16

[Tracy Walker [:tracy]]

this appears fixed in windows commercial build 2001-05-23-13-trunk

Comment 17

[John Unruh]

Fixed in the afternoon 5/23 WinNT build.

Comment 18

[David P. Drinan]

*** Bug 82411 has been marked as a duplicate of this bug. ***

Comment 19

[Jay Patel [:jay]]

adding topcrash keyword and Trunk [@ nsKeygenFormProcessor::Init] to summary for 
tracking.

Comment 20

[Brendan Eich [:brendan]]

Doesn't anyone read comments in bugs any more?  This was to remain open.

/be

Comment 21

[Chris McAfee]

resummarize for non-crasher, and people won't close it.

Comment 22

[David P. Drinan]

*** Bug 82538 has been marked as a duplicate of this bug. ***

Comment 23

[chris hofmann]

putting om 0.9.1 radar

Comment 24

[Brendan Eich [:brendan]]

I'll let ddrinan resummarize and keword-ize -- I'm not as in the know, all I do
know is that this bug should stay open.  The double-free I found by inspection
in a live code path sure sounds like a crash bug to me.

/be

Comment 25

[David P. Drinan]

The method nsNSSComponent::GetPIPNSSBundleString() gets called from >100 places. 
Changing it's interface now would also involve changing all these callers to use 
this new interface. I think this is too big of a change for 0.9.1. 

I propose the following: For 0.9.1, fix the double-free and open a separate bug 
for 0.9.2 to rewrite this method taking into account brendans comments and 
suggestions.

Comment 26

[David P. Drinan]

Attachment: Patch to fix double-free. Javi, please review.

Comment 27

[Javier Delgadillo]

r=javi

Comment 28

[David P. Drinan]

blizzard,
Please super-review. Thanks.

Comment 29

[Brendan Eich [:brendan]]

Certainly, fix the double-free now.  You might find that most easily done by
also unifying the SetLength(0) calls.

/be

Comment 30

[Christopher Blizzard (:blizzard)]

So, looking at that code I've noticed a couple of things:

o If you do get the ptrv return from GetStringFromName(), assign to outString (
which is an nsString, not a PRUnichar! ) and then return isn't it going to leak?
 |operator=(PRUnichar *)| of an nsString maps to |nsString::Assign| and doesn't
subsume.

[...]
 nsresult rv = mPIPNSSBundle->GetStringFromName(name, &ptrv);
    if (NS_SUCCEEDED(rv)) {
      outString = ptrv;
      return NS_OK;
    }
[...]

o You don't need the extra SetLength(0) in there.  In fact, you can simplify
this entire function a lot.  You don't need the else{} after the first if since
there's a return.  You also don't need the second else{} after the first free
that you are removing since it will be called only on an error.

Might as well clean it up while we're at it...

Comment 31

[Jay Patel [:jay]]

Talkback is showing this crash last occurring with build 2001052309.  Was this 
fixed or has it magically disappeared?

Comment 32

[Brendan Eich [:brendan]]

A necessary condition for this bug to bite was removed by back-out, but the bug
in the code remains, without talkback symptoms at this time.  It should be fixed
soon.

/be

Comment 33

[David P. Drinan]

Attachment: Updated patch. Blizzard, please sr.

Comment 34

[Christopher Blizzard (:blizzard)]

sr=blizzard

Comment 35

[Christopher Blizzard (:blizzard)]

a=blizzard for 0.9.1

Comment 36

[David P. Drinan]

*** Bug 76970 has been marked as a duplicate of this bug. ***

Comment 37

[chris hofmann]

David, is this fix checked in? If not, the 0.9.1 branch is ready. Please also
check in on the trunk.

Comment 38

[David P. Drinan]

Fix checked into to branch and trunk.

Comment 39

[John Unruh]

Verified.