Closed Bug 718708 Opened 8 years ago Closed 8 years ago

Doorhanger notification mentions (null) in this case with password form in data url iframe

Categories

(Toolkit :: Password Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: martijn.martijn, Assigned: Margaret)

References

(Depends on 1 open bug, )

Details

Attachments

(1 file)

See url, steps to reproduce:
- Type some values in the inputs inside the iframe
- Submit

Expected result:
- Password doorhanger notification with the text "Do you want Nightly to remember the password for "a" on mozilla.org?"

Actual result:
- Password doorhanger notification with the text "Do you want Nightly to remember the password for "a" on (null)?"
Assignee: nobody → margaret.leibovic
This also happens on desktop, and I don't know if this is even expected to work. We're getting a null hostname, and I imagine that's because we're looking for the host in the iframe, but this testcase just uses a data: URL for the iframe.

Dolske, what say you?
Assignee: margaret.leibovic → nobody
Component: General → Password Manager
Product: Fennec Native → Toolkit
QA Contact: general → password.manager
Oops, I can still be assigned, although this might be a WONTFIX.
Assignee: nobody → margaret.leibovic
Yea, the form is submitting to a data uri, so we can't actually save it.

Though there's a bug here for sure, pressing "Remember password" actually throws an exception. We shouldn't even show the doorhanger here so we can avoid that entirely.
(In reply to Paul O'Shannessy [:zpao] from comment #3)
> Though there's a bug here for sure, pressing "Remember password" actually
> throws an exception. We shouldn't even show the doorhanger here so we can
> avoid that entirely.

That sounds like a good idea - should I just do a check somewhere in here and return if we don't have a valid hostname: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManager.js#822. Is there anything else that could be invalid?
At https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsLoginManager.js#846 hostname & formSubmitURL are both null (because we caught an exception in _getPasswordOrigin, for both). We have to have a hostname to save, but we don't need a formSubmitURL (could have an httpRealm instead).

So if hostname is null or empty, just bail. I think _getPasswordOrigin covers it well enough for now - any protocol that doesn't allow a port throws & we return null (that means we can't save passwords on about: urls)
Attached patch patchSplinter Review
Bail if hostname is null.
Attachment #589781 - Flags: review?(paul)
Attachment #589781 - Attachment is patch: true
Comment on attachment 589781 [details] [diff] [review]
patch

Review of attachment 589781 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. I'm not sure what the current tests look like, but it would be nice to have a test for this.

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +842,5 @@
>          // If password saving is disabled (globally or for host), bail out now.
>          if (!this._remember)
>              return;
>  
>          var hostname      = this._getPasswordOrigin(doc.documentURI);

nit: now that this isn't lined up with formSubmitURL, can you can get rid of the extraneous spacing?
Attachment #589781 - Flags: review?(paul) → review+
OS: Android → All
Hardware: ARM → All
(In reply to Paul O'Shannessy [:zpao] from comment #7)
> Looks good to me. I'm not sure what the current tests look like, but it
> would be nice to have a test for this.

This is on my list of things to do, but I didn't want this patch to get left behind by accident:

https://hg.mozilla.org/integration/mozilla-inbound/rev/731fce748f78
(In reply to Margaret Leibovic [:margaret] from comment #8)
> (In reply to Paul O'Shannessy [:zpao] from comment #7)
> > Looks good to me. I'm not sure what the current tests look like, but it
> > would be nice to have a test for this.
> 
> This is on my list of things to do, but I didn't want this patch to get left
> behind by accident:

And now that I filed bug 720043 it's _really_ on your list of things to do :)
https://hg.mozilla.org/mozilla-central/rev/731fce748f78
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.