422974 - Prism uses old "Remember password?" mechanism

Status: RESOLVED FIXED

(Mozilla Labs :: Prism, defect)

Description

[Matthew Wilson]

With Firefox 3 beta 4, Prism 0.2 extension on Windows.

I created a Prism app for Google Reader.

When I launched it and tried to log in, I received the old-style prompt

  "Do you want Prism to remember this password?"

and had to reply before the request was submitted.

In Firefox 3, the behaviour is for the request to be submitted immediately, and then, on page load, to be asked if you want to remember the password.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Looks like the code in nsLoginManagerPrompter.js is hard wired to look for a "tabbrowser" in "navigator:browser" window types. So it won't work for Prism (or any other application that is not designed in just the right way). Some form of a callback would be a nice addition in the code.

See http://mxr.mozilla.org/seamonkey/source/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js#888

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Yeah, the dependency on "browser" internals like the presence of a tabbrowser/getBrowser()/etc. for notification bar prompting isn't ideal. I think ideally we'd let the toolkit prompting code notify the browser code and let it do the prompting itself, somewhat like how popup blocking works (with its DOMPopupBlocked event fired on the window). Don't think we can rework this for 1.9, but we should get a bug on file.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: Patch v.1

So, this works. Comments on the appropriateness of the implementation?

Something's not quite right here, though. After banging my head against this today, I found I have to use .wrappedJSObject to get at nsIDOMChromeWindow's |browserDOMWindow| attribute. Otherwise I get an "illegal value" exception thrown when trying to access the attribute. I'm not really sure what's going on with that. :(

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Does it work with an explicit QI to nsIBrowserDOMWindow instead of .wrappedJSObject?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Unfortunately, that won't help. The problem is that the browserDOMWindow set on the chrome window is a JS-implemented object. That means that the stored nsIBrowserDOMWindow in C++ is an XPCWrappedJS. Then, without the wrappedJSObject, looking up chromeWin.browserDOMWindow causes XPCNativeWrapper to try to deep-wrap the XPCWrappedJS, and that isn't allowed, so it throws.

Because chromeWin is a chrome window, using .wrappedJSObject to get at the underlying object is safe and the correct thing to do here.

Comment 6

[Justin Dolske [:Dolske]]

Attachment: Patch v.2

This fixes Firefox to work the new way, will need to also add the browser.js change to Seamonkey's naviagtor.js, and wherever Prism's equivalent it.

I don't think this specific change needs a test, as it should be covered by the general password manager notification bar tests. Which I, uhh, need to finish writing first.

Comment 7

[Justin Dolske [:Dolske]]

Attachment: Patch for SeaMonkey, v.1

This should be the obvious change for SeaMonkey. Not sure how to verify it since SM isn't using the new login manager yet, perhaps Mark could give it a quick spin with his patches for login manager?

Comment 8

[Matthew Gertner]

Once this lands, please assign to me so I can fix Prism. !Gracias!

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: Patch for Prism

This patch adds the <notificationbox> to the XUL and the getNotificationBox method to nsBrowserAccess. When combined with the nsLoginManagerPrompter changes, cool modeless password notifications should happen.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 328777 [details] [diff] [review]
Patch v.2

>diff --git a/dom/public/idl/base/nsIBrowserDOMWindow.idl b/dom/public/idl/base/nsIBrowserDOMWindow.idl

bz or bsmedberg should probably review this.

>diff --git a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>     _getNotifyBox : function () {
>+        var notifyBox = null;
>+
>+        // Given a content DOM window, returns the chrome window it's in.
>+        function getChromeWindow(aWindow) {
>+            var chromeWin = aWindow 
>+                                .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                .getInterface(Ci.nsIWebNavigation)
>+                                .QueryInterface(Ci.nsIDocShellTreeItem)
>+                                .rootTreeItem
>+                                .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                .getInterface(Ci.nsIDOMWindow);
>+            // no need to QI to nsIDOMChromeWindow
>+            return chromeWin;

Are you sure? Sometimes this works because the element was previously QIed in unrelated code, and relying on that can be fragile. I'd add the explicit QI to be sure.

Comment 11

[Justin Dolske [:Dolske]]

Attachment: Patch v.3

Added QI from gavin's review, requesting review from bz for the nsIBrowserDOMWindow.idl change.

Comment 12

[[e-mail]]

(In reply to comment #10)
>(From update of attachment 328777 [details] [diff] [review])
>>+            // no need to QI to nsIDOMChromeWindow
>>+            return chromeWin;
>Are you sure?
Windows have class info, so yes.

Comment 13

[[e-mail]]

Comment on attachment 330302 [details] [diff] [review]
Patch v.3

>             if (notifyWindow.opener) {
>+                var chromeWin = getChromeWindow(notifyWindow);
...
>             }
> 
> 
>+            // Get the chrome window for the content window we're using.
>+            var chromeWin = getChromeWindow(notifyWindow);
Why not get the chrome window first?

>+            var browserWin = chromeWin.wrappedJSObject.browserDOMWindow;
This wrappedJSObject looks fishy to me, whatever mrbkap says.

Comment 14

[[e-mail]]

Comment on attachment 330302 [details] [diff] [review]
Patch v.3

>+                                .QueryInterface(Ci.nsIInterfaceRequestor)
>+                                .getInterface(Ci.nsIWebNavigation)
Another option is to do
var browser = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                     .getInterface(Ci.nsIWebNavigation)
                     .QueryInterface(Ci.nsIDocShell)
                     .chromeEventHandler;
then crawl the parent node chain looking for a notificationbox.

Comment 15

[[e-mail]]

Comment on attachment 328789 [details] [diff] [review]
Patch for SeaMonkey, v.1

I tested this in conjunction with the core patch and a patch to build login manager in SeaMonkey.

>   isTabContentWindow: function isTabContentWindow(aWindow) {
>     var browsers = gBrowser.browsers;
>     for (var i = 0; browsers.item(i); i++)
>       if (browsers[i].contentWindow == aWindow)
>         return true;
>     return false;
>+  },
>+
Nit: file style is not to have blank lines in object literals.

>+  getNotificationBox : function(aWindow)
>+  {
>+    var foundBrowser = gBrowser.getBrowserForDocument(aWindow.document);
>+    if (foundBrowser)
>+      return gBrowser.getNotificationBox(foundBrowser)
>+    return null;
>   }
Nit: getBrowserForDocument was only created for the convenience of ported Firefox extensions. (Mind you, the isTabContentWindow loop is out of date too, it should use i < browsers.length). You can also use browsers[i].parentNode to refer to the notificationbox.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 330302 [details] [diff] [review]
Patch v.3

I'm not sure I can review this: I'm not a toolkit/browser peer last I checked, and I just plain don't know the code you're touching.

Comment 17

[Justin Dolske [:Dolske]]

Comment on attachment 330302 [details] [diff] [review]
Patch v.3

(bz r- the IDL change in earlier discussions on IRC. So, I need to look for a new place to glue this interface.)

Comment 18

[Justin Dolske [:Dolske]]

Attachment: Patch v.4

Slightly different approach: avoids all the XPCOMmyness, and just hangs |getNotificationBox| directly on the window.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 333012 [details] [diff] [review]
Patch v.4

>diff --git a/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js b/toolkit/components/passwordmgr/src/nsLoginManagerPrompter.js

>             if (notifyWindow.opener) {
>+                var chromeWin = getChromeWindow(notifyWindow);
>+                var chromeDoc = chromeWin.document.documentElement;

Just use:

var chromeDoc = getChromeWindow(notifyWindow).document.documentElement;

here to avoid redeclaring chromeWin?

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

I noticed that getBrowser() and getNavToolbox() have been deprecated in favor of "smart" properties. However, other getXxx() functions are still being used so we might be safe.

Comment 21

[Justin Dolske [:Dolske]]

Fixed gavin's nit, and pushed changeset f3577b7629fb.

Comment 22

[Justin Dolske [:Dolske]]

Attachment: Patch for SeaMonkey, v.2

Updated patch for SeaMonkey.

Comment 23

[Justin Dolske [:Dolske]]

Attachment: Patch for Prism, v.2

Updated patch for Prism.

(Looks like the old patch was already landed, so this reverts the old version too.)

Comment 24

[[e-mail]]

Comment on attachment 333312 [details] [diff] [review]
Patch for SeaMonkey, v.2

>+function getNotificationBox(aWindow)
>+{
>+  var foundBrowser = gBrowser.getBrowserForDocument(aWindow.document);
>+  if (foundBrowser)
>+    return gBrowser.getNotificationBox(foundBrowser)
>+  return null;
>+}

This won't work for the sidebar, but I think this might:
return aWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
              .getInterface(Components.interfaces.nsIWebNavigation)
              .QueryInterface(Components.interfaces.nsIDocShell)
              .chromeEventHandler.parentNode.wrappedJSObject;

Comment 25

[[e-mail]]

Comment on attachment 333312 [details] [diff] [review]
Patch for SeaMonkey, v.2

Please comment this as API used by login manager.

What's the bug# for the sidebar bug?

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

Landed Prism patch #2 (333314)
Completed: At revision: 17712  

Comment 27

[Justin Dolske [:Dolske]]

(In reply to comment #25)

> What's the bug# for the sidebar bug?

Bug 430959. You're already CC'd and have commented there. :) 

Comment 28

[Justin Dolske [:Dolske]]

I suppose this new API should be documented somewhere, but not sure where... http://developer.mozilla.org/En/XUL:notificationbox ?

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #28)
> I suppose this new API should be documented somewhere, but not sure where...
> http://developer.mozilla.org/En/XUL:notificationbox ?
> 

How about:
http://developer.mozilla.org/en/Using_nsILoginManager
http://developer.mozilla.org/en/NsILoginManager

Comment 30

[Justin Dolske [:Dolske]]

Well, it's really a general API for getting at the notification box, not something that's pwmgr specific.

Comment 31

[Eric Shepherd [:sheppy]]

Question: if you call this method within a window (ie:

notifyBox = chromeWin.getNotificationBox(notifyWindow);

What's the difference between chromeWin and notifyWindow here?

Comment 32

[Mark Finkle (:mfinkle) (use needinfo?)]

chromeWin is the XUL window, the actual Firefox window.
notifyWindow is the web content window, displayed in each tab.