185690 - Preference "Client Certificate Selection" has no effect

Status: RESOLVED FIXED

(Core :: XUL, defect, P3)

Description

[Christian Franke]

User-Agent:       Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3a) Gecko/20021212
Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.3a) Gecko/20021212

The Preference "Client Certificate Selection" has no effect in the UI and in
prefs.js.


Reproducible: Always

Steps to Reproduce:
1. Start Browser
2. Edit|Preferences|Privacy&Security|Certificates
3. Select one of the 2 buttons of "Client Certificate Selection"
4. OK
5. Edit|Preferences|Privacy&Security|Certificates
Actual Results:  
No "Client Certificate Selection" button selected
No change in prefs.js


Expected Results:  
Selection of Step 3. should be persistent.


Bug was also present in 1.2

Comment 1

[Matthias Versen [:Matti]]

no security hole = not security:general

-> PSM

Comment 2

[John Unruh]

I cannot reproduce. Make sure that you are installing into a clean directory.

Comment 3

[Kai Engert [:KaiE:]]

I can not reproduce either.

Christian, after you have run into the problem, please open the JavaScript
console from Tools/Web Development. Does it show errors? Please paste them in
here. To make sure you don't see errors from other components, use a fresh
browser session where you haven't done anything else besides reproducing the
problem.

Comment 4

[Christian Franke]

Regarding comment 3: sry, I missed the JS Debugging output:

 Error: val has no properties
 Source File: chrome://global/content/bindings/radio.xml#radiogroup.selectedItem
 (setter)
 Line: 2

Thanks for the tip in comment 2. The installation was in a clean directory, but
the profile was converted from NS4.7. Restarting with a fresh profile, I also
could not reproduce but found the workaround:

Remove the following line from the converted prefs.js:

  user_pref("security.default_personal_cert", "default");
 
(To reproduce the bug, simply insert this setting in any profile ;-)

This is possibly a problem in Profile conversion.
There was another converted setting, that may have this problem:

  user_pref("security.default_proxy_cert", "default");


*Reassign this bug to "Browser/Profile Migration" ?

Comment 5

[Kai Engert [:KaiE:]]

Ok, this is a regression. I have worked on a similar problem before in bug
129740. At that time in the past, the radio buttons worked without JS
exceptions, even if the preferences did not match the choices in XUL.

I have a fix for radio.xml, which makes sure the exception is not triggered. By
doing so, the JS context continues to work and a new selection done by the user
will be stored when pressing ok.

Comment 6

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Suggested fix

Comment 7

[Kai Engert [:KaiE:]]

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

Hi Jag, can you please r/sr? Can you suggest somebody who should do the other
review, since I'm not working in this kind of the code often? Thanks!

Comment 8

[John Morrison]

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

This isn't a review, but the following change cannot be correct.

>-      var alreadySelected = val.getAttribute("selected") == "true";
>+      var alreadySelected = false;
>+      if (val) val.getAttribute("selected") == "true";

Comment 9

[Kai Engert [:KaiE:]]

Indeed, good catch, the second line must be 
  if (val) alreadySelected = val.getAttribute("selected") == "true";

Comment 10

[jag (Peter Annema)]

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

How about this:

	<setter>
	<![CDATA[
	  var focused = this.getAttribute("focused") == "true";
	  var alreadySelected = false;

	  if (val) {
	    alreadySelected = val.getAttribute("selected") == "true";
	    val.setAttribute("focused", focused);
	    val.setAttribute("selected", "true");
	    this.value = val.value;
	  }

	  // uncheck all other group nodes
....

That should be functionally equivalent (double check though) and hopefully make
the code easier to understand.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This won't block 1.3beta from being released, but I'd definitely approve a patch
to fix this issue if you can get it ready in time (say, before Wednesday).

Comment 12

[Kai Engert [:KaiE:]]

Attachment: Patch v2

patch using Jag's code

Comment 13

[jag (Peter Annema)]

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

sr=jag

Comment 14

[Kai Engert [:KaiE:]]

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

Javi, can you please review?

Comment 15

[Javier Delgadillo]

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

r=javi

Comment 16

[Kai Engert [:KaiE:]]

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

requesting approval for this profile migration correctness patch

Comment 17

[Asa Dotzler [:asa]]

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

a=asa (on behalf of drivers) for checkin to 1.3

Comment 18

[Kai Engert [:KaiE:]]

Checked in, marking fixed.