408791 - nsLoginManagerPrompter.js does not work with TestGtkEmbed prompt.

Status: RESOLVED INVALID

(Toolkit :: Password Manager, defect)

Description

[Oleg Romashin (:romaxa)]

Download and extract latest xulrunner:
http://ftp.mozilla.org/pub/mozilla.org/xulrunner/nightly/latest-trunk/xulrunner-1.9b2pre.en-US.linux-i686.tar.bz2

Build TestGtkEmbed binary

echo 'user_pref("signon.debug", true); ' > ~/.TestGtkEmbed/TestGtkEmbed/user.js
./run-mozilla.sh ./TestGtkEmbed https://projects.maemo.org/bugzilla

EXPECTED OUTCOME:
Prompt dialog for entering password is shown

ACTUAL OUTCOME:
Authorization required page is shown.

Here is the password manager log:
Pwmgr Prompter: ===== initialized =====
Pwmgr Prompter: ===== promptAuth called =====
Login Manager: Searching for logins matching host: https://projects.maemo.org, formSubmitURL: null, httpRealm: projects.maemo.org
PwMgr Storage: Initializing key3.db with default blank password.
PwMgr Storage: Checking file signons3.txt (SignonFileName3)
PwMgr Storage: Checking file signons2.txt (SignonFileName2)
PwMgr Storage: Checking file signons.txt (SignonFileName)
PwMgr Storage: Reading passwords from /home/romaxa/.TestGtkEmbed/TestGtkEmbed/signons3.txt
PwMgr Storage: Creating new signons file...
PwMgr Storage: Writing passwords to /home/romaxa/.TestGtkEmbed/TestGtkEmbed/signons3.txt
Login Manager: Checking if logins to https://projects.maemo.org can be saved.
Login Manager: onStateChange accepted: req = https://projects.maemo.org/bugzilla, flags = 196612
Login Manager: onStateChange: adding dom listeners
Login Manager: domEventListener: got event DOMContentLoaded

Comment 1

[Oleg Romashin (:romaxa)]

after this call
this._promptService.promptAuth | nsLoginManagerPrompter.js:226

It supposed to go into nsPromptService::PromptAuth and
 GtkPromptService::PromptUsernameAndPassword
... but it doesn't.

Comment 2

[Justin Dolske [:Dolske]]

So, it sounds like password manager is correctly calling promptAuth(), but then that doesn't seem to end up in GtkPromptService?

Have you stepping through this in gdb, starting with a breakpoint in nsPromptService::PromptAuth()? Or, hmm, how is GtkPromptService even supposed to get involved?

Comment 3

[Oleg Romashin (:romaxa)]

> Have you stepping through this in gdb, starting with a breakpoint in
> nsPromptService::PromptAuth()? Or, hmm, how is GtkPromptService even supposed
> to get involved?
> 
Yes, and in firefox case backtrace is next:

this._promptService.promptAuth | nsLoginManagerPrompter.js:226
js_Interpret
NS_InvokeByIndex_P
nsPromptService::PromptAuth | nsPromptService.cpp:724
nsPrompt::PromptPasswordAdapter | nsPrompt.cpp:501
nsPromptService::PromptUsernameAndPassword |nsPromptService.cpp:602
nsPromptService::DoDialog | nsPromptService.cpp:846
and it goes to
after this._promptService.promptAuth | nsLoginManagerPrompter.js:227...


js_Interpret
this._promptService.promptAuth | nsLoginManagerPrompter.js:226
NS_InvokeByIndex_P

and it doesn't go to nsLoginManagerPrompter.js:227...


It not appears even in nsPromptService::PromptAuth().

Comment 4

[Christian Persch (GNOME) (away; not receiving bug mail)]

nsLoginManagerPrompter.js:
 113     get _promptService() {
 114         if (!this.__promptService)
 115             this.__promptService =
 116                 Cc["@mozilla.org/embedcomp/prompt-service;1"].
 117                 getService(Ci.nsIPromptService2);
 118         return this.__promptService;
 119     },

This looks wrong. nsLoginManagerPrompter uses this._promptService to call both methods on nsIPromptService and nsIPromptService2. However Gtkmozembed's prompter only implements nsIPromptService, not nsIPromptService2. So this will throw. There exists an adapter for embedders which only implement nsIPromptService in http://mxr.mozilla.org/mozilla/source/embedding/components/windowwatcher/src/nsPrompt.cpp#80  but if getService'd like that it's not being used. I guess this should use nsWindowWatcher's nsIPromptFactory::GetPrompt to get a nsIAuthPrompt2 ?

Comment 5

[Justin Dolske [:Dolske]]

I'm not really sure what's supposed to be going on here on the embedding side. I suppose the GTK stuff should be implementing nsIPromptService2 now, but I thought the prompt service was supposed to take care of doing the v1/v2 wrapping... Biesi?

Comment 6

[Alexander Sack]

So what would be the right fix for this? implement a nsIPromptServiceAdapterFactory and use that in the nsLoginManagerPrompter.js code instead of Qi'ing nsIPromptService2 directly?

Comment 7

[Alexander Sack]

requesting blocking-firefox3 (aka blocking1.9): from what i understand implementing nsIPromptService2 is still _ment_ to be optional for gecko embedders and the toolkit code should adapt to a generic nsIPromptService if no nsIPromptService2 implementation is provided.

Comment 8

[Alexander Sack]

Attachment: fallback to nsIPromptService if nsIPromptService2 isn't avail

Comment 9

[Christian Persch (GNOME) (away; not receiving bug mail)]

I don't think calling nsIPromptService and nsIPromptService2 directly is right. You should use the windowwwatcher's ::GetPrompt to get a nsIPrompt for calling confirmEx and select; and nsIAuthPrompt2 for calling promptAuth.
The windowwatcher will take care of providing the right interface even if the embedder doesn't implement nsIPromptService2, see 
http://mxr.mozilla.org/mozilla/source/embedding/components/windowwatcher/src/nsWindowWatcher.cpp#1012  and http://mxr.mozilla.org/mozilla/source/embedding/components/windowwatcher/src/nsPrompt.cpp .

Comment 10

[Alexander Sack]

Yes, I couldn't do that easily, because nsLoginManagerPrompt.js does need the checkbox value to determine whether to rememberLogin.

nsIAuthPromptX doesn't provide that capability. Thus, moving the adapter code to the nsLoginManagerPrompt.js appeared to be a pragmatic solution to me.

Comment 11

[Justin Dolske [:Dolske]]

This doesn't strike me as the right approach. All this wrapping is supposed to be happening down in the prompt service. Requiring prompt service consumers to implement the wrapping again seems overly complex and prone to breakage. [And lord knows this code is already ugly enough.]

Can't GtkPromptService just implement nsIPromptService2::PromptAuth() and sidestep this whole problem?

Comment 12

[Benjamin Smedberg]

Well, we're at a funny spot here: we froze nsIPromptService, which could mean:

A: we won't change nsIPromptService, but that doesn't mean your code will keep working
B: if you implement nsIPromptService, your code will keep working

If option A, GtkPromptService should just implement nsIPromptService2. If option B, this patch is probably correct. bz/biesi, opinions?

Comment 13

[Boris Zbarsky [:bzbarsky]]

I think we want option B, in general.

That said, Justin is right, imo.  Consumers shouldn't have to do the fallback manually.  We handle this for global history; we should do the same for the prompt service.

Comment 14

[Benjamin Smedberg]

Well... with global history I used a separate contractid, which makes it possible to use fallback impls. With the prompt service we don't have a separate contractid, right? I don't see a backwards-compatible way to implement an adapter without making the promptservice clients get the adapter manually.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Maybe we should have a separate contractid for the "implements both nsIPromptService and nsIPromptService2" contract...  Though it's not really possible to map the nsIPromptService2 methods into nsIPromptService methods, is it?

Comment 16

[Alexander Sack]

(In reply to comment #15)
> Maybe we should have a separate contractid for the "implements both
> nsIPromptService and nsIPromptService2" contract...  Though it's not really
> possible to map the nsIPromptService2 methods into nsIPromptService methods, is
> it?
> 

... we could add another layer of adaption here for sure. I just wonder if there is really a reason to do so? Isn't this toolkit code the only place where we need to provide this particular kind of backward compatibility? If so, i don't think its really bad to just add this code here for now.

Comment 17

[Benjamin Smedberg]

Comment on attachment 295350 [details] [diff] [review]
fallback to nsIPromptService if nsIPromptService2 isn't avail

ok, I think the general approach here is correct. I'm going to ask dolske to look over the details, because I don't know this code very well.

Comment 18

[Alexander Sack]

dolske, benjamin: any decision?

Thanks,
 - Alexander

Comment 19

[Justin Dolske [:Dolske]]

Comment on attachment 295350 [details] [diff] [review]
fallback to nsIPromptService if nsIPromptService2 isn't avail

*sigh* I still think propagating the nsIPromptService/nsIPromptService2 mess further up into consumers isn't the right thing to do, and is likely to cause more headaches down the road. I guess if bsmedberg says this is the lesser of two evils, then I'm probably grudgingly willing to defer to him, and grind my teeth until Mozilla 2 can save us. :-/

>Index: toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js
>===================================================================


>+    _makeDialogText : function (aChannel,
>+                                aAuthInfo) {

Nit: 1 line.

>+        var stringBundleSvc = Cc["@mozilla.org/intl/stringbundle;1"].
>+                               getService(Ci.nsIStringBundleService);
>+        var stringBundle = stringBundleSvc.createBundle("chrome://global/locale/prompts.properties");

Nit: Align "Cc" and "getService" to same column. stringBundle assignment line is > 80 columns. Maybe s/stringBundleSvc/bundleSvc/ to help keep things short?
 
>+        var port = aChannel.URI.hostPort;
>+        var host = aChannel.URI.host;
>+        var scheme = aChannel.URI.scheme;
>+        var username = aAuthInfo.username;
>+        var password = aAuthInfo.password;
>+        var proxyAuth = aAuthInfo.AUTH_PROXY & aAuthInfo.flags;
>+        var realm = aAuthInfo.realm;
>+        var text = "";
> 
>+        if (port > -1)
>+            host = host + ":" + port;

This can all be replaced with:

var [hostname, httpRealm] = this._getAuthTarget(aChannel, aAuthInfo);

>+        if (proxyAuth) {
>+            text = "EnterUserPasswordForProxy";
>+        } else {
>+            text = "EnterUserPasswordForRealm";
>+            host = scheme + "://" + host;
>+        }

Tweaking host not needed here with _getAuthTarget, can also then remove brackets.

>+        if (aAuthInfo.flags & aAuthInfo.ONLY_PASSWORD) {
>+            text = password;

This should be |text = "EnterPasswordFor";|

>+        } else if (!proxyAuth && !realm) {
>+            text = "EnterUserPasswordFor";
>+            count--;
>+            strings[0] = strings[1];
>+        }

This case goes away with _getAuthTarget(). It returns the hostname as the realm when there's no realm.

>+    _promptPasswordAdapter : function (aPromptService,
...
>+        if(flags & aAuthInfo.ONLY_PASSWORD) {
>+           rv = aPromptService.promptPassword(aParentDOMWindow, null, message, defaultPass, aCheckLabel, aCheckValue);
>+        }

This isn't right, see next comment. [Sorry, was reviewing bits in a random order. :-)]

>+        else  {
>+           var defaultUserObject = new Object();
>+           defaultUserObject.value = defaultUser;
>+           var defaultPassObject = new Object();
>+           defaultPassObject.value = defaultPass;
>+           rv = aPromptService.promptUsernameAndPassword(aParentDOMWindow, null, message, defaultUserObject, defaultPassObject, aCheckLabel, aCheckValueInOut);

This isn't right, the values entered by the user will be ignored.

You want:

           var usernameInOut = { value : defaultUser }
           var passwordInOut = { value : defaultPass }
           rv = aPromptService.promptUsernameAndPassword(...)
           if (!rv)
             this._SetAuthInfo(aAuthInfo, usernameInOut.value, passwordInOut.value);

(Check that rv works as expected there)

Also, nit, line is > 80 columns.

>-        var ok = this._promptService.promptAuth(this._window, aChannel,
>-                                aLevel, aAuthInfo, checkboxLabel, checkbox);
>+        var ok = false;
>+        try {
>+            ok = this._promptService2.promptAuth(this._window, aChannel,
>+                                      aLevel, aAuthInfo, checkboxLabel, checkbox);
>+        } catch (e) {
>+            try {
>+            ok = this._promptPasswordAdapter (this._promptService, this._window, aChannel,
>+                                              aLevel, aAuthInfo, checkboxLabel, checkbox);
>+            } catch(e) { dump("exception in _promptPasswordAdapter ::: "+e+" \n"); throw e; }
>+        }
>+            

I'd rather contain all this in promptPasswordAdapter()... Just s/promptAuth/promptPasswordAdapter/ in the original code, and have promptPasswordAdapter() handle the try/catch fallback.

Is there some to determine which promptService to use, without a try/catch? I'm a bit concerned that we could get into a case where promptService2.promptAuth() is what we really do want to be calling, it throws an exception [on purpose or accidentally]. Now we would be ignoring the failure and trying again with promptService1.

Comment 20

[Justin Dolske [:Dolske]]

Oh, almost forgot. The realm returned from _getAuthTarget() should be trimmed to 150 characters. This was just changed in bug 244273. [And is an example of why I don't like duplicating code like this. :-(]

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

This does not block the final release of Firefox 3.

Comment 23

[Rolf Leggewie]

Is there still anything that needs to be done here?

https://bugs.launchpad.net/xulrunner/+bug/182519 lists this as the upstream task, but epiphany in ubuntu seems to be running fine.

Comment 24

[Tero Turtiainen]

I have an embedding problem. I have implemented AsyncPromptAuth of nsIPromptService2. It works fine, if nsLoginManagerPrompter.js has been deleted. Authentication prompt handling is asynchronous and everything works fine. But I also need the functionality provided by nsLoginManagerPrompter.js. And when nsLoginManagerPrompter.js is enabled, my async authentication prommpt handling is never called. Instead it looks like nsLoginManagerPrompter.js calls the synchronous PromptAuth or my component which is definitely something I don't want. I want to get rid of the synchronous PromptAuth completely.

The synchronous PromptAuth seems to get called from here in nsLoginManagerPrompter.js:

        var runnable = {
            run : function() {
...
                    ok = prompt.prompter.promptAuth(prompt.channel,
                                                    prompt.level,
                                                    prompt.authInfo);

Is my problem related to this bug, is my problem something else or am I doing something wrong?

Somehow it seems strange that the the prompt situation handling of nsLoginManagerPrompter.js should launch the actual prompt and in the case of asynchronous prompt it would launch a synchronous prompt.

Comment 25

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This bug was last touched six years ago, is it still a bug?

Comment 26

[Matthew N. [:MattN]]

I don't know anything about TestGtkEmbed and whether that's something we need to support anymore.

Comment 27

[Mike Hommey [:glandium]]

TestGtkEmbed was removed in bug 648156, 4.5 years ago.