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

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Martijn Wargers (dead)]

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

Comment 1

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

Comment 2

[:Margaret Leibovic]

Oops, I can still be assigned, although this might be a WONTFIX.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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.

Comment 4

[:Margaret Leibovic]

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

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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)

Comment 6

[:Margaret Leibovic]

Attachment: patch

Bail if hostname is null.

Comment 7

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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?

Comment 8

[:Margaret Leibovic]

(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

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(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 :)

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/731fce748f78