134441 - Scripts & Windows pref panel loses checkmarks the second time viewing it

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Nicholas Allen]

Build 2002032916 Win2k.

Go to Edit -> Prefs -> Advanced -> Scripts & Windows

Under "Allow webpages to" make sure at least one box is checked (I have Change 
Images, Create or Change Cookies, and Read Cookies checked).
Click on one of the other categories (such as Cache).
Click on Scripts & Windows.
Under "Allow webpages to" all boxes are unchecked.

If a permission was disabled, leaving it checked or unchecked works correctly.

If a permission was enabled, leaving it checked or unchecked both leave the 
permission enabled.  Once this problem occurs, you have to close the prefs 
dialog and reopen to remove these permissions.

Comment 1

[Nicholas Allen]

Marking regression as this works in 0322.  Tree conversion fallout?

Comment 2

[Doron Rosenberg (IBM)]

ack. Can you check if the js console has any errors?

Comment 3

[Vadim Berezniker]

I'm seeing this on linux trunk build #2002033015. confirming.

os -> all

Comment 4

[Tuukka Tolvanen (sp3000)]

nothing on js console.

Comment 5

[Nicholas Allen]

Nothing on js console.

Telling Venkman to stop on errors gets no hits.

Telling Venkman to stop on exceptions doing the above sequence of steps gives:

Exception [XPComponent] [class: XPCWrappedNative_NoHelper] {0} thrown from 
function anonymous(aPrefType=string:"bool", 
aPrefString=string:"pref.browser.homepage.disable_button.current_page", 
aDefaultFlag=void:void) in <chrome://communicator/content/pref/nsPrefWindow.js> 
line 137.
Stopped for thrown exception.
$[0] = [XPComponent] [class: XPCWrappedNative_NoHelper] {0}
function anonymous(aPrefType=string:"bool", 
aPrefString=string:"pref.browser.homepage.disable_button.current_page", 
aDefaultFlag=void:void) in <chrome://communicator/content/pref/nsPrefWindow.js> 
line 137
135: {
136: case "bool":
137: return !aDefaultFlag ? pref.GetBoolPref( aPrefString ) : 
pref.GetDefaultBoolPref( aPrefString );
138: case "int":
139: return !aDefaultFlag ? pref.GetIntPref( aPrefString ) : 
pref.GetDefaultIntPref( aPrefString );
Continuing from thrown exception.
Exception [XPComponent] [class: XPCWrappedNative_NoHelper] {0} thrown from 
function anonymous(aPrefType=string:"bool", 
aPrefString=string:"pref.browser.homepage.disable_button.select_file", 
aDefaultFlag=void:void) in <chrome://communicator/content/pref/nsPrefWindow.js> 
line 137.
Stopped for thrown exception.
$[0] = [XPComponent] [class: XPCWrappedNative_NoHelper] {0}
function anonymous(aPrefType=string:"bool", 
aPrefString=string:"pref.browser.homepage.disable_button.select_file", 
aDefaultFlag=void:void) in <chrome://communicator/content/pref/nsPrefWindow.js> 
line 137
135: {
136: case "bool":
137: return !aDefaultFlag ? pref.GetBoolPref( aPrefString ) : 
pref.GetDefaultBoolPref( aPrefString );
138: case "int":
139: return !aDefaultFlag ? pref.GetIntPref( aPrefString ) : 
pref.GetDefaultIntPref( aPrefString );
Continuing from thrown exception.
Exception [XPComponent] [class: XPCWrappedNative_NoHelper] {0} thrown from 
function getPrefValueForCheckbox
(prefName=string:"capability.policy.default.HTMLImageElement.src") in 
<chrome://communicator/content/pref/pref-scripts.js> line 74.
Stopped for thrown exception.
$[0] = [XPComponent] [class: XPCWrappedNative_NoHelper] {0}
function getPrefValueForCheckbox
(prefName=string:"capability.policy.default.HTMLImageElement.src") in 
<chrome://communicator/content/pref/pref-scripts.js> line 74
072:
073: try {
074: prefValue = pref.GetCharPref(prefName);
075:
076: if(prefValue != "allAccess" && prefValue != "sameOrigin"){
Continuing from thrown exception.
Exception [XPComponent] [class: XPCWrappedNative_NoHelper] {0} thrown from 
function getPrefValueForCheckbox
(prefName=string:"capability.policy.default.HTMLDocument.cookie.get") in 
<chrome://communicator/content/pref/pref-scripts.js> line 74.
Stopped for thrown exception.
$[0] = [XPComponent] [class: XPCWrappedNative_NoHelper] {0}
function getPrefValueForCheckbox
(prefName=string:"capability.policy.default.HTMLDocument.cookie.get") in 
<chrome://communicator/content/pref/pref-scripts.js> line 74
072:
073: try {
074: prefValue = pref.GetCharPref(prefName);
075:
076: if(prefValue != "allAccess" && prefValue != "sameOrigin"){
Continuing from thrown exception.
Exception [XPComponent] [class: XPCWrappedNative_NoHelper] {0} thrown from 
function getPrefValueForCheckbox
(prefName=string:"capability.policy.default.HTMLDocument.cookie.set") in 
<chrome://communicator/content/pref/pref-scripts.js> line 74.
Stopped for thrown exception.
$[0] = [XPComponent] [class: XPCWrappedNative_NoHelper] {0}
function getPrefValueForCheckbox
(prefName=string:"capability.policy.default.HTMLDocument.cookie.set") in 
<chrome://communicator/content/pref/pref-scripts.js> line 74
072:
073: try {
074: prefValue = pref.GetCharPref(prefName);
075:
076: if(prefValue != "allAccess" && prefValue != "sameOrigin"){
Continuing from thrown exception.
Exception [XPComponent] [class: XPCWrappedNative_NoHelper] {0} thrown from 
function anonymous(aPrefType=string:"bool", 
aPrefString=string:"pref.advanced.cache.disable_button.clear_memory", 
aDefaultFlag=void:void) in <chrome://communicator/content/pref/nsPrefWindow.js> 
line 137.
Stopped for thrown exception.
$[0] = [XPComponent] [class: XPCWrappedNative_NoHelper] {0}
function anonymous(aPrefType=string:"bool", 
aPrefString=string:"pref.advanced.cache.disable_button.clear_memory", 
aDefaultFlag=void:void) in <chrome://communicator/content/pref/nsPrefWindow.js> 
line 137
135: {
136: case "bool":
137: return !aDefaultFlag ? pref.GetBoolPref( aPrefString ) : 
pref.GetDefaultBoolPref( aPrefString );
138: case "int":
139: return !aDefaultFlag ? pref.GetIntPref( aPrefString ) : 
pref.GetDefaultIntPref( aPrefString );
Continuing from thrown exception.
Exception [XPComponent] [class: XPCWrappedNative_NoHelper] {0} thrown from 
function anonymous(aPrefType=string:"bool", 
aPrefString=string:"pref.advanced.cache.disable_button.clear_disk", 
aDefaultFlag=void:void) in <chrome://communicator/content/pref/nsPrefWindow.js> 
line 137.
Stopped for thrown exception.
$[0] = [XPComponent] [class: XPCWrappedNative_NoHelper] {0}
function anonymous(aPrefType=string:"bool", 
aPrefString=string:"pref.advanced.cache.disable_button.clear_disk", 
aDefaultFlag=void:void) in <chrome://communicator/content/pref/nsPrefWindow.js> 
line 137
135: {
136: case "bool":
137: return !aDefaultFlag ? pref.GetBoolPref( aPrefString ) : 
pref.GetDefaultBoolPref( aPrefString );
138: case "int":
139: return !aDefaultFlag ? pref.GetIntPref( aPrefString ) : 
pref.GetDefaultIntPref( aPrefString );
Continuing from thrown exception.

In particular, no exceptions are thrown when going back to the Scripts & 
Windows display the second time.

Comment 6

[John Morrison]

-> hewitt, nsbeta1

Comment 7

[Doron Rosenberg (IBM)]

can you check other panels to see if this happens as well? 

Comment 8

[Nicholas Allen]

I couldn't find any other pref panels that were losing their settings.

Comment 9

[Vadim Berezniker]

*** Bug 134981 has been marked as a duplicate of this bug. ***

Comment 10

[Samir Gehani]

Nav triage team: nsbeta1-

Comment 11

[John Morrison]

If Netscape is shipping this panel (which I'm told is true) then this panel
should work correctly. As it stands right now, if a user switches away from the 
panel and returns, they are (erroneously) told that they have disabled all of 
the options to allow windows to open/resize windows, use cookies, etc.

Comment 12

[[not reading bugmail]]

in order to fix this, its not skin related as modern theme has same issue, so I
think this rules out the css code, and Advanced > System panel has the same
exact checkboxes, and looks like it repaints just fine.. so we need to look at
the code that is attached to that and compare it with the scripts & windows
code.  Which should give us a clue here why it is funky.  It appears that there
is no dataloss going on here.

Comment 13

[André Dahlqvist]

*** Bug 135969 has been marked as a duplicate of this bug. ***

Comment 14

[Vadim Berezniker]

To Clarify for non-windows users,

The panel Dennis is talking about is where you can associate mozilla with
various extentions under windows.

Comment 15

[Alfonso Martinez]

*** Bug 136377 has been marked as a duplicate of this bug. ***

Comment 16

[Peter Trudelle]

nsbeta1- per Nav triage team.  helpwanted, ->1.0.1

Comment 17

[Christopher Aillon (sabbatical, not receiving bugmail)]

*** Bug 137214 has been marked as a duplicate of this bug. ***

Comment 18

[Christopher Aillon (sabbatical, not receiving bugmail)]

Taking bug.  I'm working on a fix.

Comment 19

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Proposed patch v1.0

Okay there were a few problems here.  We were setting the checked state
depending on whether the checkbox has changed state since we first loaded the
panel.	The problem there is that we didn't cache the initial state, so we
would uncheck all items which were not changed since first load and checking
all items which did change.

Of course, we didn't even get that far because we were looking at the wrong
thing.	So in the end, we were setting our checked states to a null variable,
which translates to false (not checked).

This patch fixes all that, plus some highly annoying nits I had in the file,
including opening braces on the same line as the function declaration (yuck).

Comment 20

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v1.0.1

Oops, remember to save next time before diffing.

Comment 21

[Doron Rosenberg (IBM)]

Comment on attachment 79056 [details] [diff] [review]
Patch v1.0.1

Needs some work:

-  function getCheckboxValue(name){
-    if ("onCheckboxCheck" in window)
-      return document.getElementById(name).checked;
-
-    return data[name].checked;
+  function getPrefValueFromCheckbox(name)
+  {
+    return !document.getElementById(name).checked;
   }

First, you should add a comment about the negating

Second, and more important, your new function, getPrefValueFromCheckbox(), does
not check if the loaded panel is the scripts panel. If it isn't (user changes
values, goes to another panel, then presses OK), you'll get a js error.  See
the function you removed for how I solved that.

Other than that, everything looks fine

Comment 22

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Patch v2.0

Better version of a patch.  It seems we were getting the right thing, just that
when we changed from a <checkbox> to a <listitem type="checkbox"> the wsm was
not picking up the .checked property on the data object and that made it appear
as if we were getting the wrong thing.

Comment 23

[Alfonso Martinez]

*** Bug 137492 has been marked as a duplicate of this bug. ***

Comment 24

[Joe Hewitt (gone)]

Comment on attachment 79208 [details] [diff] [review]
Patch v2.0

r=hewitt

Comment 25

[Joe Hewitt (gone)]

Comment on attachment 79208 [details] [diff] [review]
Patch v2.0

r=hewitt

Comment 26

[Boris Zbarsky [:bzbarsky]]

*** Bug 137774 has been marked as a duplicate of this bug. ***

Comment 27

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 79208 [details] [diff] [review]
Patch v2.0

sr=[e-mail]

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 79208 [details] [diff] [review]
Patch v2.0

a=[e-mail]

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Checked in on trunk and branch.  Fixed.

Comment 30

[Tony Tovar]

It's not totally fixed yet!  I'm still seeing a cosmetic problem on Build
2002-04-16 (1.0 branch) on WinNT.  The checkmarks disappear but are not saved as
"unchecked", i.e. if you press OK, then go back, they're still checkmarked.

Comment 31

[Vadim Berezniker]

It was checked in today and you're using a build from yesterday.
Think about it :)

Comment 32

[Asa Dotzler [:asa]]

adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it
was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify
the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword
with verified1.0.0.

Comment 33

[Andrew Hagen]

*** Bug 134651 has been marked as a duplicate of this bug. ***

Comment 34

[Jonas Jørgensen]

verified1.0.0 on branch build 2002050306.

Comment 35

[Nicholas Allen]

v

This has long been fixed.