Closed
Bug 82359
Opened 23 years ago
Closed 23 years ago
Crash when hitting "Submit query" in Bugzilla - Trunk [@ nsKeygenFormProcessor::Init]
Categories
(Core Graveyard :: Security: UI, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.1
People
(Reporter: coen, Assigned: ddrinan0264)
References
()
Details
(4 keywords, Whiteboard: wanted for 0.9.1 has r= and sr= Need a=)
Crash Data
Attachments
(2 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
OS: win98
Build: 2001052304 talkback installer
Reproducible: Always
Submitting other forms also crash. (IE google.com, altavista.com...)
Confirmed on my WinMe system w/ 2001052304
Talkback ID's:
TB30811255W
TB30811227Q
TB30810859W
Comment 5•23 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 7•23 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
Comment 10•23 years ago
|
||
*** Bug 82382 has been marked as a duplicate of this bug. ***
Comment 11•23 years ago
|
||
*** Bug 82391 has been marked as a duplicate of this bug. ***
Updated•23 years ago
|
Summary: Crash when hiting "Submit query" → Crash when hiting "Submit query" in Bugzilla
Updated•23 years ago
|
Comment 12•23 years ago
|
||
javi backed out changes for 77837, we need to test this again now
and mark fixed if it is so.
Comment 13•23 years ago
|
||
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•23 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•23 years ago
|
||
reducing to critical after talking with ddrinan.
Severity: blocker → critical
Comment 16•23 years ago
|
||
this appears fixed in windows commercial build 2001-05-23-13-trunk
Comment 17•23 years ago
|
||
Fixed in the afternoon 5/23 WinNT build.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 18•23 years ago
|
||
*** Bug 82411 has been marked as a duplicate of this bug. ***
Comment 19•23 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]
Comment 20•23 years ago
|
||
Doesn't anyone read comments in bugs any more? This was to remain open.
/be
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 21•23 years ago
|
||
resummarize for non-crasher, and people won't close it.
Assignee | ||
Comment 22•23 years ago
|
||
*** Bug 82538 has been marked as a duplicate of this bug. ***
Comment 24•23 years ago
|
||
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•23 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•23 years ago
|
||
Comment 27•23 years ago
|
||
r=javi
Assignee | ||
Comment 28•23 years ago
|
||
blizzard,
Please super-review. Thanks.
Comment 29•23 years ago
|
||
Certainly, fix the double-free now. You might find that most easily done by
also unifying the SetLength(0) calls.
/be
Comment 30•23 years ago
|
||
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•23 years ago
|
||
Talkback is showing this crash last occurring with build 2001052309. Was this
fixed or has it magically disappeared?
Comment 32•23 years ago
|
||
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•23 years ago
|
Whiteboard: Working on revised patch. ETA 5/30.
Assignee | ||
Comment 33•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Whiteboard: Working on revised patch. ETA 5/30. → Waiting for sr.
Updated•23 years ago
|
Whiteboard: Waiting for sr. → wanted for 0.9.1 has r= Waiting for sr= and a=
Comment 34•23 years ago
|
||
sr=blizzard
Updated•23 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=
Comment 35•23 years ago
|
||
a=blizzard for 0.9.1
Updated•23 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•23 years ago
|
||
*** Bug 76970 has been marked as a duplicate of this bug. ***
Comment 37•23 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•23 years ago
|
||
Fix checked into to branch and trunk.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Crash Signature: [@ nsKeygenFormProcessor::Init]
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•