No password management anymore in epiphany

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?

> 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?

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?

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.

Created an attachment (id=295350)
fallback to nsIPromptService if nsIPromptService2 isn't avail

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 .

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?

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.

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?

(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.

(From update of attachment 295350)
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.

dolske, benjamin: any decision?

Thanks,
 - Alexander

(From update of attachment 295350)
*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.promptUserna...

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.

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

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.

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.