41390 - Javascript's prompt() contains a "remember this value" checkbox and "User Name" in titlebar

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Stephen P. Morse]

Calling on the javascript routine prompt("hello") is supposed to bring up a 
dialog with the word "hello" and OK/Cancel buttons.  But that dialog now 
contains a meaningless checkbox with the caption "remember this value".

I'm pretty sure this regression is caused by the restructuring of the prompt 
modules that Warren did when he re-integrated ftp/http authentication with 
wallet.  So I'm assigning this bug to warren.

To demonstrate the problem, do the following:

1. From the menu select tasks->privacy->form-manager->view-stored-data
2. On the resulting dialog click the add-new button below the field-name list.
3. The prompt dialog that appears has a checkbox in it.

Comment 1

[Stephen P. Morse]

Nominating for nsbeta2 because this is a major embarrasement.

Comment 2

[Jim Roskind]

Setting to beta2 plus, will be minus if not fixed by 6/22

Comment 3

[Blake Ross]

yes, this is very annoying - furthermore, the titlebar string parameter of 
prompt() is completely ignored, and "User Name" appears instead in the 
titlebar.  This has been the cause of such bugs as bug 41542 and bug 41283

Comment 4

[Blake Ross]

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

Comment 5

[Blake Ross]

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

Comment 6

[Stephen P. Morse]

Here's a work-around that I plan on using in case this bug doesn't get fixed.  
I'm replacing my call to javascript's "prompt" with a call to a local routine 
called "myPrompt" which is the following:

function myPrompt(message, oldValue, title) {
  var newValue = { };
  if (!title) {
    title = " ";
  }
  var commonDialogService =
    Components.classes
      ["component://netscape/appshell/commonDialogs"].getService();
  commonDialogService =
    commonDialogService.QueryInterface(Components.interfaces.nsICommonDialogs);
  commonDialogService.Prompt
    (window, title, message, oldValue, newValue)
  return newValue.value;
}

Comment 7

[Blake Ross]

yep, that's nearly the same method I used to fix bug 41283.  It's a good, easy 
workaround but I still think this needs to remain nsbeta2+ because of all the 
websites out there that use the common JS prompt() method, and how embarrassing 
(and confusing) this is.

Comment 8

[Warren Harris]

First, common dialogs is really an internal thing that's slated for deprecation. 
It would be bad to use it in js code. Second, I would appreciate it if someone 
could take the time to investigate this bug. It's probably something trivial 
(calling the wrong entry point in common dialogs from webshell window). I just 
haven't had time to look into it.

Comment 9

[Blake Ross]

Point me where to look and I will . . .

Comment 10

[Stephen P. Morse]

Warren, it's going to be pretty hard for anyone else to look at this if common 
dialogs is going away, since we don't know what it's going to be replaced with.  
This dialog with the checkbox is in fact coming from common dialogs.  The 
problem is that the javascript call is being routed through single signon (due 
to the recent restructuring that you did) before getting to common dialogs which 
is why the checkbox is getting added.  So whether javascript calls common 
dialogs directly as I was proposing, or does so indirectly via single signon as 
is done now, the end result is a commonDialogs dialog.

Can you elaborate on the replacement for common dialogs.

Comment 11

[Warren Harris]

Here's what I see going on: JS is calling WindowPrompt which calls 
nsIPrompt::Prompt (as it should). This calls single sign-on (which it should). 
However, somewhere along the way, si_CheckGetData is calling 
nsNetSupportDialog::UniversalDialog rather than calling through to the 
underlying nsIPrompt instance. This is wrong. We need to eliminate 
kNetSupportDialogCID (and probably the use of UniversalDialog from single 
sign-on). (Note that I don't think this was related to my SI changes.)

nsDOMWindowPrompter::UniversalDialog(nsDOMWindowPrompter * const 0x02d1f920, 
const unsigned short * 0x00000000, const unsigned short * 0x031d7b60, const 
unsigned short * 0x0012e514, const unsigned short * 0x031d7d60, const unsigned 
short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 
0x00000000, const unsigned short * 0x00000000, ...) line 1933
nsSingleSignOnPrompt::UniversalDialog(nsSingleSignOnPrompt * const 0x02d1fb90, 
const unsigned short * 0x00000000, const unsigned short * 0x031d7b60, const 
unsigned short * 0x0012e514, const unsigned short * 0x031d7d60, const unsigned 
short * 0x00000000, const unsigned short * 0x00000000, const unsigned short * 
0x00000000, const unsigned short * 0x00000000, ...) line 485
nsNetSupportDialog::UniversalDialog(nsNetSupportDialog * const 0x02d17f60, const 
unsigned short * 0x00000000, const unsigned short * 0x031d7b60, const unsigned 
short * 0x0012e514, const unsigned short * 0x031d7d60, const unsigned short * 
0x00000000, const unsigned short * 0x00000000, const unsigned short * 
0x00000000, const unsigned short * 0x00000000, const unsigned short * ...) l
si_CheckGetData(unsigned short * * 0x0012e4f4, const unsigned short * 
0x0012e514, int * 0x0012e258) line 452 + 71 bytes
SINGSIGN_Prompt(const unsigned short * 0x00000000, const unsigned short * 
0x0012e514, const unsigned short * 0x0012e474, unsigned short * * 0x0012e4f4, 
const char * 0x0012e3d0, nsIPrompt * 0x02d7a970, int * 0x0012e598) line 2298 + 
20 bytes
nsSingleSignOnPrompt::Prompt(nsSingleSignOnPrompt * const 0x02d7b6d0, const 
unsigned short * 0x00000000, const unsigned short * 0x0012e514, const unsigned 
short * 0x00000000, const unsigned short * 0x0012e474, unsigned short * * 
0x0012e4f4, int * 0x0012e598) line 435 + 47 bytes
GlobalWindowImpl::Prompt(GlobalWindowImpl * const 0x02d0a644, JSContext * 
0x02d0a450, long * 0x028f7d04, unsigned int 0x00000001, long * 0x0012e5c4) line 
1231 + 76 bytes
WindowPrompt(JSContext * 0x02d0a450, JSObject * 0x027894e0, unsigned int 
0x00000001, long * 0x028f7d04, long * 0x0012e674) line 1182 + 31 bytes
js_Invoke(JSContext * 0x02d0a450, unsigned int 0x00000001, unsigned int 
0x00000000) line 686 + 23 bytes
js_Interpret(JSContext * 0x02d0a450, long * 0x0012f02c) line 2490 + 15 bytes
js_Execute(JSContext * 0x02d0a450, JSObject * 0x027894e0, JSScript * 0x031d7ed0, 
JSFunction * 0x00000000, JSStackFrame * 0x00000000, unsigned int 0x00000000, 
long * 0x0012f02c) line 857 + 13 bytes
JS_EvaluateUCScriptForPrincipals(JSContext * 0x02d0a450, JSObject * 0x027894e0, 
JSPrincipals * 0x031d2040, const unsigned short * 0x0012f664, unsigned int 
0x00000014, const char * 0x031d7fd0, unsigned int 0x00000001, long * 0x0012f02c) 
line 2754 + 27 bytes
nsJSContext::EvaluateString(nsJSContext * const 0x02d0a5e0, const nsString & {"
prompt("Hello");
"}, void * 0x027894e0, nsIPrincipal * 0x031d203c, const char * 0x031d7fd0, 
unsigned int 0x00000001, const char * 0x00000000, nsString & {""}, int * 
0x0012f088) line 464 + 55 bytes
HTMLContentSink::EvaluateScript(nsString & {"
prompt("Hello");
"}, nsIURI * 0x03166990, int 0x00000001, const char * 0x00000000) line 4342
HTMLContentSink::ProcessSCRIPTTag(const nsIParserNode & {...}) line 4674
HTMLContentSink::AddLeaf(HTMLContentSink * const 0x031e8570, const nsIParserNode 
& {...}) line 2970 + 12 bytes
CNavDTD::AddLeaf(const nsIParserNode * 0x020f15c0) line 3493 + 22 bytes
CNavDTD::AddHeadLeaf(nsIParserNode * 0x020f15c0) line 3612 + 17 bytes
CNavDTD::HandleStartToken(CToken * 0x02d67c80) line 1560 + 12 bytes
CNavDTD::HandleToken(CNavDTD * const 0x031d5140, CToken * 0x022fa140, nsIParser 
* 0x031eb7a0) line 770 + 12 bytes
CNavDTD::BuildModel(CNavDTD * const 0x031d5140, nsIParser * 0x031eb7a0, 
nsITokenizer * 0x031d2700, nsITokenObserver * 0x00000000, nsIContentSink * 
0x031e8570) line 499 + 20 bytes
nsParser::BuildModel() line 1646 + 34 bytes
nsParser::ResumeParse(int 0x00000001, int 0x00000000) line 1527 + 11 bytes
nsParser::OnDataAvailable(nsParser * const 0x031eb7a8, nsIChannel * 0x031f28a0, 
nsISupports * 0x00000000, nsIInputStream * 0x031eec9c, unsigned int 0x00000000, 
unsigned int 0x00000035) line 1975 + 19 bytes
nsDocumentOpenInfo::OnDataAvailable(nsDocumentOpenInfo * const 0x031f5c60, 
nsIChannel * 0x031f28a0, nsISupports * 0x00000000, nsIInputStream * 0x031eec9c, 
unsigned int 0x00000000, unsigned int 0x00000035) line 210 + 46 bytes
nsFileChannel::OnDataAvailable(nsFileChannel * const 0x031f28a8, nsIChannel * 
0x031ef5e0, nsISupports * 0x00000000, nsIInputStream * 0x031eec9c, unsigned int 
0x00000000, unsigned int 0x00000035) line 661 + 49 bytes
nsOnDataAvailableEvent::HandleEvent(nsOnDataAvailableEvent * const 0x031ee9f0) 
line 404 + 47 bytes
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x031ee9a0) line 97 + 12 bytes
PL_HandleEvent(PLEvent * 0x031ee9a0) line 575 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x01060990) line 520 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x014809c0, unsigned int 0x0000c0cd, unsigned int 
0x00000000, long 0x01060990) line 1032 + 9 bytes
USER32! 77e71820()

Comment 12

[Stephen P. Morse]

You are right, the fact that single signon calls universal dialog is not related 
to your si change.  What is related to your change is that everyone now comes 
through single signon whereas before they didn't.

The correct thing to do, as we just discussed on the phone, is to have an 
additional parameter to the prompt call that determines whether or not si should 
put up the checkbox.  You proposed combining that with a persistence parameter 
(making it a trinary instead of a binary) and that would be fine.

Comment 13

[Stephen P. Morse]

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

Comment 14

[Warren Harris]

Ok, here's the diffs for this for your review. I'm cc'ing Brendan and Vidur 
because I changed the semantics of javascript's 'prompt' method to allow 2 
additional default parameters: 

   prompt(<message>, <default-value>, <dialog-title>, <save-value>)

where save-value is one of the new enums added to nsIPrompt:

    /**
     * Values for the savePassword parameter to prompt, promptPassword and
     * promptUsernameAndPassword.
     */
    const unsigned long SAVE_PASSWORD_NEVER             = 0;
    const unsigned long SAVE_PASSWORD_FOR_SESSION       = 1;
    const unsigned long SAVE_PASSWORD_PERMANENTLY       = 2;

The title defaults to null, which eventually defaults to "Prompt". The enum 
defaults to SAVE_PASSWORD_NEVER (like before -- this causes the checkbox to not 
be present).

Here's a list of what I did here:

1. Changed nsIPrompt's persistPassword bool to a tri-state value (above).

2. Added savePassword arg to nsIPrompt::Prompt.

3. Made single sign-on use the underlying nsIPrompt implementation that it's 
passed instead of getting the global kNetSupportDialogCID service. This allows 
all dialogs to be matched with an initiating window, but had one side effect, 
(#4, below)...

4. Had to add an nsIPrompt argument to single sign-on's HaveData method because 
somewhere deep down in the code it can prompt the user (SINGSIGN_HaveData -> 
si_RestoreOldSignonDataFromBrowser -> si_GetUser -> si_SelectDialog -> 
nsIPrompt::Select). (Maybe Steve can suggest something better -- maybe in this 
code path, prompting can't happen.)

5. Made it so that the dialog title and default value can be passed all the way 
through single sign-on code.

6. Changed Prompt's default title to just be "Prompt" (was "User Name"). 
(There's probably somewhere else in the code where someone is using Prompt and 
should be passing "User Name" instead of null, but I don't know where that is.)

7. Did NOT change wallet.cpp to be passed an underlying nsIPrompt -- it still 
gets the global kNetSupportDialogCID service in a couple of places. (Steve 
should probably fix this someday.)

8. Added new default args to javascript's 'prompt' method (see above).

9. Added nsJSUtils::nsConvertJSValToUint32 for convenience.

Please review, and I'll check it in.

Comment 15

[Warren Harris]

Attachment: diffs described in last comment

Comment 16

[Stephen P. Morse]

Looks ok to me.  r=morse

Comment 17

[Blake Ross]

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

Comment 18

[Brendan Eich [:brendan]]

Warren, can you see that this enhancement to prompt (a very ancient DOM level 0 
window method) gets documented?

Nit: use PRUint32 in the IDL, it matches up with the C++ better (unsigned long 
in C or C++ is not a portably-usable type).

/be

Comment 19

[Warren Harris]

Checked in.

Comment 20

[sairuh (rarely reading bugmail)]

phil, is this your realm? don't think it's mine.

Comment 21

[Blake Ross]

prompts look fine with 62108 winME/win2k.  waiting on linux and mac 
verification...

Comment 22

[Phil Schwartau]

Verified on WinNT, Linux and Mac, using:

Mozilla tip build 2000-07-18 on WinNT
Mozilla tip build 2000-07-19 onLinux.
Mozilla build ID 2000070511 on MacOS9.