If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsLoginManagerPrompter.js does not work with TestGtkEmbed prompt.

RESOLVED INVALID

Status

()

Toolkit
Password Manager
RESOLVED INVALID
10 years ago
2 years ago

People

(Reporter: romaxa, Assigned: Alexander Sack)

Tracking

Trunk
x86
Linux
Points:
---
Bug Flags:
blocking1.9 -
wanted1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
after this call
this._promptService.promptAuth | nsLoginManagerPrompter.js:226

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

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?
(Reporter)

Comment 3

10 years ago
> 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().
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 ?
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?
(Assignee)

Comment 6

10 years ago
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?
(Assignee)

Comment 7

10 years ago
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.

Flags: blocking-firefox3?
(Assignee)

Comment 8

10 years ago
Created attachment 295350 [details] [diff] [review]
fallback to nsIPromptService if nsIPromptService2 isn't avail
Assignee: nobody → asac
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Attachment #295350 - Flags: review?(benjamin)
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 .
(Assignee)

Comment 10

10 years ago
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.



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

10 years ago
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?
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

10 years ago
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.
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?
(Assignee)

Comment 16

10 years ago
(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

10 years ago
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.
Attachment #295350 - Flags: review?(dolske)
Attachment #295350 - Flags: review?(benjamin)
Attachment #295350 - Flags: review+
(Assignee)

Comment 18

10 years ago
dolske, benjamin: any decision?

Thanks,
 - Alexander
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.
Attachment #295350 - Flags: review?(dolske) → review-
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. :-(]
This does not block the final release of Firefox 3.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-

Updated

10 years ago
Duplicate of this bug: 432291
Product: Firefox → Toolkit

Comment 23

9 years ago
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

8 years ago
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.

Updated

8 years ago
This bug was last touched six years ago, is it still a bug?
Flags: needinfo?(romaxa)
Flags: needinfo?(MattN+bmo)
I don't know anything about TestGtkEmbed and whether that's something we need to support anymore.
Flags: needinfo?(MattN+bmo) → needinfo?(dolske)
TestGtkEmbed was removed in bug 648156, 4.5 years ago.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(romaxa)
Flags: needinfo?(dolske)
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.