278019 - FF10 crash [@ nsPasswordManager::Notify]

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[Jay Patel [:jay]]

This is a topcrasher with Firefox 1.0 and although the user comments and urls
vary, this seems to be a consistent crash according to the stack traces.  

A few of the comments mention going from a secure site to an unencrypted page
and I have tried a few of the cases, but have not seen any problems.

Here is a Talkback query for all nsPasswordManager::Notify crashes:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=+nsPasswordManager%3A%3ANotify&vendor=All&product=All&platform=All&buildid=&sdate=&stime=&edate=&etime=&sortby=bbid

And a recent incident with stack:
Incident ID: 2981472
Stack Signature	nsPasswordManager::Notify 57c26fff
Product ID	Firefox10
Build ID	2004110711
Trigger Time	2005-01-10 08:21:19.0
Platform	Win32
Operating System	Windows NT 5.1 build 2600
Module	firefox.exe + (003ea98c)
URL visited	fidelity.com
User Comments	in going from a secure Fidelity site to a non-secure Fidelity
site, it asks if it's ok, when I click OK, Firefox "must close". This has
happened several times. If I remember and click Cancel, it seems to work fine.
Since Last Crash	460247 sec
Total Uptime	532203 sec
Trigger Reason	Access violation
Source File, Line No.
d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/toolkit/components/passwordmgr/base/nsPasswordManager.cpp,
line 1001
Stack Trace 	
nsPasswordManager::Notify 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/toolkit/components/passwordmgr/base/nsPasswordManager.cpp,
line 1001]
nsHTMLFormElement::NotifySubmitObservers 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLFormElement.cpp,
line 1029]
nsHTMLFormElement::SubmitSubmission 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLFormElement.cpp,
line 942]
nsHTMLFormElement::DoSubmit 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLFormElement.cpp,
line 870]
nsHTMLFormElement::DoSubmitOrReset 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLFormElement.cpp,
line 797]
nsHTMLFormElement::HandleDOMEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLFormElement.cpp,
line 756]
PresShell::HandleDOMEventWithTarget 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6139]
nsHTMLInputElement::HandleDOMEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLInputElement.cpp,
line 1612]
PresShell::HandleDOMEventWithTarget 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6139]
nsHTMLInputElement::MaybeSubmitForm 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLInputElement.cpp,
line 991]
nsHTMLInputElement::HandleDOMEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/content/html/content/src/nsHTMLInputElement.cpp,
line 1539]
PresShell::HandleEventInternal 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 6059]
PresShell::HandleEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/layout/html/base/src/nsPresShell.cpp,
line 5921]
nsViewManager::HandleEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2280]
nsViewManager::DispatchEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/view/src/nsViewManager.cpp,
line 2066]
HandleEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/view/src/nsView.cpp,
line 77]
nsWindow::DispatchEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1067]
nsWindow::DispatchKeyEvent 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 2978]
nsWindow::OnChar 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 3162]
nsWindow::ProcessMessage 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 3878]
nsWindow::WindowProc 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsWindow.cpp,
line 1349]
USER32.dll + 0x8709 (0x77d48709)
USER32.dll + 0x87eb (0x77d487eb)
USER32.dll + 0x89a5 (0x77d489a5)
USER32.dll + 0x89e8 (0x77d489e8)
nsAppShell::Run 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/widget/src/windows/nsAppShell.cpp,
line 159]
nsAppShellService::Run 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/xpfe/appshell/src/nsAppShellService.cpp,
line 495]
main 
[d:/builds/tinderbox/firefox-1.0/WINNT_5.0_Clobber/mozilla/browser/app/nsBrowserApp.cpp,
line 58]
kernel32.dll + 0x16d4f (0x7c816d4f)

Comment 1

[Daniel Veditz [:dveditz]]

nominating topcrash

Comment 2

[Mike Shaver (:shaver emeritus)]

Attachment: Use GetOwnerDoc instead of GetDocument, to avoid crashing if the document has been torn down before we get here.

Comment 3

[Daniel Veditz [:dveditz]]

blocking 1.0.1.  Is this needed for the 1.7 branch?

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 173401 [details] [diff] [review]
Use GetOwnerDoc instead of GetDocument, to avoid crashing if the document has been torn down before we get here.

r=jst

Comment 5

[Brian Ryner (not reading)]

(In reply to comment #2)
> Created an attachment (id=173401) [edit]
> Use GetOwnerDoc instead of GetDocument, to avoid crashing if the document has
> been torn down before we get here.
> 

1. Going on the theory that the secure -> insecure form submission dialog causes
the document to be torn down prematurely... please file a bug on fixing that
underlying problem.

2. I think I'd rather have this use GetCurrentDoc() and null-check the result. 
That will cause form data capture for this case to fail until (1) is fixed.  But
it seems more correct than capturing data for a form that was perhaps
constructed via script and never inserted into the document.  For now, it may be
more important to get this working without a fix for (1), so I'd be ok with this
patch and a comment referencing the bug about the premature teardown.

Comment 6

[Johnny Stenback (:jst)]

Attachment: Same thing for the 1.0.1 branch

I initially thought we'd need to do this through the DOM interfaces on the
branch (since there is no GetOwnerDoc() method in nsIContent there, but after
looking at the code it seems safe to assume that aFormNode is an element, and
then it's safe to get to the owner document through the elements nodeinfo
(which is what GetOwnerDoc() does too). So that's what this patch does...

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 173675 [details] [diff] [review]
Same thing for the 1.0.1 branch

Thanks!

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 173675 [details] [diff] [review]
Same thing for the 1.0.1 branch

a=dveditz for the 1.0.1 branch.

Comment 9

[Johnny Stenback (:jst)]

Fix landed on the 1.0.1 branch.

Comment 10

[Yusuf Goolamabbas]

shaver, has this landed on the trunk ?

Comment 11

[Mike Shaver (:shaver emeritus)]

yusufg: sure looks like it from
http://lxr.mozilla.org/mozilla/source/toolkit/components/passwordmgr/base/nsPasswordManager.cpp#998

Comment 12

[Jay Patel [:jay]]

Verified Fixed based on latest Firefox 1.0.1 Talkback data.