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

VERIFIED FIXED in mozilla0.9.1

Status

Core Graveyard
Security: UI
--
critical
VERIFIED FIXED
17 years ago
a year ago

People

(Reporter: coen, Assigned: David P. Drinan)

Tracking

(4 keywords)

1.0 Branch
mozilla0.9.1
x86
Windows 98
crash, regression, smoketest, topcrash

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: wanted for 0.9.1 has r= and sr= Need a=, crash signature, URL)

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
OS: win98
Build: 2001052304 talkback installer
Reproducible: Always
(Reporter)

Comment 1

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

Comment 2

17 years ago
Confirmed on my WinMe system w/ 2001052304

Talkback ID's:
TB30811255W
TB30811227Q
TB30810859W
Severity: critical → blocker
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash

Comment 3

17 years ago
kewords
Keywords: regression, smoketest
(Reporter)

Comment 4

17 years ago
Build 2001052206 don't show this problem

Comment 5

17 years ago
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  
(Assignee)

Comment 6

17 years ago
*** Bug 82367 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 7

17 years ago
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.
Component: Form Submission → Client Library
Product: Browser → PSM
Target Milestone: --- → 2.0
Version: other → 2.0
(Assignee)

Comment 8

17 years ago
Assigning to ddrinan.
Assignee: rods → ddrinan

Updated

17 years ago
Depends on: 77873

Comment 9

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

Comment 10

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

Comment 11

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

Updated

17 years ago
Summary: Crash when hiting "Submit query" → Crash when hiting "Submit query" in Bugzilla

Updated

17 years ago
Depends on: 77837
No longer depends on: 77873

Comment 12

17 years ago
javi backed out changes for 77837, we need to test this again now
and mark fixed if it is so.
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
(Assignee)

Comment 14

17 years ago
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

17 years ago
reducing to critical after talking with ddrinan.
Severity: blocker → critical
this appears fixed in windows commercial build 2001-05-23-13-trunk

Comment 17

17 years ago
Fixed in the afternoon 5/23 WinNT build.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

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

Comment 19

17 years ago
adding topcrash keyword and Trunk [@ nsKeygenFormProcessor::Init] to summary for 
tracking.
Keywords: topcrash
Summary: Crash when hiting "Submit query" in Bugzilla → Crash when hiting "Submit query" in Bugzilla - Trunk [@ nsKeygenFormProcessor::Init]
Doesn't anyone read comments in bugs any more?  This was to remain open.

/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 21

17 years ago
resummarize for non-crasher, and people won't close it.
(Assignee)

Comment 22

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

Comment 23

17 years ago
putting om 0.9.1 radar
Target Milestone: 2.0 → mozilla0.9.1
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
(Assignee)

Comment 25

17 years ago
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.
(Assignee)

Comment 26

17 years ago
Created attachment 36026 [details] [diff] [review]
Patch to fix double-free. Javi, please review.

Comment 27

17 years ago
r=javi
(Assignee)

Comment 28

17 years ago
blizzard,
Please super-review. Thanks.
Certainly, fix the double-free now.  You might find that most easily done by
also unifying the SetLength(0) calls.

/be
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

17 years ago
Talkback is showing this crash last occurring with build 2001052309.  Was this 
fixed or has it magically disappeared?
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
(Assignee)

Updated

17 years ago
Whiteboard: Working on revised patch. ETA 5/30.
(Assignee)

Comment 33

17 years ago
Created attachment 36458 [details] [diff] [review]
Updated patch. Blizzard, please sr.
(Assignee)

Updated

17 years ago
Whiteboard: Working on revised patch. ETA 5/30. → Waiting for sr.

Updated

17 years ago
Whiteboard: Waiting for sr. → wanted for 0.9.1 has r= Waiting for sr= and a=
sr=blizzard

Updated

17 years ago
Whiteboard: wanted for 0.9.1 has r= Waiting for sr= and a= → wanted for 0.9.1 has r= and sr= Need a=
a=blizzard for 0.9.1

Updated

17 years ago
Summary: Crash when hiting "Submit query" in Bugzilla - Trunk [@ nsKeygenFormProcessor::Init] → Crash when hitting "Submit query" in Bugzilla - Trunk [@ nsKeygenFormProcessor::Init]
(Assignee)

Comment 36

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

Comment 37

17 years ago
David, is this fix checked in? If not, the 0.9.1 branch is ready. Please also
check in on the trunk.
(Assignee)

Comment 38

17 years ago
Fix checked into to branch and trunk.
Status: REOPENED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 39

17 years ago
Verified.
Status: RESOLVED → VERIFIED

Updated

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

Updated

10 years ago
Version: psm2.0 → 1.0 Branch
Crash Signature: [@ nsKeygenFormProcessor::Init]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.