Closed Bug 784028 Opened 8 years ago Closed 7 years ago

Password Manager does not save passwords in dialog window

Categories

(Toolkit :: Password Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: stomlinson, Assigned: MattN)

References

Details

Attachments

(2 files, 2 obsolete files)

The password manager does not show when filling in passwords from a dialog window that was opened using window.open. This affects Mozilla Persona since users enter their passwords in a dialog.

STR
1) Enable Password Manager debugging as described on https://wiki.mozilla.org/Firefox:Password_Manager_Debugging
2) Visit 123done.org
3) Click "Sign In with Persona"
4) Open the Error Console
5) In the Persona dialog, enter email address and password

The following exception is displayed in the Error Console:

Timestamp: 20/08/2012 15:38:55
Error: [Exception... "'PopupNotifications_show: invalid browser' when calling method: [nsILoginManagerPrompter::promptToSavePassword]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: resource:///components/nsLoginManager.js :: <TOP_LEVEL> :: line 971"  data: no]
Source File: resource:///components/nsLoginManager.js
Line: 971

Please see Mozilla Persona issue #314:
https://github.com/mozilla/browserid/issues/314
Component: Security → Password Manager
Product: Firefox → Toolkit
Version: 14 Branch → Trunk
(In reply to Shane Tomlinson [:stomlinson] from comment #0)
> Error: [Exception... "'PopupNotifications_show: invalid browser' when
> calling method: [nsILoginManagerPrompter::promptToSavePassword]"  nsresult:
> "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame ::
> resource:///components/nsLoginManager.js :: <TOP_LEVEL> :: line 971"  data:
> no]
> Source File: resource:///components/nsLoginManager.js
> Line: 971

This error implies that we're passing a null "browser" argument to PopupNotifications.show() here:
http://hg.mozilla.org/mozilla-central/annotate/7b385ab02118/toolkit/components/passwordmgr/nsLoginManagerPrompter.js#l822

I can reproduce this, I'll look into it.
So the issue is that we try to use the opener window (_getNotifyWindow returns the opener because the window being prompted for is chromeless), but then we try to retrieve a browser to use from it using the document that we're actually notifying for (the document in the popup window). That returns null, and we fail.
As far as I can tell this bug dates back to the original implementation of door hangers notifications for passwords, in Firefox 4:
https://hg.mozilla.org/mozilla-central/rev/6a8048ca8b63
OS: Mac OS X → All
Hardware: x86 → All
Attached patch patch (obsolete) — Splinter Review
This should fix the issue, I think it's correct. I haven't tested what ends up actually happening, though - if we don't focus the opener and the popup window stays open, this could be rather confusing UI.
Assignee: nobody → mnoorenberghe+bmo
Status: NEW → ASSIGNED
Attached patch Tests v.1 (obsolete) — Splinter Review
Attachment #666866 - Flags: review?(gavin.sharp)
Comment on attachment 666866 [details] [diff] [review]
Tests v.1

Seems like you could save on some duplication by re-using subtst_notifications_11_popup.html, and having it take the password to submit from location.search or something. Same with subtst_notifications_11.html and the popup URL to open. But maybe you just copied this infra; r=me either way.
Attachment #666866 - Flags: review?(gavin.sharp) → review+
Comment on attachment 665734 [details] [diff] [review]
patch

I manually tested various scenarios changing the following variables:
* whether the opener tab is selected
* save password vs. update password
* whether the popup window closes right after submission vs. staying open (e.g. wrong password on Persona)

and I didn't see any blocking problems.  This patch is changing the behaviour to how it worked before with notifications bars and we only hit this case when the login form was the first document loaded so I think this fix outweighs the possible confusion having a doorhanger display in the opener. We include the opener's domain name in the save password doorhanger (but not for password-change).

An idea to possibly improve the experience:
* Also show the doorhanger in a dismissed state in the popup and remove the doorhanger from the opener after a timeout.

Shall I file a follow-up?
Attachment #665734 - Flags: review+
Do we focus the window that the popup notification opened in? Or do you only see the notification when you switch to it manually, in the case where the popup stays open?
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #8)
> Do we focus the window that the popup notification opened in? 

No, we don't change the focus when popup notifications are displayed.

> Or do you only see the notification when you switch to it manually, in the case 
> where the popup stays open?

Yes
I tested how this works in popups with chrome and noticed that bug 113580 regressed the @chromehidden check.
Attachment #665734 - Attachment is obsolete: true
Attachment #667235 - Flags: review?(gavin.sharp)
Comment on attachment 667235 [details] [diff] [review]
v.2 fix chromehidden check

good catch!

Showing a popup notification in a background window that doesn't get focus seems like really poor UX, but I'm not sure what better options we have. It's certainly better than failing to show the notification at all, though, so we can file a followup for that.
Attachment #667235 - Flags: review?(gavin.sharp) → review+
Cleaned up duplication and added tests for the chromehidden fix.

I think we can use bug 631802 for further UX improvements. I add bug 631802 comment 12 with my idea of show doorhangers in both windows along with some more details.
Attachment #666866 - Attachment is obsolete: true
Attachment #667372 - Flags: review?(gavin.sharp)
Depends on: 631802
Comment on attachment 667372 [details] [diff] [review]
Tests v.2 - added visible chrome tests

>diff --git a/toolkit/components/passwordmgr/test/test_notifications_popup.html b/toolkit/components/passwordmgr/test/test_notifications_popup.html

>+PasswordMgrObserver = {
>+  observe: function passwordMgrObserver_observe(subject, topic, data) {
>+      if (data == "removeLogin")
>+          return;
>+      runNextTest();
>+  },
>+};

Can you explain how this works in a comment? Seems to depend on the specifics of the rest of the test in non-obvious ways, hard to follow why this is doing what it's doing.

Thanks a lot for picking this up, these tests are great.
Attachment #667372 - Flags: review?(gavin.sharp) → review+
No longer depends on: 631802
Disabled the test on android: https://hg.mozilla.org/integration/mozilla-inbound/rev/d93199111fda

I didn't think this test would run on Android because I knew test_notifications.html (which I based this on) would have failed and didn't know about android.json until now.
Duplicate of this bug: 631802
Duplicate of this bug: 631802
You need to log in before you can comment on or make changes to this bug.