1288557 - Replace custom exceptions dialog (passwordManagerExceptions.xul) with usage of permissions.xul

Status: RESOLVED FIXED

(Toolkit :: Password Manager, enhancement, P3)

Description

[Matthew N. [:MattN]]

Bug 1058438 switches password manager to use permission manager to store exceptions like most other modules instead of custom storage.

This means we can replace our custom management UI with the generic and flexible permissions.xul.

This means:
1) changing showPasswordExceptions[1] to use
> gSubDialog.open("chrome://browser/content/preferences/permissions.xul", …) //[2]
2) Removing passwordManagerExceptions.xul, passwordManagerExceptions.js and any strings that becomes orphaned by that.
3) Cleanup some leftover code in passwordManagerCommon.js e.g. `rejects` and `rejectsTree`. I'll file a separate follow-up to actually merge passwordManagerCommon.js into passwordManager.js so we can make incremental progress in this bug.

[1] https://dxr.mozilla.org/mozilla-central/search?q=showPasswordExceptions
[2] https://dxr.mozilla.org/mozilla-central/search?q=gSubDialog.open+permissions.xul&redirect=false

Comment 1

[pushpankark]

How do i know which strings are orphaned by step 1?

Comment 2

[Matthew N. [:MattN]]

(In reply to pushpankark from comment #1)
> How do i know which strings are orphaned by step 1?

You can search for the string's ID on https://dxr.mozilla.org/mozilla-central/ to see if it's used in a context other than the one you will delete.

Comment 3

[pushpankark]

Attachment: permission-fix1.patch

I am not sure that i am doing it right. Is this ok?

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8775053 [details] [diff] [review]
permission-fix1.patch

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

::: browser/components/preferences/in-content/security.js
@@ +127,5 @@
>     * where passwords are never saved.
>     */
>    showPasswordExceptions: function ()
>    {
> +    gSubDialog.open("chrome://browser/content/preferences/permissions.xul");

You're not passing the appropriate arguments to permissions.xul. See other examples in this directory. You need to have it open for the "login-saving" permission but since bug 1058438 got backed out you need to apply those patches first to actually test this.

::: toolkit/components/passwordmgr/jar.mn
@@ -7,5 @@
>      content/passwordmgr/login.xml                      (content/login.xml)
>  *   content/passwordmgr/passwordManager.xul            (content/passwordManager.xul)
>      content/passwordmgr/passwordManager.js             (content/passwordManager.js)
> -    content/passwordmgr/passwordManagerExceptions.js   (content/passwordManagerExceptions.js)
> -    content/passwordmgr/passwordManagerExceptions.xul  (content/passwordManagerExceptions.xul)

You should actually delete these files too (e.g. git/hg's `rm` command)

Comment 5

[pushpankark]

In showAddonException in security.js "gSubDialog.open(...." is being called with the "null" and "param" but in showPasswordException there is no "params". Should i create one similar to that or I should pass null. (Sorry for disturbing you so much on a simple bug)

Comment 6

[Matthew N. [:MattN]]

(In reply to pushpankark from comment #5)
> In showAddonException in security.js "gSubDialog.open(...." is being called
> with the "null" and "param" but in showPasswordException there is no
> "params". Should i create one similar to that or I should pass null. (Sorry
> for disturbing you so much on a simple bug)

You need to pass the params like https://dxr.mozilla.org/mozilla-central/rev/db3ed1fdbbeaf5ab1e8fe454780146e7499be3db/browser/components/preferences/in-content/privacy.js#565. The old dialog didn't need params since it was specific to password exceptions. permissions.xul is generic and supports many configurations and any permission type so you need to pass appropriate options.

Comment 7

[Steve Chung [:steveck]]

Hi Matt, the permission.xul and js seem not very general original, because all the behaviors are bound with permission.js, and exception for password part works differently. Should we integrate the logic into the permission.js, or still leave the logic into different js and create a general js for handling the different type of dialogs?

Comment 8

[Matthew N. [:MattN]]

(In reply to Steve Chung [:steveck] from comment #7)
> Hi Matt, the permission.xul and js seem not very general original,
It's very general in my opinion…

> because
> all the behaviors are bound with permission.js, and exception for password
> part works differently.

No they don't, it uses the "login-saving" permission so I don't see the problem.

> Should we integrate the logic into the
> permission.js, or still leave the logic into different js and create a
> general js for handling the different type of dialogs?

permission.js should already have all the logic we need. If you don't agree, please elaborate on the problem.

Comment 9

[Steve Chung [:steveck]]

Attachment: bug-1288557.patch

Hi Matt, you are right because I didn't figure out the permission dialog also support saving login type as well, and missed the permission type while I started the patch. Now it seems works with proper parameters, but I still faced some problem in the test :/ It seems stuck after setting the host list by Services.logins.setLoginSavingEnabled. The observer didn't get the "passwordmgr-dialog-updated" event at the correct timing. And the permission.xul might need the passwordManagerCommon.js, otherwise it might have further problems if we close the password dialog before starting the exception dialog test.

Comment 10

[Matthew N. [:MattN]]

Comment on attachment 8786701 [details] [diff] [review]
bug-1288557.patch

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

Hi Steve, I'm glad you figured out how permissions.xul works now. Could you please use MozReview for future patches? https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html

(In reply to Steve Chung [:steveck] from comment #9)
> Now it seems works with proper parameters, but I still
> faced some problem in the test :/ It seems stuck after setting the host list
> by Services.logins.setLoginSavingEnabled. The observer didn't get the
> "passwordmgr-dialog-updated" event at the correct timing.

OK, I understand what's going on. The new dialog doesn't sent that notification but I think that's fine as we can just make a new test for permissions.xul that doesn't rely on it. Here's what I would suggest:
1) Move references to pmexDialog from browser_passwordmgr_observers.js to a new file in that directory: browser_exceptions_dialog.js (and add it to browser.ini)
2) Write browser_exceptions_dialog.js using add_task (modern style) to test the two cases: setLoginSavingEnabled(LOGIN_HOST, false); and setLoginSavingEnabled(LOGIN_HOST, true); like the following pseudo code:
```
const LOGIN_HOST = "http://example.com";

openExceptionsDialog() {
  let pmexDialog = window.openDialog(…);
}

add_task(function* test_disable() {
  openExceptionsDialog();
  let promiseChanged = TestUtils.topicObserved("passwordmgr-storage-changed",
                                               (subject, data) => {
                                                 return data == "hostSavingDisabled" &&
                                                        subject.prePath == LOGIN_HOST;
                                               });
  setLoginSavingEnabled(LOGIN_HOST, false);
  yield promiseStorageChanged; // I didn't look closely but I believe we're guaranteed that this will happen after perm-change which is what permission.xul listens for.

  is(countDisabledHosts(), 1, "Verify disabled host added");
});

add_task(function* test_enable() {
  // similar to above but with `setLoginSavingEnabled(LOGIN_HOST, true);` and "hostSavingEnabled"
});
```
 

> And the
> permission.xul might need the passwordManagerCommon.js, otherwise it might
> have further problems if we close the password dialog before starting the
> exception dialog test.

I'm not sure what you mean by this. Which part of passwordManagerCommon.js do you think we may need?


Overall this is looking very good. Great work!

::: browser/components/preferences/in-content/security.js
@@ +130,5 @@
> +    var params = this._loginSavingParams;
> +
> +    if (!params.windowTitle || !params.introText) {
> +      var bundlePrefs = document.getElementById("bundlePreferences");
> +      params.windowTitle = bundlePrefs.getString("savedLoginsExceptions_title");

I don't think it's worth memoizing the string value as other consumers that I know of don't seem to do this (e.g. privacy.js), it's not very slow, and it's not a hot code path. Please move the params into a temporary function scoped object like other consumers and inline the getString calls on the object literal defintion.

Btw. I see now you got this idea from `this._addonParams` in this file and that dates back to hg@1…

@@ +143,5 @@
> +   * Parameters for the login saving permissions dialog.
> +   */
> +  _loginSavingParams:
> +  {
> +    blockVisible: false,

I think it will be nice to have this visible now since we basically get it for free

@@ +146,5 @@
> +  {
> +    blockVisible: false,
> +    sessionVisible: false,
> +    allowVisible: false,
> +    prefilledHost: "",

Adding `hideStatusColumn: true,` will make it more like the old dialog

::: toolkit/components/passwordmgr/content/passwordManagerCommon.js
@@ +33,5 @@
>    kObserverService = Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService);
>    kObserverService.addObserver(signonReloadDisplay, "passwordmgr-storage-changed", false);
>  
>    signonsTree = document.getElementById("signonsTree");
> +  rejectsTree = document.getElementById("permissionsTree");

Please remove this and any other variables in this file which are specific to the rejects/exceptions dialog. Probably any code mentioning "reject" should be removed.

Bug 1288558 will handle merging the file into passwordManager.js though so don't worry about that stuff in this bug.

Comment 11

[Steve Chung [:steveck]]

Attachment: Bug 1288557 - Part 1: Replace custom exceptions dialog with usage of permissions.

Review commit: https://reviewboard.mozilla.org/r/76010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76010/

Comment 12

[Steve Chung [:steveck]]

This patch still has some problem while receiving the events. For the "hostSavingDisabled"event the subject will be an empty object, for the remove case there will be a "removeAllLogins" event instead of "hostSavingEnabled".

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 8787154 [details]
Bug 1288557 - Part 1: Replace custom exceptions dialog with usage of permissions.

https://reviewboard.mozilla.org/r/76010/#review74196

Really close but not marking r+ yet because there are quite a few test issues to fix.

::: browser/components/preferences/in-content/security.js:146
(Diff revision 1)
> +  /**
> +   * Parameters for the login saving permissions dialog.
> +   */
> +  _loginSavingParams:
> +  {
> +    blockVisible: true,
> +    sessionVisible: false,
> +    allowVisible: false,
> +    hideStatusColumn: true,
> +    prefilledHost: "",
> +    permissionType: "login-saving"
>    },

Looks like you forgot to delete this.

::: toolkit/components/passwordmgr/test/browser/browser.ini:59
(Diff revision 1)
>  [browser_passwordmgr_fields.js]
>  [browser_passwordmgr_observers.js]
>  [browser_passwordmgr_sort.js]
>  [browser_passwordmgr_switchtab.js]
>  [browser_passwordmgrdlg.js]
> +[browser_exceptions_dialog.js]

Please insert this new test at alphabetical order

::: toolkit/components/passwordmgr/test/browser/browser_exceptions_dialog.js:1
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +

Nit: Test files are public domain by default (for the lasy few years) so you don't need to put this on new test files.

::: toolkit/components/passwordmgr/test/browser/browser_exceptions_dialog.js:4
(Diff revision 1)
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +'use strict';

Nit: Use double quotes

::: toolkit/components/passwordmgr/test/browser/browser_exceptions_dialog.js:24
(Diff revision 1)
> +  let doc = dialog.document;
> +  let rejectsTree = doc.getElementById("permissionsTree");
> +  return rejectsTree.view.rowCount;

After fixing promiseStorageChanged I get "browser_exceptions_dialog.js:26 - TypeError: rejectsTree is null",

This is because this is running too early and giving the about:blank document.

I think we should make openExceptionsDialog return a promise that resolves to the dialog window when it has finished loading. Then rename it promiseExceptionsDialogs. You can use/return BrowserTestUtils.waitForEvent to handle the load.

::: toolkit/components/passwordmgr/test/browser/browser_exceptions_dialog.js:29
(Diff revision 1)
> +function promiseStorageChanged() {
> +  return TestUtils.topicObserved(
> +    "passwordmgr-storage-changed",
> +    (subject, data) => {
> +      return (data == "hostSavingDisabled" || data == "hostSavingEnabled") &&
> +             subject.prePath == LOGIN_HOST;

We should be explcit about which of the two data values we expect in each case. You can make it an argument to this function and pass the appropriate string for the two callers.

::: toolkit/components/passwordmgr/test/browser/browser_exceptions_dialog.js:30
(Diff revision 1)
> +  return TestUtils.topicObserved(
> +    "passwordmgr-storage-changed",

We don't normally use this style of wrapping in new code, please keep the first argument on the same line as the method name. If you don't like how the 2nd argument would wrap then you can move it to a named private function local to promiseStorageChaged

::: toolkit/components/passwordmgr/test/browser/browser_exceptions_dialog.js:34
(Diff revision 1)
> +function promiseStorageChanged() {
> +  return TestUtils.topicObserved(
> +    "passwordmgr-storage-changed",
> +    (subject, data) => {
> +      return (data == "hostSavingDisabled" || data == "hostSavingEnabled") &&
> +             subject.prePath == LOGIN_HOST;

See https://dxr.mozilla.org/mozilla-central/rev/b7f7ae14590aced450bb0b0469dfb38edd2c0ace/toolkit/components/passwordmgr/test/unit/test_notifications.js#37 and https://dxr.mozilla.org/mozilla-central/rev/b7f7ae14590aced450bb0b0469dfb38edd2c0ace/toolkit/components/passwordmgr/nsLoginManager.js#465

I was wrong in my example code about subject being an nsIURI, it's actually an nsISupportsString but you need to QueryInterface it since the observer interface defines it as simply nsISupports:

`subject.QueryInterface(Ci.nsISupportsString).data == LOGIN_HOST`

::: toolkit/components/passwordmgr/test/browser/browser_exceptions_dialog.js:45
(Diff revision 1)
> +  is(countDisabledHosts(dialog), 1, "Verify disabled host added");
> +});
> +
> +add_task(function* test_enable() {
> +  let dialog = openExceptionsDialog();

You should close the dialog at the end of each task:

`yield BrowserTestUtils.closeWindow(dialog);`

::: toolkit/components/passwordmgr/test/browser/browser_exceptions_dialog.js:54
(Diff revision 1)
> +  let dialog = openExceptionsDialog();
> +  let promiseChanged = promiseStorageChanged();
> +
> +  Services.logins.setLoginSavingEnabled(LOGIN_HOST, true);
> +  yield promiseChanged;
> +  is(countDisabledHosts(dialog), 1, "Verify disabled host removed");

This should be a 0, not a 1 but it gives a 1 currently because your test found a real bug in permissions.js!

Can you add a separate commit before this one to change the line:
`this._removePermissionFromList(permission);`
to
`this._removePermissionFromList(permission.principal);`

and I can review that as well.

Comment 14

[Steve Chung [:steveck]]

Attachment: Bug 1288557 - Part 0: Fix permissions dialog while receiving deleted event.

Review commit: https://reviewboard.mozilla.org/r/76234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/76234/

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8787154 [details]
Bug 1288557 - Part 1: Replace custom exceptions dialog with usage of permissions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76010/diff/1-2/

Comment 16

[Matthew N. [:MattN]]

Comment on attachment 8787501 [details]
Bug 1288557 - Part 0: Fix permissions dialog while receiving deleted event.

https://reviewboard.mozilla.org/r/76234/#review74330

Nit: replace the word "bug" with the word "dialog" in your commit message to make it more descriptive.

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8787154 [details]
Bug 1288557 - Part 1: Replace custom exceptions dialog with usage of permissions.

https://reviewboard.mozilla.org/r/76010/#review74332

Note that your try push should have request xpcshell and mochitests, not marionette. I've added some mochitest jobs to your push but not on all platforms.

Comment 18

[Steve Chung [:steveck]]

Comment on attachment 8787501 [details]
Bug 1288557 - Part 0: Fix permissions dialog while receiving deleted event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76234/diff/1-2/

Comment 19

[Steve Chung [:steveck]]

Comment on attachment 8787154 [details]
Bug 1288557 - Part 1: Replace custom exceptions dialog with usage of permissions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/76010/diff/2-3/

Comment 20

[Steve Chung [:steveck]]

I updated the commit message, but the build seems failed in L10n, not sure if it related to the patch... Anyway, thanks for all the review!

Comment 21

[Matthew N. [:MattN]]

It seems infra related but is also tier 2 so I think we can ignore it.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df8f6646de9e
Part 0: Fix permissions dialog while receiving deleted event. r=MattN
https://hg.mozilla.org/integration/autoland/rev/2fbd7f36b1cd
Part 1: Replace custom exceptions dialog with usage of permissions. r=MattN

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/df8f6646de9e
https://hg.mozilla.org/mozilla-central/rev/2fbd7f36b1cd