442150 - "Backup all" button not enabled after cert import

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[Marcia Knous [:marcia]]

Attachment: Screenshot of issue using Firefox 3

Seen while testing  Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9) Gecko/2008053008 Firefox/3.0.

STR:
1. Install multiple certs in Firefox.
2. Observe the attached screenshot. I would expect the "Backup all" button to be available.

The one note I wanted to make is I think the first cert is expired. I don't know if that makes any difference.

Comment 1

[Johnathan Nightingale [:johnath]]

I'm not seeing this on my machine ("Backup all..." is enabled) with three valid certs... MXR suggests this line is to blame, one way or another:

http://mxr.mozilla.org/mozilla/source/security/manager/pki/resources/content/certManager.js#108

It sort of seems like, in your case, the treeview is reporting 0 rows even though there are clearly rows present?  Kai knows this code better than I do.

Comment 2

[Johnathan Nightingale [:johnath]]

It would be helpful to know if anything shows up in your js error console when you open this dialog (assuming "javascript.options.showInConsole" is set to true).

Comment 3

[Marcia Knous [:marcia]]

Oddly enough, when I started running my machine today and check the cert panel, the "Backup All" button is now enabled. I did restart the browser several times. I will resolve this as WFM for now but I will keep an eye out if it happens again.

Comment 4

[Marcia Knous [:marcia]]

https://litmus.mozilla.org/show_test.cgi?id=4208 should cover this scenario if you have multiple certs installed.

Comment 5

[Eddy Nigg (StartCom)]

The screen shot shows that you have selected a specific certificate. In that case the "Backup All' button is disabled. Deselect any of the selected certificates (or simply don't select any in your "Your Certificates" store) and 'Backup All" will be back. 

Comment 6

[Kai Engert (:KaiE:)]

Thanks Eddy.
Maybe backup-all should always be enabled, even if you have selected something.

Comment 7

[Marcia Knous [:marcia]]

I agree with Kai in Comment 6 - it makes more sense to have it enabled even if something is selected.

Comment 8

[Eddy Nigg (StartCom)]

(In reply to comment #6)
> Maybe backup-all should always be enabled, even if you have selected something.
> 

Not sure. If one hits "Backup All" unintentionally, it could lead to some unwanted surprises maybe.

Comment 9

[Kai Engert (:KaiE:)]

Johnathan, what's your opinion? See comment 6 and following.

Comment 10

[Johnathan Nightingale [:johnath]]

(In reply to comment #9)
> Johnathan, what's your opinion? See comment 6 and following.

I think my opinion is contingent on whether Eddy's diagnosis is correct.  I suspected there might have been multi-select foo at work here when I saw Marcia's report, but I tried multi-selecting (on Mac) and the Backup All button remained.  I tried selecting multiple certs, multiple of the CA meta-entries, and combinations, and in all cases, the Backup All button remained.  What am I missing here?

I do think that "backup all" should always backup _all_, and that subset-backups can be supported through multi-selecting and clicking the "backup..." button.  But I'm not sure that's even the bug under discussion until we can confirm that that is the cause.  :)

Comment 11

[Marcia Knous [:marcia]]

Attachment: Screenshot of cert dialog after initial certificate install

I think I see what may be going on. While testing, if I install multiple certs and don't select any, the "Backup All" is not enabled after the initial install. As soon as I switch tabs away from "Your Certificates" and then go back to the tab, then the Backup All button is enabled. So that may be the bug that I am seeing, and may explain why when I came back after a restart the button was enabled.

Comment 12

[Johnathan Nightingale [:johnath]]

Yep, that's entirely plausible!  Kai, I bet that after installing the cert, LoadCerts isn't being called again to reset the button enablement?

I think we should leave the semantic meaning of "Backup All..." as-is, but we should fix the stale-disabled-state bug here.  Would you agree?

Comment 13

[Kai Engert (:KaiE:)]

(In reply to comment #12)
> I think we should leave the semantic meaning of "Backup All..." as-is, but we
> should fix the stale-disabled-state bug here.  Would you agree?

Yes, I agree

Comment 14

[Kai Engert (:KaiE:)]

(In reply to comment #12)
> Kai, I bet that after installing the cert,
> LoadCerts isn't being called again to reset the button enablement?

LoadCerts is a very expensive function call, which can take many seconds to complete.

We should not call it after importing.

We should create a separate function that does the button enabling etc. and call it after importing.

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Comment 16

[Johnathan Nightingale [:johnath]]

Comment on attachment 328472 [details] [diff] [review]
Patch v1

>diff --git a/security/manager/pki/resources/content/certManager.js b/security/manager/pki/resources/content/certManager.js
>+  enableBackupAllButton();
>+}
>+
>+function enableBackupAllButton()
>+{
>   var rowCnt = userTreeView.rowCount;
>   var enableBackupAllButton=document.getElementById('mine_backupAllButton');
>   if(rowCnt < 1) {
>     enableBackupAllButton.setAttribute("disabled",true);
>   } else  {
>     enableBackupAllButton.setAttribute("enabled",true);

I know this is the code as-is, but I find the variable name "enableBackupAllButton" confusing, since it refers to a button element, not a boolean about what to do WITH that button.  If you feel like it, changing this to just be "backupAllButton" would seem preferable.

Looks good to me - do we want to add a test here to make sure this doesn't happen again?  I don't honestly know what PSM's unit-testing expectations are, if any?

Comment 17

[Kai Engert (:KaiE:)]

Attachment: Patch v2

I like your proposal for an additional reason, because my v1 patch would introduce an ambiguity between function and variable names.

Comment 18

[Johnathan Nightingale [:johnath]]

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

I haven't tested this patch, but it looks good.

Comment 19

[Kai Engert (:KaiE:)]

(In reply to comment #18)
> I haven't tested this patch, but it looks good.


Sorry, I should have tested before submitting.
My testing has now shown the patch does not work.

I think it's because "enabled" and "disabled" are separate attributes.

When entering the dialog with zero certs, 
it sets disabled=true

After importing a cert,
it sets enabled=true

But that doesn't enable the button...


I've changed the code to always modify attribute "disabled", setting it to either true or false. That makes the patch work.


During testing I thought of one more scenario: When deleting the last cert the backup-all button should go to disabled.
I'll add a call to the new function whenever we delete a personal cert.

Comment 20

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Comment 21

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

.removeAttribute("disabled") is a more reliable way of enabling something (many bindings only check for the presence of the attribute, ignoring its value).

Comment 22

[Johnathan Nightingale [:johnath]]

(In reply to comment #20)
> Created an attachment (id=328571) [details]
> Patch v3
> 

What Gavin said.  :)  (And I should have caught.  :(  Bad johnath, no cookie.)

Comment 23

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

Actually, you should just use the property "button.disabled = true/false", which is even more reliable!

Comment 24

[Johnathan Nightingale [:johnath]]

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

Gavin's comments here are right on - let's do this as reliably as possible.

Comment 25

[Kai Engert (:KaiE:)]

Attachment: Patch v4

So instead of 

  if(rowCnt < 1) {
    backupAllButton.setAttribute("disabled",true);
  } else  {
    backupAllButton.setAttribute("disabled",false);
  }


you want this

  if(rowCnt < 1) {
    backupAllButton.disabled = true;
  } else  {
    backupAllButton.disabled = false;
  }

right?

Patch updated

Comment 26

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

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

>diff --git a/security/manager/pki/resources/content/certManager.js b/security/manager/pki/resources/content/certManager.js

>+function enableBackupAllButton()

>   if(rowCnt < 1) {
>+    backupAllButton.disabled = true;
>   } else  {
>+    backupAllButton.disabled = false;
>   }

This could just be:

backupAllButton.disabled = rowCnt < 1;

Kind of silly that this took me so long to get to, sorry about that. This UI seems kind of broken in general (collapsable treeview means the buttons only work when child rows are selected, etc.). We should probably fix it at some point!

Comment 27

[Kai Engert (:KaiE:)]

(In reply to comment #26)
> backupAllButton.disabled = rowCnt < 1;

fixed with Gavin's change proposal added.

http://hg.mozilla.org/mozilla-central/rev/ff199a2f3ab4