1147561 - When deciding whether to autofill a form, ignore the scheme in the formSubmitURL

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P1)

Description

[Tanvi Vyas[:tanvi]]

When finding all the login candidates for a form in the password manager database, we use the formSubmitURL as one of the keys.  During fill time, instead of checking the current page's form action against the full scheme:hostname in the formSubmitURL, just check the hostname.  We can continue capturing scheme:hostname.

For example, assume we have a username/password record in the password manager database for http://foo.com with a formSubmitURL http://bar.com.  Now assume the user visits http://foo.com and finds a login form with action https://bar.com.  The password manager should fill the the record we have in the password manager database with formSubmitURL http://bar.com, even though the scheme is different.

Comment 1

[Tanvi Vyas[:tanvi]]

This should probably be done here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/storage-json.js#336

We can still store the scheme:hostname and just match on the hostname.

Comment 2

[Matthew N. [:MattN]]

Tanvi, is this a dupe of one of the other bugs you're assigned? e.g. bug 667233?

Comment 3

[Tanvi Vyas[:tanvi]]

(In reply to Matthew N. [:MattN] from comment #2)
> Tanvi, is this a dupe of one of the other bugs you're assigned? e.g. bug
> 667233?

No.  This bug is about the formSubmitURL while the others are about the hostname.  This bug needs an owner if anyone wants to take it.  Shouldn't take too long.

Comment 4

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: bz://1147561/rittme

/r/9203 - Bug 1147561 - If no formSubmitURL matches, look for matches with different schemes

Pull down this commit:

hg pull -r 1a0aef4f9303b7ef6529f2842c5b3b181c175c6e https://reviewboard-hg.mozilla.org/gecko/

Comment 5

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/9203/#review7865

I don't see any callers in the shipping code that use the new ignoreScheme argument of .matches. Isn't that needed to avoid the remember dialog from appearing when a user takes advantage of this new functionality?

::: toolkit/components/passwordmgr/nsLoginInfo.js:54
(Diff revision 1)
> +      if(!ignoreScheme)
> -      return false;
> +        return false;

Nit: space after `if` and always use curly braces for `if`/`else` as that's the current style recommendation.

::: toolkit/components/passwordmgr/test/unit/test_logins_search.js:104
(Diff revision 1)
> + * @param aQuery
> + *        The "hostname", "formSubmitURL", and "httpRealm" properties of this
> + *        object are passed as parameters to findLogins, countLogins
> + *        and searchLogins function.
> + * @param buildQuery
> + *        The "hostname", "formSubmitURL", and "httpRealm" properties of the
> + *        object used to build the expected logins to have as a result.
> + * @param aExpectedCount

Nit: include types for new @param tags e.g.
@param {Number} aExpectedCount

::: toolkit/components/passwordmgr/test/unit/test_logins_search.js:287
(Diff revision 1)
> +    formSubmitURL:"http://www.example.com",

Nit: missing space after colon

::: toolkit/components/passwordmgr/test/unit/test_logins_search.js:282
(Diff revision 1)
> +    "formSubmitURL": "https://www.example.com",
> +    "hostname": "http://www.example.com"
> +  };
> +
> +  let buildQuery = {
> +    formSubmitURL:"http://www.example.com",
> +    "hostname": "http://www.example.com"

Nit: Don't quote the property names unless you need to (e.g. for special characters).

Nit: Always include a trailing comma after the last property value in an object so blame doesn't change if a new property is added after it.

::: toolkit/components/passwordmgr/nsLoginInfo.js:42
(Diff revision 1)
> -  matches : function (aLogin, ignorePassword) {
> +  matches : function (aLogin, ignorePassword, ignoreScheme) {

I'm still not really convinced we need a new argument for this but it seems like you're not currently using the argument outside of tests. If you are making an optional argument, add a default value of null to make this obvious. e.g. `…, ignoreScheme = null) {`

Nit: While you're touching this line, can you switch it to newer method syntax:
`matches(aLogin, ignorePassword, ignoreScheme = null) {`

::: toolkit/components/passwordmgr/test/test_basic_form_1pw.html:127
(Diff revision 1)
> +<form id='form15' action='https://mochi.test:8888/tests/formtest.js'> 15

Are you sure this is the correct path? Or are we not actually submitting anyways?

::: toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html:92
(Diff revision 1)
>      pwmgr.addLogin(login6B);
>      pwmgr.addLogin(login7);
>      pwmgr.addLogin(login8A);
>      pwmgr.addLogin(login8B);
> +    pwmgr.addLogin(login10);

Nit: what happened to login 9? 7 ate 9? :P
https://en.wikipedia.org/wiki/Riddle#Contemporary_riddles

I guess you're trying to align it with the form id which is fine if that's what we did in other parts of the test

::: toolkit/components/passwordmgr/test/unit/test_logins_search.js
(Diff revision 1)
> -  checkAllSearches({ formSubmitURL: "http://www.example.com/" }, 0);

I was suggesting yesterday that you could change the number to 2 here to test the new behaviour along with adding a line below it that included more of a path. Does that not work?

Comment 6

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8608905 [details]
MozReview Request: bz://1147561/rittme

/r/9203 - Bug 1147561 - If no formSubmitURL matches, look for matches with different schemes

Pull down this commit:

hg pull -r 1a0aef4f9303b7ef6529f2842c5b3b181c175c6e https://reviewboard-hg.mozilla.org/gecko/

Comment 7

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8608905 [details]
MozReview Request: bz://1147561/rittme

/r/9203 - Bug 1147561 - If no formSubmitURL matches, look for matches with different schemes

Pull down this commit:

hg pull -r 40c436568ab54da33be37393ad2585d4f62d0760 https://reviewboard-hg.mozilla.org/gecko/

Comment 8

[Bernardo Rittmeyer [:rittme]]

https://reviewboard.mozilla.org/r/9203/#review7869

> Are you sure this is the correct path? Or are we not actually submitting anyways?

We are not submitting the form, so the path shouldn't matter. The host address is enough for the autocomplete to work.

> I was suggesting yesterday that you could change the number to 2 here to test the new behaviour along with adding a line below it that included more of a path. Does that not work?

The problem is it would not work. Because the expected logins list is created using strict equality, for this formSubmitURL it will find 0 expected logins. The actual search will find 4 results, as it will also match all the ones without "/".

Is it ok to ignore this case or should I find another way to test this cases? Like creating another test method to do so.

Comment 9

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/9203/#review7873

::: toolkit/components/passwordmgr/nsLoginInfo.js:42
(Diff revision 2)
> -  matches : function (aLogin, ignorePassword) {
> +  matches (aLogin, ignorePassword, ignoreScheme = null) {

Nit: remove the space in `matches (`

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8608905 [details]
MozReview Request: bz://1147561/rittme

Clearing review while the following is getting addressed:

(Quoting Matthew N. [:MattN] from comment #5)
> https://reviewboard.mozilla.org/r/9203/#review7865
> 
> I don't see any callers in the shipping code that use the new ignoreScheme
> argument of .matches. Isn't that needed to avoid the remember dialog from
> appearing when a user takes advantage of this new functionality?

Comment 11

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=40c436568ab5

Comment 12

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8608905 [details]
MozReview Request: bz://1147561/rittme

/r/9203 - Bug 1147561 - If no formSubmitURL matches, look for matches with different schemes
/r/9389 - removed nslogininfo.match last argument, added support for mozstorage, fixed tests for this new scenario

Pull down these commits:

hg pull -r db3e5c5d254fa3c3c0cfa4cd5372fa0a67143bfc https://reviewboard-hg.mozilla.org/gecko/

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8608905 [details]
MozReview Request: bz://1147561/rittme

https://reviewboard.mozilla.org/r/9201/#review8151

::: toolkit/components/passwordmgr/storage-mozStorage.js:551
(Diff revision 3)
> +        if(login.formSubmitURL == '' ||

Nit: missing space after `if`

::: toolkit/components/passwordmgr/storage-mozStorage.js:572
(Diff revision 3)
> +    if(!logins.length && fallbackLogins.length) {

Ditto

Comment 14

[Matthew N. [:MattN]]

(In reply to Matthew N. [:MattN] from comment #10)
> (Quoting Matthew N. [:MattN] from comment #5)
> > https://reviewboard.mozilla.org/r/9203/#review7865
> > 
> > I don't see any callers in the shipping code that use the new ignoreScheme
> > argument of .matches. Isn't that needed to avoid the remember dialog from
> > appearing when a user takes advantage of this new functionality?

Can you add a test that ensures the doorhanger doesn't appear in this case?

Comment 15

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8608905 [details]
MozReview Request: bz://1147561/rittme

/r/9203 - fixed if missing spaces
/r/9389 - Bug 1147561 - new test check if doorhanger don't open for different scheme formSubmitURL

Pull down these commits:

hg pull -r f5452ca854a00a7c3a385e7ec63d237e5d0a65c2 https://reviewboard-hg.mozilla.org/gecko/

Comment 16

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Bug 1147561 - If no formSubmitURL matches, look for matches with different schemes
***
Fixed small issues with the last patch
***
removed nslogininfo.match last argument, added support for mozstorage, fixed tests for this new scenario
***
fixed if missing spaces
***
new test check if doorhanger don't open for different scheme formSubmitURL
***
removed trailing space

Comment 17

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Bug 1147561 - If no formSubmitURL matches, look for matches with different schemes
***
Fixed small issues with the last patch
***
removed nslogininfo.match last argument, added support for mozstorage, fixed tests for this new scenario
***
fixed if missing spaces
***
new test check if doorhanger don't open for different scheme formSubmitURL
***
removed trailing space

Comment 18

[Matthew N. [:MattN]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

https://reviewboard.mozilla.org/r/9203/#review8303

Looking good fo far. I'll finish this review tomorrow but here are some small things.

::: toolkit/components/passwordmgr/storage-json.js:415
(Diff revision 4)
> +    if(!foundLogins.length && fallbackLogins.length) {

Nit: missing space after `if`

::: toolkit/components/passwordmgr/nsLoginInfo.js:5
(Diff revision 4)
>  
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");

Nit: Can you delete the extra empty line 5, add aliases for other properties on Components using the new syntax:
`const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;`
and switch these two lines to use `Cu`.

::: toolkit/components/passwordmgr/nsLoginInfo.js:55
(Diff revision 4)
> +      // Check if it matches with different protocol

Nit: You're missing a word and "scheme" is slightly more accurate then "protocol".

::: toolkit/components/passwordmgr/nsLoginInfo.js:56
(Diff revision 4)
> +      try {
> +        let loginURI = Services.io.newURI(aLogin.formSubmitURL, null, null);
> +        let matchURI = Services.io.newURI(this.formSubmitURL, null, null);
> +        if (loginURI.hostPort == matchURI.hostPort) {
> +          return true;
> +        }

The flow of this method is that `return false` is used when something doesn't match and then the default is `true`. This makes it easier to added new conditions like the one you're adding since you don't need to go back and change existing cases that were previously returning true to add some extra conditions. Can you switch this `return true` to a `return false` and invert the condition to keep the flow of the method?

::: toolkit/components/passwordmgr/storage-json.js:406
(Diff revision 4)
> +        if(result.strictMatch) {

Nit: missing space after `if`

::: toolkit/components/passwordmgr/storage-mozStorage.js:551
(Diff revision 4)
> +        if (login.formSubmitURL == '' ||

Double quotes are preferred by default in m-c and it also matches this file.

Comment 19

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Bug 1147561 - If no formSubmitURL matches, look for matches with different schemes
***
Fixed small issues with the last patch
***
removed nslogininfo.match last argument, added support for mozstorage, fixed tests for this new scenario
***
fixed if missing spaces
***
new test check if doorhanger don't open for different scheme formSubmitURL
***
removed trailing space
***
fixed last review issues

Comment 20

[Matthew N. [:MattN]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

https://reviewboard.mozilla.org/r/9203/#review8361

Great job! Please make sure tests still pass locally `mach test toolkit/components/passwordmgr/` (including with mozStorage) after fixing these issues then I think it's good to land! \o/

::: toolkit/components/passwordmgr/storage-json.js:343
(Diff revision 5)
> -                return false;
> +                // Check if it matches with different scheme

Nit: "a different scheme"

::: toolkit/components/passwordmgr/storage-json.js:352
(Diff revision 5)
> +                } catch(e) {

Nit: missing space after `catch`

::: toolkit/components/passwordmgr/storage-json.js:405
(Diff revision 5)
> +        // If protocol does not match, use as fallback login

Nit: missing word: "use as a fallback login"

::: toolkit/components/passwordmgr/storage-mozStorage.js:493
(Diff revision 5)
>          case "formSubmitURL":
>            if (value != null) {
> -              conditions.push("formSubmitURL = :formSubmitURL OR formSubmitURL = ''");
> -              params["formSubmitURL"] = value;
>                break;

Add a comment above the `break;` that this gets filtered handled by filtering the result of the query.

::: toolkit/components/passwordmgr/storage-mozStorage.js:551
(Diff revision 5)
> +        if (login.formSubmitURL == "" ||
> +           login.formSubmitURL == matchData["formSubmitURL"]) {
> -        logins.push(login);
> +            logins.push(login);
> -        ids.push(stmt.row.id);
> +            ids.push(stmt.row.id);

Unless RB is lying to me, the indentation looks wrong inside the `if` here

::: toolkit/components/passwordmgr/test/LoginTestUtils.jsm:78
(Diff revision 5)
> +  assertLoginListsMatches(actual, expected, ignorePassword, ignoreScheme) {
> +    Assert.ok(expected.every(e => actual.some(a => a.matches(e, ignorePassword, ignoreScheme))));
> +  },

Leftover `ignoreScheme` argument x2?

::: toolkit/components/passwordmgr/test/LoginTestUtils.jsm:78
(Diff revision 5)
> +  assertLoginListsMatches(actual, expected, ignorePassword, ignoreScheme) {
> +    Assert.ok(expected.every(e => actual.some(a => a.matches(e, ignorePassword, ignoreScheme))));

Why aren't you checking the length here? I think you're only checking for a subset as-implemented.

::: toolkit/components/passwordmgr/test/unit/test_logins_search.js:277
(Diff revision 5)
> +add_task(function test_search_different_scheme()

Nit: different scheme of what? You should specify what field you're referring to in the test name.

Comment 21

[Matthew N. [:MattN]]

Also, please cleanup the commit message from the squashing and make it more clear that the commit is related to the password manager by mentioning words like "login" or "password" somewhere. Ideally I should be able to read the commit message on m-c and have an idea of what code it's touching without being that familiar with that area.

Comment 22

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Comment 23

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Comment 24

[Matthew N. [:MattN]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

https://reviewboard.mozilla.org/r/9203/#review8375

Ship It!

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2614fbb8d385

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android mochitest failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=3265461&repo=fx-team

Comment 27

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/a0506dfd42cc

Comment 28

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26170df9ac1d

Comment 29

[Bernardo Rittmeyer [:rittme]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=db610dc02b1a

Comment 30

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Comment 31

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Comment 32

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Comment 33

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Comment 34

[Matthew N. [:MattN]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

https://reviewboard.mozilla.org/r/9203/#review8917

There are a few more small issues to address.

::: toolkit/components/passwordmgr/storage-json.js:352
(Diff revisions 6 - 8)
> +                      (loginURI.scheme != "http" && loginURI.scheme != "https" ) ||
> +                      (matchURI.scheme != "http" && matchURI.scheme != "https" )) {

Nit: There are extra spaces before ")"

::: toolkit/components/passwordmgr/storage-json.js:354
(Diff revisions 6 - 8)
>                      return returnValue; // not a match at all
>                    } else {

Nit: `else` after `return` isn't necessary and goes against the style rules. You can simply remove the else line and outdent the body that was inside the else section. Using the return to avoid nesting also makes it easier to break the long condition up and make it easier to comment:

`
if (loginURI.hostPort != matchURI.hostPort) {
  return returnValue; // not a match at all
}

if (loginURI.scheme != "http" || loginURI.scheme != "https" ||
    matchURI.scheme != "http" || matchURI.scheme != "https") {
  // not a match at all since we only fallback HTTP <=> HTTPS
  return returnValue;
}

returnValue.strictMatch = false; // not a strict match
`

Note that I also changed the scheme checking logic as I don't understand why you were using `&&`. Don't we want to return no match if any of the 4 conditions are false?

::: toolkit/components/passwordmgr/storage-json.js:348
(Diff revisions 6 - 8)
>                  try {
>                    let loginURI = Services.io.newURI(aLogin.formSubmitURL, null, null);
>                    let matchURI = Services.io.newURI(value, null, null);

I think we should be consistent with the mozStorage version and hide less errors by removing the try…catch and handling "javascript:" explicitly.

::: toolkit/components/passwordmgr/storage-mozStorage.js:732
(Diff revisions 6 - 8)
> +    let _countLoginsHelper = (function (hostname, formSubmitURL, httpRealm) {

Unless I'm misremembering, you don't need to wrap the function in braces to use .bind and we normally don't.

Instead of using bind, you can use arrow function syntax which "lexically binds the this value" for you:
`let _countLoginsHelper = (hostname, formSubmitURL, httpRealm) => {`

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

::: toolkit/components/passwordmgr/storage-mozStorage.js:761
(Diff revisions 6 - 8)
> +        let formSubmitURIobj = Services.io.newURI(formSubmitURL, null, null);

Nit: I think you have the "obj" suffix to differentiate from `formSubmitURL` but it's the convention (though not perfectly followed) that variables names with the URI suffix are nsIURIs while a URL suffix is for a string variable and therefore I don't think you need the `obj` suffix.

::: toolkit/components/passwordmgr/storage-mozStorage.js:759
(Diff revisions 6 - 8)
> +    if (resultLogins == 0 && formSubmitURL != null && formSubmitURL != "" ) {
> +      try {

Same here. I think you should just check formSubmitURL != "javascript:"

Nit: Also, you have an extra space before `)`.

Comment 35

[Bernardo Rittmeyer [:rittme]]

https://reviewboard.mozilla.org/r/9203/#review8923

> Nit: `else` after `return` isn't necessary and goes against the style rules. You can simply remove the else line and outdent the body that was inside the else section. Using the return to avoid nesting also makes it easier to break the long condition up and make it easier to comment:
> 
> `
> if (loginURI.hostPort != matchURI.hostPort) {
>   return returnValue; // not a match at all
> }
> 
> if (loginURI.scheme != "http" || loginURI.scheme != "https" ||
>     matchURI.scheme != "http" || matchURI.scheme != "https") {
>   // not a match at all since we only fallback HTTP <=> HTTPS
>   return returnValue;
> }
> 
> returnValue.strictMatch = false; // not a strict match
> `
> 
> Note that I also changed the scheme checking logic as I don't understand why you were using `&&`. Don't we want to return no match if any of the 4 conditions are false?

I'm not sure about this. If I'm not wrong the condition `(loginURI.scheme != "http" || loginURI.scheme != "https")` would always be true, because `loginURI.scheme` will always be different from one of both cases. For example `a != 1 || a != 2`, a can't be 1 and 2 at the same time, so it will always evaluate to true. 
I think what we want should be something like:
```
if (loginURI.scheme != "http" && loginURI.scheme != "https" ||
    matchURI.scheme != "http" && matchURI.scheme != "https") {
```
This way we would check if `loginURI` is neither http nor https and the same for `matchURI`.
Does this make sense?

> I think we should be consistent with the mozStorage version and hide less errors by removing the try…catch and handling "javascript:" explicitly.

After removing the try...catch I'm not able any more to pass one xpcshell test that says:
```
"Testing all search functions for {"formSubmitURL":"example.com"}"
```

I get an exception when trying to create the nsIURI object from `example.com`.
Do we have to expect this type of untreated value to get to this low level?

The test is at [test_logins_search.js](http://lxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/unit/test_logins_search.js#208) line 208, or 254 on my patch.

I think we can change this line to something like:
```
Assert.throws(() => checkAllSearches({ formSubmitURL: "example.com" }, 0), /NS_ERROR_MALFORMED_URI/);
```
But then I'm not sure we want to rely on this exception either.

Comment 36

[Bernardo Rittmeyer [:rittme]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

Comment 37

[Matthew N. [:MattN]]

Comment on attachment 8611591 [details]
MozReview Request: Bug 1147561 - Fallback to logins with the same hostPort but different scheme for the form action. r=MattN

https://reviewboard.mozilla.org/r/9203/#review9067

Ship It!

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ede3d5324f31

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ede3d5324f31

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ede3d5324f31

Comment 41

[Tanvi Vyas[:tanvi]]

https://reviewboard.mozilla.org/r/9203/#review9345

First time using mozreview, so please bare with me if I've checked a box I shouldn't have.

::: toolkit/components/passwordmgr/storage-mozStorage.js:773
(Diff revision 9)
> +    this.log("_countLogins: counted logins: " + resultLogins);

In the logging messages, you may want to distinguish between counted logins and counted fallback logins.

::: toolkit/components/passwordmgr/nsLoginInfo.js:57
(Diff revision 9)
> +        if (loginURI.hostPort != matchURI.hostPort) {

In general, I think its better to check loginURI.hostPort == matchURI.hostPORT and only then return true.  Check for success cases and return true, otherwise return false by default.  This code does the opposite, where it checks for failures and returns false, but returns true by default.

::: toolkit/components/passwordmgr/storage-mozStorage.js:562
(Diff revision 9)
> +          if (loginURI.hostPort == matchURI.hostPort &&

This is much cleaner than the implementation in storage-json.js

::: toolkit/components/passwordmgr/storage-json.js:334
(Diff revision 9)
> +        strictMatch: true

Why is strictMatch true by defult?  Shouldn't it be false by default and set to true only when we do have a strictMatch?

Comment 42

[Bernardo Rittmeyer [:rittme]]

Attachment: MozReview Request: Bug 1147561 - new test check if doorhanger don't open for different scheme formSubmitURL