Closed Bug 395681 Opened 12 years ago Closed 7 years ago

Password manager dialogs observe topics that are never fired

Categories

(Toolkit :: Password Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Gavin, Assigned: andreshm)

References

()

Details

(Whiteboard: [mentor=gavin][lang=js])

Attachments

(1 file, 5 obsolete files)

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.
Product: Firefox → Toolkit
Whiteboard: [ui]
Depends on: 475634
Duplicate of this bug: 571108
Bug 571108 has a bit of data about how things are there nowadays, just as a note.
Whiteboard: [ui] → [mentor=gavin]
Whiteboard: [mentor=gavin] → [mentor=gavin][lang=js]
I can take a look at this one.
Assignee: nobody → andres
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #644965 - Flags: review?(gavin.sharp)
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).
Attachment #644965 - Flags: review?(gavin.sharp) → review-
Status: NEW → ASSIGNED
Attached patch Patch v2 (obsolete) — Splinter Review
Removed the signonSelectUser observer and updated the signonChanged with passwordmgr-storage-changed. Now, when syncing the password manager is updated.
Attachment #644965 - Attachment is obsolete: true
Attachment #650366 - Flags: review?(gavin.sharp)
Attachment #650366 - Flags: review?(gavin.sharp) → review?(adw)
Comment on attachment 650366 [details] [diff] [review]
Patch v2

I don't know this code well enough to review.
Attachment #650366 - Flags: review?(adw) → review?(gavin.sharp)
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
Attachment #650366 - Flags: review?(gavin.sharp) → review-
Attached patch Patch v3 (obsolete) — Splinter Review
Fixed the suggestions and added a new test.
Attachment #650366 - Attachment is obsolete: true
Attachment #662370 - Flags: review?(mnoorenberghe+bmo)
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.
Attachment #662370 - Flags: review?(mnoorenberghe+bmo) → review+
Attached patch Patch v4 (obsolete) — Splinter Review
Updated patch.
Attachment #662370 - Attachment is obsolete: true
Attachment #663615 - Flags: review?(mnoorenberghe+bmo)
All good, just a couple of oranges. See: https://tbpl.mozilla.org/?tree=Try&rev=2077c6174bf2
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.
Attachment #663615 - Flags: review?(mnoorenberghe+bmo) → feedback-
Attached patch Patch v5 (obsolete) — Splinter Review
Added observer and test fixes.
Attachment #663615 - Attachment is obsolete: true
Attachment #664261 - Flags: review?(mnoorenberghe+bmo)
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) {
Attachment #664261 - Flags: review?(mnoorenberghe+bmo) → review-
Attached patch Patch v6Splinter Review
Updated patch.
Attachment #664261 - Attachment is obsolete: true
Attachment #664531 - Flags: review?(mnoorenberghe+bmo)
https://hg.mozilla.org/mozilla-central/rev/0787c32d87f3
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 834000
You need to log in before you can comment on or make changes to this bug.