395681 - Password manager dialogs observe topics that are never fired

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

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

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/passwordmgr/content/passwordManagerCommon.js&rev=1.4&mark=67-71#66

It listens for "signonChanged" and "signonSelectUser" topics that seem to only have been sent by wallet. The previous passwordmgr implementation didn't send them either, as far as I can tell, but we should either have the new password manager send them, or remove the code that watches for them. I think we want a way to detect newly added passwords or exceptions, and the existing signonChanged topic with "signons" or "rejects" data params should be fine with that. I don't think signonSelectUser applies to our current code, we should just remove it.

Comment 2

[Robert Kaiser]

Bug 571108 has a bit of data about how things are there nowadays, just as a note.

Comment 3

[Andres Hernandez [:andreshm]]

I can take a look at this one.

Comment 4

[Andres Hernandez [:andreshm]]

Attachment: Patch v1

Comment 5

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

Hmm, it looks like we might actually need to update this code to use passwordmgr-storage-changed, rather than just ripping it all out. It seems like for the moment, the password manager dialog doesn't get updated in response to changes to the DB (e.g. from Sync).

Comment 6

[Andres Hernandez [:andreshm]]

Attachment: Patch v2

Removed the signonSelectUser observer and updated the signonChanged with passwordmgr-storage-changed. Now, when syncing the password manager is updated.

Comment 7

[Drew Willcoxon :adw]

Comment on attachment 650366 [details] [diff] [review]
Patch v2

I don't know this code well enough to review.

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 650366 [details] [diff] [review]
Patch v2

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

Hey Andres, the code you've written looks good, there's just some more cleanup to be done still.

AFAICT gSelectUserInUse will never be set anymore with this patch so it's definition in this file and usage in passwordManager.js should be removed. https://mxr.mozilla.org/mozilla-central/search?string=gSelectUserInUse

Could you add tests for the notifications received in passwordManager.xul and passwordManagerExceptions.xul? See https://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/browser/ for some existing tests.

Thanks

::: toolkit/components/passwordmgr/content/passwordManagerCommon.js
@@ +54,5 @@
> +        case "modifyLogin":
> +        case "removeLogin":
> +        case "removeAllLogins":
> +          signons.length = 0;
> +          if (lastSignonSortColumn == "hostname") {

This common observer function is trying to do work for both passwordManagerExceptions.xul and passwordManager.xul even though it is only loaded in one or the other.  This leads to exceptions (which are normally not seen because they are in the observer function).  Add a try/catch block inside the function to log the exception and you will see this. This applies to both sets of cases. Luckily we didn't end up doing a lot of wasted work such as LoadSignons/LoadRejects because of the earlier exceptions.

One method to fix this is to do an early return in the cases based on the truthiness of signonsTree/rejectsTree as they indicate which dialog we're in.

The old code had the same problem but since you're going to hook this notification up again, we should fix it.

Exception in passwordManagerExceptions.xul from the notification:
Error: lastSignonSortColumn is not defined
Source File: chrome://passwordmgr/content/passwordManagerCommon.js
Line: 58

@@ +66,5 @@
> +          break;
> +        case "hostSavingEnabled":
> +        case "hostSavingDisabled":
> +          rejects.length = 0;
> +          if (lastRejectSortColumn == "hostname") {

Same here for passwordManager.xul:
Error: lastRejectSortColumn is not defined
Source File: chrome://passwordmgr/content/passwordManagerCommon.js
Line: 70

Comment 9

[Andres Hernandez [:andreshm]]

Attachment: Patch v3

Fixed the suggestions and added a new test.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 662370 [details] [diff] [review]
Patch v3

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

r=me with these fixes (assuming this is fine with a toolkit peer).

::: toolkit/components/passwordmgr/content/passwordManagerCommon.js
@@ +52,5 @@
> +        case "addLogin":
> +        case "modifyLogin":
> +        case "removeLogin":
> +        case "removeAllLogins":
> +          if (signonsTree) {

An early return is preferred as it reduces the number of levels of indentation making it easier to follow the scoping:
if (!signonsTree) {
  return;
}

@@ +66,5 @@
> +          }
> +          break;
> +        case "hostSavingEnabled":
> +        case "hostSavingDisabled":
> +          if (rejectsTree) {

same as above

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_observers.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

FYI test files can be licensed to the public domain (see https://www.mozilla.org/MPL/headers/ )

@@ +5,5 @@
> +function test() {
> +  waitForExplicitFinish();
> +
> +  let LOGIN_HOST = "http://example.com";
> +  let LOGIN_COUNT = 5;

Nit: make these two variables const to match the naming scheme and prevent accidental changes.

@@ +15,5 @@
> +      if (topic == "passwordmgr-storage-changed") {
> +        switch (data) {
> +          case "addLogin":
> +            // wait for signons to load
> +            setTimeout(function() {

Replace setTimeout usage with SimpleTest.executeSoon in this test file.

Comment 11

[Andres Hernandez [:andreshm]]

Attachment: Patch v4

Updated patch.

Comment 12

[Andres Hernandez [:andreshm]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=2077c6174bf2

Comment 13

[Andres Hernandez [:andreshm]]

All good, just a couple of oranges. See: https://tbpl.mozilla.org/?tree=Try&rev=2077c6174bf2

Comment 14

[Matthew N. [:MattN]]

Comment on attachment 663615 [details] [diff] [review]
Patch v4

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

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_observers.js
@@ +10,5 @@
> +  let loginCounter = 0;
> +  let testNumber = 0;
> +  let testObserver = {
> +    observe: function (subject, topic, data) {
> +      if (topic == "passwordmgr-storage-changed") {

To avoid intermittent failures, I now think we should use a new observer notification to know when the dialogs have reacted to passwordmgr-storage-changed.  I propose that this test listen for passwordmgr-dialog-updated and we fire that at the end of signonReloadDisplay.observe. The executeSoons's can then be removed.  Apologies for telling you to switch to executeSoon now that it will go away.

Comment 15

[Andres Hernandez [:andreshm]]

Attachment: Patch v5

Added observer and test fixes.

Comment 16

[Andres Hernandez [:andreshm]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=f42347e552b0

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 664261 [details] [diff] [review]
Patch v5

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

::: toolkit/components/passwordmgr/content/passwordManagerCommon.js
@@ +64,5 @@
> +          // apply the filter if needed
> +          if (document.getElementById("filter") && document.getElementById("filter").value != "") {
> +            _filterPasswords();
> +          }
> +          kObserverService.notifyObservers(null, "passwordmgr-dialog-updated", data);

The two notifyObservers calls can be combined below the switch to avoid duplication. It seems a bit weird to re-use |data| here so we should change the 3rd argument to null and then update the test to switch on |testNumber|.

::: toolkit/components/passwordmgr/test/browser/browser_passwordmgr_observers.js
@@ +22,5 @@
> +  let testObserver = {
> +    observe: function (subject, topic, data) {
> +      if (topic == "passwordmgr-dialog-updated") {
> +        switch (data) {
> +          case "addLogin":

switch (testNumber) {

Comment 18

[Andres Hernandez [:andreshm]]

Attachment: Patch v6

Updated patch.

Comment 19

[Matthew N. [:MattN]]

Comment on attachment 664531 [details] [diff] [review]
Patch v6

r=me (dolske approved)

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/0787c32d87f3

Comment 20

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/0787c32d87f3