51568 - WinNT-Encrypting data w/o db password locks up browser

Status: VERIFIED FIXED

(SeaMonkey :: Passwords & Permissions, defect, P2)

Description

[John Unruh]

1.) With a new profile, visit mail.yahoo.com and enter a username and password.
2.) Say yes to save the data.
3.) Encrypt the data.
What happens: A large grey blank box appears, and the browser is locked up. 
Using PSM 1.3 with the 9/6 WinNT build.

Comment 1

[nitinp]

adding keyword nsbeta3 and nominating for nsbeta3+ . this was working a while 
ago and it is back . Must fix for PR3.
Also ccing thayes and drrinan.

Comment 2

[Stephen P. Morse]

I'm not seeing the gray box but I am seeing the hang.  It's in the psm module.  
Reassigning to ddrinan.

NTDLL! 77f678ff()
WS2_32! 776b84ee()
WSOCK32! 776d1173()
_PR_MD_RECV(PRFileDesc * 0x03bb8830, void * 0x0012d3f0, int 8, int 0, unsigned 
int 4294967295) line 175 + 19 bytes
SocketRecv(PRFileDesc * 0x03bb8830, void * 0x0012d3f0, int 8, int 0, unsigned 
int 4294967295) line 558 + 25 bytes
PR_Recv(PRFileDesc * 0x03bb8830, void * 0x0012d3f0, int 8, int 0, unsigned int 
4294967295) line 193 + 28 bytes
nsPSMShimReceive(void * 0x03c5bdd0, void * 0x0012d3f0, unsigned int 8) line 222 
+ 29 bytes
CMT_ReadThisMany(_CMT_CONTROL * 0x03c5f080, void * 0x03c5bdd0, void * 
0x0012d3f0, unsigned long 8) line 401 + 24 bytes
CMT_ReceiveMessage(_CMT_CONTROL * 0x03c5f080, CMTItemStr * 0x0012d43c) line 363 
+ 21 bytes
CMT_ReadMessageDispatchEvents(_CMT_CONTROL * 0x03c5f080, CMTItemStr * 
0x0012d43c) line 261 + 13 bytes
CMT_SendMessage(_CMT_CONTROL * 0x03c5f080, CMTItemStr * 0x0012d43c) line 312 + 
13 bytes
CMT_Hello(_CMT_CONTROL * 0x03c5f080, unsigned long 81, char * 0x0012dd98, char * 
0x03c5a548) line 408 + 13 bytes
nsPSMComponent::GetControlConnection(nsPSMComponent * const 0x0347d3b0, 
_CMT_CONTROL * * 0x0012de10) line 589 + 34 bytes
nsSecretDecoderRing::Logout(nsSecretDecoderRing * const 0x03c4f330) line 230 + 
22 bytes
wallet_ReencryptAll(const char * 0x03c51800, void * 0x02f8ae34) line 2963 + 17 
bytes
pref_DoCallback(const char * 0x03c51800) line 2238 + 17 bytes
pref_HashPref(const char * 0x03c51800, PrefValue {...}, int 128, int 1) line 
1808 + 9 bytes
PREF_SetBoolPref(const char * 0x03c51800, int 1) line 752 + 20 bytes
nsPref::SetBoolPref(nsPref * const 0x00b8ecc0, const char * 0x03c51800, int 1) 
line 771 + 13 bytes
XPTC_InvokeByIndex(nsISupports * 0x00b8ecc0, unsigned int 18, unsigned int 2, 
nsXPTCVariant * 0x0012e0e8) line 139
nsXPCWrappedNativeClass::CallWrappedMethod(JSContext * 0x026c7790, 
nsXPCWrappedNative * 0x02683e80, const XPCNativeMemberDescriptor * 0x00d51154, 
nsXPCWrappedNativeClass::CallMode CALL_METHOD, unsigned int 2, long * 
0x025a5f98, long * 0x0012e298) line 915 + 43 bytes
WrappedNative_CallMethod(JSContext * 0x026c7790, JSObject * 0x00d68610, unsigned 
int 2, long * 0x025a5f98, long * 0x0012e298) line 226 + 34 bytes
js_Invoke(JSContext * 0x026c7790, unsigned int 2, unsigned int 0) line 731 + 23 
bytes
js_Interpret(JSContext * 0x026c7790, long * 0x0012ec20) line 2538 + 15 bytes
js_Invoke(JSContext * 0x026c7790, unsigned int 1, unsigned int 2) line 748 + 13 
bytes
js_InternalInvoke(JSContext * 0x026c7790, JSObject * 0x02622a58, long 39987904, 
unsigned int 0, unsigned int 1, long * 0x0012edb4, long * 0x0012ed44) line 821 + 
19 bytes
JS_CallFunctionValue(JSContext * 0x026c7790, JSObject * 0x02622a58, long 
39987904, unsigned int 1, long * 0x0012edb4, long * 0x0012ed44) line 3175 + 31 
bytes
nsJSContext::CallEventHandler(nsJSContext * const 0x026ba0a0, void * 0x02622a58, 
void * 0x02622ac0, unsigned int 1, void * 0x0012edb4, int * 0x0012edb0, int 0) 
line 902 + 33 bytes
nsJSEventListener::HandleEvent(nsIDOMEvent * 0x03c40fb4) line 154 + 64 bytes
nsEventListenerManager::HandleEventSubType(nsListenerStruct * 0x02b266d0, 
nsIDOMEvent * 0x03c40fb4, nsIDOMEventTarget * 0x02b26b6c, unsigned int 8, 
unsigned int 7) line 788 + 19 bytes
nsEventListenerManager::HandleEvent(nsIPresContext * 0x027bd260, nsEvent * 
0x0012f464, nsIDOMEvent * * 0x0012f3f4, nsIDOMEventTarget * 0x02b26b6c, unsigned 
int 7, nsEventStatus * 0x0012f4ac) line 1666 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x02b26b60, nsIPresContext * 
0x027bd260, nsEvent * 0x0012f464, nsIDOMEvent * * 0x0012f3f4, unsigned int 1, 
nsEventStatus * 0x0012f4ac) line 3257
PresShell::HandleDOMEventWithTarget(PresShell * const 0x027be520, nsIContent * 
0x02b26b60, nsEvent * 0x0012f464, nsEventStatus * 0x0012f4ac) line 4087 + 39 
bytes
nsMenuFrame::Execute() line 1498
nsMenuFrame::HandleEvent(nsMenuFrame * const 0x02518ba0, nsIPresContext * 
0x027bd260, nsGUIEvent * 0x0012f894, nsEventStatus * 0x0012f784) line 378
PresShell::HandleEventInternal(nsEvent * 0x0012f894, nsIView * 0x03c611c0, 
nsEventStatus * 0x0012f784) line 4055 + 38 bytes
PresShell::HandleEvent(PresShell * const 0x027be524, nsIView * 0x03c611c0, 
nsGUIEvent * 0x0012f894, nsEventStatus * 0x0012f784, int 0, int & 1) line 3975 + 
23 bytes
nsView::HandleEvent(nsView * const 0x03c611c0, nsGUIEvent * 0x0012f894, unsigned 
int 8, nsEventStatus * 0x0012f784, int 0, int & 1) line 379
nsView::HandleEvent(nsView * const 0x03c618e0, nsGUIEvent * 0x0012f894, unsigned 
int 8, nsEventStatus * 0x0012f784, int 0, int & 1) line 352
nsView::HandleEvent(nsView * const 0x03c4f560, nsGUIEvent * 0x0012f894, unsigned 
int 8, nsEventStatus * 0x0012f784, int 0, int & 1) line 352
nsView::HandleEvent(nsView * const 0x027bebb0, nsGUIEvent * 0x0012f894, unsigned 
int 28, nsEventStatus * 0x0012f784, int 1, int & 1) line 352
nsViewManager2::DispatchEvent(nsViewManager2 * const 0x027bed90, nsGUIEvent * 
0x0012f894, nsEventStatus * 0x0012f784) line 1429
HandleEvent(nsGUIEvent * 0x0012f894) line 68
nsWindow::DispatchEvent(nsWindow * const 0x03c617a4, nsGUIEvent * 0x0012f894, 
nsEventStatus & nsEventStatus_eIgnore) line 614 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012f894) line 635
nsWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 3811 + 
21 bytes
ChildWindow::DispatchMouseEvent(unsigned int 301, nsPoint * 0x00000000) line 
4021
nsWindow::ProcessMessage(unsigned int 514, unsigned int 0, long 4390943, long * 
0x0012fc10) line 2889 + 24 bytes
nsWindow::WindowProc(HWND__ * 0x01b6071e, unsigned int 514, unsigned int 0, long 
4390943) line 883 + 27 bytes
USER32! 77e71268()
MOZILLA! _except_list + 106527 bytes

Comment 3

[John Unruh]

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

Comment 4

[John Unruh]

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

Comment 5

[Terry Hayes]

I suspect that the original bug, and the one reported by Steve are different.  
It the original report, a "grey box" appears.  This is probably the window that 
should be filled in with the data from the "set password" URL.  In the past, 
things have stopped here due to problems with threads and modal operation of 
windows.  Has anything changed in the operation of modal windows?

Steve's stack trace shows the client trying to establish an initial connection 
to PSM. It has not yet issued an SDR decryption operation, and there is no 
password required during this step of the operation.  I suspect that PSM is not 
starting correctly in this case, probably due to an installation problem.

Comment 6

[nitinp]

This is happening with today morning's build on windows2000 too. I will check 
linux when I pick the build later in the day.

Comment 7

[John Unruh]

Since this is nsbeta3+, setting priority to P2

Comment 8

[nitinp]

still happens for the 09/18 10:43pm build. It happens on win2000, win98

Comment 9

[Terry Hayes]

The behavior of this indicates that the UI window is not getting displayed by 
the client.  We rely on processing of events in "modal" windows to allow 
operations to continue.  If the events don't get processed, then things will 
hang.  I suspect that somethings was changed in this area.  Reassigning to 
valeski to review/comment.

Comment 10

[Stephen P. Morse]

I was able to reproduce this using this morning's build (September 19).   And 
this time I indeed saw the large grey box exactly as described in the desciption 
section of this report.  I did a break and got a stack trace -- it's exactly the 
same stack trace that I included above.

So there are not two bugs here, only one.

Comment 11

[selmer (gone)]

pdt agrees p2.

Comment 12

[Judson Valeski]

not mine.

Comment 13

[Javier Delgadillo]

The following patch fixes the problem:

D:\moz-client\mozilla\extensions\psm-glue\src>cvs diff -c nsPSMUICallbacks.cpp
Index: nsPSMUICallbacks.cpp
===================================================================
RCS file: /cvsroot/mozilla/extensions/psm-glue/src/nsPSMUICallbacks.cpp,v
retrieving revision 1.22
diff -c -r1.22 nsPSMUICallbacks.cpp
*** nsPSMUICallbacks.cpp        2000/09/13 23:51:12     1.22
--- nsPSMUICallbacks.cpp        2000/09/20 22:17:56
***************
*** 102,111 ****

      // Set up arguments for "window.open"
      void *stackPtr;
!     char params[36];

      if (modal) {  // if you change this, remember to change the buffer size ab
ove.
!           strcpy(params, "menubar=no,height=%d,width=%d,dependent");
      } else {
                strcpy(params, "menubar=no,height=%d,width=%d");
        }
--- 102,113 ----

      // Set up arguments for "window.open"
      void *stackPtr;
!     char params[128];

      if (modal) {  // if you change this, remember to change the buffer size ab
ove.
!       // Do not modify this string without contacting the security team
!       // either ddrina or javi. (PSM team)
!           strcpy(params, "menubar=no,height=%d,width=%d,dependent,modal");
      } else {
                strcpy(params, "menubar=no,height=%d,width=%d");
        }

Comment 14

[Brendan Eich [:brendan]]

javi, please attach diff -u formatted patches to the bug (don't insert them as
part of a comment) next time.

The params buffer is unnecessary, and a buffer overrun accident waiting to
happen (next time someone adds a window.open option substring but forgets to
check the 128 size).  I'm attaching a better patch, which also fixes some
indentation issues and expands tabs in the area patched.

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: my proposed fix

Comment 16

[Javier Delgadillo]

I just applied brendan's patch.  Everything works just fine.

We'd like to keep the comment scaring people from modifying the format string to
prevent this bug from rearing its ugly head again in the future.

Comment 17

[Brendan Eich [:brendan]]

The "if you change this..." comment referred to the defunct params array.  If
you write a similar comment about the 256-byte buffer, cool -- only the bad
effect then would be truncation of the window options string, not buffer
overrun.  We could just use PR_smprintf and the matching PR_ free function, but
maybe that's overkill.  Given %d format specifiers, there's no way the formatted
string could be longer by 20 or so chars than the format string constant.

a=brendan on my patch plus any appropriate warning comment.

/be

Comment 18

[Javier Delgadillo]

Fix checked in

Comment 19

[John Unruh]

Verified with the 9/22 WinNT commercial build.