373525 - "Certificate Manager" opens with wrong tab selected, and inconsistent content displayed

Status: VERIFIED FIXED

(Core Graveyard :: Security: UI, defect, P5)

Description

[u60234]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a3pre) Gecko/20070310 Minefield/3.0a3pre

When I open the Certificate Manager, the "Authorities" tab is selected but the content in the displayed panel is that of the "Your Certificates" tab. Switching tabs restores the window until it is closed and opened again.

Steps to reproduce:
1. In Options/Preferences -> Advanced -> Encryption, click on the "View Certificates" button to open the Certificate Manager.

Actual results:
The "Authorities" tab is selected, but the installed client certificates are shown.

Expected results:
The "Your Certificates" tab is selected and the installed client certificates are shown.

This regressed between the trunk builds from 2007-02-19-04 and 2007-02-20-05, so it is possibly related to bug 370742.
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-02-19+03%3A00%3A00&maxdate=2007-02-20+05%3A00%3A00&cvsroot=%2Fcvsroot

Comment 1

[u60234]

There is a attribute 'selected="true"' set for the CA tab in certManager.xul that has been there since the initial checkin, but as far as I could tell have never worked. I have tried builds as old as Mozilla 1.0 and they all open Cert manager with the client cert tab selected.

Removing the 'selected="true"' attribute seems to fix this bug, but I don't know if that is the right fix. Should the focus be on the CA tab when Cert manager opens?

Comment 2

[Neil Deakin]

This is a bug in tabbox.xml because the selectedIndex setter returns early after checking '!tabs[val].selected' which will be true, yet the selected panel is not changed.

Comment 3

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/2007042901 SeaMonkey/1.5a] (suiterunner, tinderbox-builds) (W2Ksp4)

Same bug with suiterunner:
first install, "Authorities" tab is selected and displayed blank (with new/default profile), switching tabs restores its content.

Quite misleading !

Comment 4

[Kaspar]

Attachment: change selectedIndex setter for tabbox

(In reply to comment #2)
> This is a bug in tabbox.xml because the selectedIndex setter returns early
> after checking '!tabs[val].selected' which will be true, yet the selected
> panel is not changed.

Would a change like this be ok, then? That does the trick for me.

If this fix seems acceptable, whom should I ask for review?

Comment 5

[Kaspar]

Attachment: Separate improvement: make selected tab in Cert Manager persistent

(In reply to comment #1)
> Removing the 'selected="true"' attribute seems to fix this bug, but I don't
> know if that is the right fix. Should the focus be on the CA tab when Cert
> manager opens?

I think that's ok when the Cert Manager is opened for the very first time (because the CA tab is probably the only one which is non-empty in this case).

Afterwards, however, I would say it makes more sense to remember the last selected tab, which can be achieved by the attached patch (make selectedIndex persistent for this tabbox). Kai, do you agree?

Comment 6

[Kai Engert (:KaiE:)]

Comment on attachment 267843 [details] [diff] [review]
Separate improvement: make selected tab in Cert Manager persistent

I agree with this idea and the patch. Thanks Kaspar. 

But just to repeat the obvious, this is separate from the regression bug.

Comment 7

[Kai Engert (:KaiE:)]

Comment on attachment 267842 [details] [diff] [review]
change selectedIndex setter for tabbox

I tested this patch and it fixes the bug for me.

(Please note, if you happen to run into a weird display and assertions while working with cert manager, please apply attachment 267909 [details] [diff] [review] from bug 383969.)

Neil, can you please review, or should somebody else?

Are additional reviews required?

Thanks.

Comment 8

[Kai Engert (:KaiE:)]

Adding dependency to bug 370724 which is suspected as having caused this regression.

Comment 9

[Neil Deakin]

Comment on attachment 267842 [details] [diff] [review]
change selectedIndex setter for tabbox

This patch will fire the select event even when setting selectedIndex to the same value.

Comment 10

[Kai Engert (:KaiE:)]

(In reply to comment #9)
> (From update of attachment 267842 [details] [diff] [review])
> This patch will fire the select event even when setting selectedIndex to the
> same value.

Do you know a better solution?
Who could help?

As this is a regression, we should either have a fix soon, or back out the original patch.

Comment 11

[Neil Deakin]

It should just not fire the events in that case, or implement a _suppressOnSelect feature which is set during the constructor around the selectedIndex call (listbox and some other elements have this for certain cases)

But if you need the fix very soon, you can check this one in, and file a followup bug on improving it.

Comment 12

[Kaspar]

Attachment: change selectedIndex setter for tabbox, patch v2

(In reply to comment #11)
> It should just not fire the events in that case, or implement a
> _suppressOnSelect feature which is set during the constructor around the
> selectedIndex call (listbox and some other elements have this for certain
> cases)

Ok, so from what I understand... would this be the correct patch, then? This also fixes the bug (with Cert Manager), but only fires the event if necessary.

Comment 13

[Neil Deakin]

Comment on attachment 267972 [details] [diff] [review]
change selectedIndex setter for tabbox, patch v2

No, you would need to check tabs[val].selected near the beginning of the function before it gets changed, store the state in a variable, then check it afterwards.

Also, the tabpanels.selectedIndex setter fires an event.

Is there a timeline on when this is needed for? I can take a better look later in the week if desired.

Comment 14

[Kaspar]

(In reply to comment #13)
> (From update of attachment 267972 [details] [diff] [review])
> No, you would need to check tabs[val].selected near the beginning of the
> function before it gets changed, store the state in a variable, then check it
> afterwards.

Well, seems like I should better leave this to someone who is actually familiar with tabbox.xml... ;-)

But anyway, out of curiosity - am I missing something? As far as I can see, neither val nor tabs[val].selected get changed anywhere in this function (only tabs[val]._selected), so why would an extra variable be needed to store the state?

> Also, the tabpanels.selectedIndex setter fires an event.

Yes, but this is actually what really *fixes* the bug :-) If

   tabpanels.selectedIndex = val;

doesn't get executed (and if the event is not fired), Cert Manager still opens with the wrong content for the "Authorities" tab...

> Is there a timeline on when this is needed for? I can take a better look later
> in the week if desired. 

It isn't *that* urgent, I guess, so your help later this week would certainly be welcome - thanks.

Comment 15

[Kai Engert (:KaiE:)]

We should definitely fix this before alpha 6.

Comment 16

[Neil Deakin]

(In reply to comment #14)
> 
> But anyway, out of curiosity - am I missing something? As far as I can see,
> neither val nor tabs[val].selected get changed anywhere in this function (only
> tabs[val]._selected), so why would an extra variable be needed to store the
> state?

Setting _selected modifies the selected attribute which .selected reads.

Comment 17

[Kai Engert (:KaiE:)]

It's unfortunate this is still broken.
Today is the checkin deadline alpha 6.

Can we back the original patch out?

Comment 18

[Kai Engert (:KaiE:)]

I discussed this with Neil.

He convinced me the "<tab selected=true>" feature didn't work in the past.

Therefore I'm ok to remove that attribute, at least temporarily until this bug is fixed.

Patch coming up.

Comment 19

[Kai Engert (:KaiE:)]

Attachment: Patch v4, workaround [Backout: Comment 57]

This patch removes the selected="true" attribute, which never worked in the past, and is not yet fully working.

Comment 20

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 270038 [details] [diff] [review]
Patch v4, workaround
[Backout: Comment 57]

r+sr=shaver, though I'd prefer FIXME over XXX there.

Comment 21

[Kai Engert (:KaiE:)]

Both patches checked in (workaround and improvement), with Mike's request added.

Neil, where do you plan to work on improving the selected="true" feature?

Maybe we should file a spearate bug and make it block this one?

Thanks

Comment 22

[Neil Deakin]

(In reply to comment #21)
> Maybe we should file a spearate bug and make it block this one?

Yes

Comment 23

[Kai Engert (:KaiE:)]

(In reply to comment #22)
> (In reply to comment #21)
> > Maybe we should file a spearate bug and make it block this one?
> Yes

done, filed 386202.

We should re-enable the selected="true" when bug 386202 is fixed.

Comment 25

[Kaspar]

(In reply to comment #21)
> Both patches checked in (workaround and improvement), with Mike's request
> added.

Unfortunately, the persist="selectedIndex" thing (added with the patch from attachment 267843 [details] [diff] [review]) regressed on trunk on 2007-09-14, with this change to nsPresShell.cpp:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsPresShell.cpp&branch=3.1057&root=/cvsroot&subdir=/mozilla/layout/base&command=DIFF_FRAMESET&rev1=3.1056&rev2=3.1057

(it's unrelated to the selected=true attribute, NB)

Having selectedIndex persist for cert manager is very useful, though - especially when bug 387480 will land (people will add cert overrides to the third tab, so it would be quite cumbersome if cert manager always opens with the first tab selected).

FWIW, adding these lines to the end of LoadCerts() in certManager.js will "fix" the problem:

  var tab = document.getElementById("certmanagertabs").selectedIndex;
  if (tab != 0) {
    document.getElementById("certmanagertabs").selectedIndex = 0;
    document.getElementById("certmanagertabs").selectedIndex = tab;
  }

[I'm not proposing this as a patch, of course, but maybe it helps to determine what exactly is happening... who can help with fixing this? Neil?]

Comment 26

[Kai Engert (:KaiE:)]

Kaspar, thanks for your comment.
I ran into this regression as well, and am very frustrated about it, but hadn't yet looked up this bug number.

I would like to remove the
  persist="selectedIndex"
until someone reports that it's really working.

Comment 27

[Kai Engert (:KaiE:)]

Attachment: Patch to disable persistent selection and make cert manager usable

Comment 28

[Kaspar]

(In reply to comment #27)
> Created an attachment (id=283202) [details]
> Patch to disable persistent selection and make cert manager usable

Before rushing into reverting this, I would actually prefer to hear the opinion of someone familiar with XBL and the toolkit widgets... maybe there's an easy workaround to this.

Having non-persistent tabs in cert manager is not what the user expects - the Advanced prefs e.g. also remembers the last tab (has code reading the selectedIndex from prefs.js though).

Comment 29

[Kai Engert (:KaiE:)]

(In reply to comment #28)
> Before rushing into reverting this, I would actually prefer to hear the opinion
> of someone familiar with XBL and the toolkit widgets... maybe there's an easy
> workaround to this.

We should test it locally once someone gives us hope it might be fixed.


> Having non-persistent tabs in cert manager is not what the user expects - the
> Advanced prefs e.g. also remembers the last tab (has code reading the
> selectedIndex from prefs.js though).

Right now it looks completely broken and confusing. I really prefer a working basic functionality over something that's cool but broken.

Comment 30

[Neil Deakin]

(In reply to comment #25)
> (In reply to comment #21)
> > Both patches checked in (workaround and improvement), with Mike's request
> > added.
> 
> Unfortunately, the persist="selectedIndex" thing (added with the patch from
> attachment 267843 [details] [diff] [review]) regressed on trunk on 2007-09-14, with this change to
> nsPresShell.cpp:
> 
> http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsPresShell.cpp&branch=3.1057&root=/cvsroot&subdir=/mozilla/layout/base&command=DIFF_FRAMESET&rev1=3.1056&rev2=3.1057
> 

If it's a regression from that, then you should file a bug, cc the author of the change, and mark it blocking. The correct fix for this is to then fix the regression. I have verified that the persist is working correctly in a Sept 12.

Comment 31

[Kaspar]

(In reply to comment #30)
> If it's a regression from that, then you should file a bug, cc the author of
> the change, and mark it blocking.

Done - bug 373525 (I hope I picked the right component).

Comment 32

[Kaspar]

I meant bug 398289 (persist="selectedIndex" no longer working for tabbox elements). Sorry for the bugspam.

Comment 33

[Robert Relyea]

Comment on attachment 283202 [details] [diff] [review]
Patch to disable persistent selection and make cert manager usable

r+, but please include a comment to revert the change one the regression is fixed.

bob

Comment 34

[Kai Engert (:KaiE:)]

Attachment: Patch to disable persistent selection and make cert manager usable v2 [Backout: Comment 50]

Added comment proposed by Bob.

Carrying forward r+

Requesting approval for this regression workaround.

Comment 35

[Kai Engert (:KaiE:)]

Comment on attachment 283229 [details] [diff] [review]
Patch to disable persistent selection and make cert manager usable v2
[Backout: Comment 50]

I checked in this patch to temporarily disable persistence.

Comment 36

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a9pre) Gecko/2007100304 Minefield/3.0a9pre] (nightly) (W2Ksp4)

Checking "Tools > Options... > Advanced > Encryption > View
Certificates" with this nightly (and new profile),
which is before "2007-10-03 10:21 / kaie%kuix.de / Bug 373525" and "2007-10-03 16:38 / bzbarsky%mit.edu / Bug 394676":
no bug, persistence works fine.

Why wouldn't I see the regression ?

Comment 37

[Kaspar]

(In reply to comment #36)
> Why wouldn't I see the regression ?

Was "accidentally" fixed by another recent commit (see bug 398289 comment 5 for details), so it's now working again, also for cert manager...

Kai, are you fine with re-enabling persistence again?

Comment 38

[Serge Gautherie (:sgautherie)]

(In reply to comment #37)
> Kai, are you fine with re-enabling persistence again?

I support the idea to test without then uncheck this patch :->

Comment 39

[Johnathan Nightingale [:johnath]]

Speaking of testing, and given that this has regressed and unregressed multiple times, can we get a test, either automatic or litmus, to catch it reliably in the future?

Comment 40

[Kaspar]

(In reply to comment #39)
> Speaking of testing, and given that this has regressed and unregressed multiple
> times, can we get a test, either automatic or litmus, to catch it reliably in
> the future?

In the meantime, a reftest has been added when bug 398289 was resolved - I've verified that it reliably detects a regression for persist="selectedIndex". 

So, can we revert the change from attachment 283229 [details] [diff] [review], please? (Does backing out need approval, too?)

Comment 41

[Kai Engert (:KaiE:)]

(In reply to comment #40)
> So, can we revert the change from attachment 283229 [details] [diff] [review], please?

I'm ok with backing it out, r=kengert
I've tested it locally, and it works for me, too.


> (Does backing out need approval, too?)

Not sure :-)
We don't have "blocking1.9"
So I conclude: Yes, we need approval to back it out.

Comment 42

[Kai Engert (:KaiE:)]

Comment on attachment 283229 [details] [diff] [review]
Patch to disable persistent selection and make cert manager usable v2
[Backout: Comment 50]

Requesting approval to undo this change, because the real problem got fixed elsewhere.

Comment 43

[Serge Gautherie (:sgautherie)]

I believe approval is not needed ... to undo a patch that was supposed to fix something which was/is not broken (at the checkin time).

Comment 44

[Kai Engert (:KaiE:)]

Thanks to dsicore for giving approval for the backout.

I backed out, re-enabling persistence.

Marking fixed.

Comment 45

[Kaspar]

(In reply to comment #44)
> Marking fixed.

Thanks for reverting the change, Kai! I hope you don't mind me reopening the bug, since one (minor) issue still awaits a fix, cf. line 70 in certManager.xul:

        <!-- FIXME Add selected="true" to ca_tab when 373525 gets fixed. -->
        <tab id="ca_tab" label="&certmgr.tab.ca;"/>

(I.e. undoing the change from attachment 270038 [details] [diff] [review], which was supposed to be a temporary workaround. Note that selected="true" will only apply to the very first time Cert Manager is opened, after that selectedIndex kicks in. You might have to clean localstore.rdf to see the effect.)

Neil's proposed patch in bug 386202 indeed fixes this (separate) issue, so it would be nice to undo the "Patch v4, workaround", too.

Gavin, would it be possible for you to review Neil's patch (bug 386202 attachment 276796 [details] [diff] [review]) "relatively soon"?

Comment 46

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

(In reply to comment #45)
> Gavin, would it be possible for you to review Neil's patch (bug 386202
> attachment 276796 [details] [diff] [review]) "relatively soon"?

I'll try to get to it today.

Comment 47

[Damon Sicore (:damons)]

Comment on attachment 283229 [details] [diff] [review]
Patch to disable persistent selection and make cert manager usable v2
[Backout: Comment 50]

Resetting all approval1.9+ flags on bugs that have not been checked in by Oct 22 11:59 PM PDT.  Please re-request approval if needed.

Comment 48

[Reed Loden [:reed]]

Comment on attachment 283229 [details] [diff] [review]
Patch to disable persistent selection and make cert manager usable v2
[Backout: Comment 50]

Landed by kaie on 2007-10-03 10:21.

Comment 49

[Damon Sicore (:damons)]

Comment on attachment 283229 [details] [diff] [review]
Patch to disable persistent selection and make cert manager usable v2
[Backout: Comment 50]

Reset approval flag to + as it was already checked in.

Comment 50

[Serge Gautherie (:sgautherie)]

(In reply to comment #48)
> (From update of attachment 283229 [details] [diff] [review])
> Landed by kaie on 2007-10-03 10:21.

(For the record: ... and backed out by '2007-10-09 15:32  kaie%kuix.de', = before Oct 22 too ;-))

Comment 51

[Mike Beltzner [:beltzner, not reading bugmail]]

The remaining issue (see comment 45) blocks for final release, but not M9.

Comment 52

[Kai Engert (:KaiE:)]

Release triaging requested a priority for this bug.

I think the remaining problem (comment 45) is not critical and should not block the release.

I'd even propose to open a separate bug to deal with the remaining issue, so we can differ between "cert manager completely broken" (the initial problem) and "the right tab should be opened by default" (remaining problem).

Comment 53

[Serge Gautherie (:sgautherie)]

Comment on attachment 270038 [details] [diff] [review]
Patch v4, workaround
[Backout: Comment 57]

'approval1.9=?':
To be reverted, per comment 45.

Comment 54

[Kai Engert (:KaiE:)]

let's be careful and let's wait a while for the code from bug 286202 to stabilize. they backed it out again.

Comment 55

[Reed Loden [:reed]]

(In reply to comment #54)
> let's be careful and let's wait a while for the code from bug 286202 to
> stabilize. they backed it out again.

I think it's been long enough. Time to revert?

Comment 56

[Kai Engert (:KaiE:)]

Ok, let's try. Reed, do you want to check it in?

For the record, my comment 54 refered to bug 386202 (not 286202).

Comment 57

[Reed Loden [:reed]]

Backed out.

Checking in security/manager/pki/resources/content/certManager.xul;
/cvsroot/mozilla/security/manager/pki/resources/content/certManager.xul,v  <--  certManager.xul
new revision: 1.44; previous revision: 1.43
done

Comment 58

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2) Gecko/2007121120 Firefox/3.0b2.