233582 - OK button doesn't work in panels other than Web Features after site added to popup whitelist

Status: RESOLVED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Jason Sinclair]

User-Agent:       
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.6) Gecko/20040206 Firefox/0.8

Pressing the OK button after opening tool -> options has no effect.
Bug occurs even if no details have been changed.
Options do not close after a prolonged wait.
(Similar reports of modal windows not closing mentioned 'waiting for a few minutes')

Reproducible: Always
Steps to Reproduce:
1. Goto Tools -> Options
2. [Details don't need to be modified]
3. Press OK button
Actual Results:  
Nothing.
(Even when left for 30 mins)

Expected Results:  
Options window should have closed/

Comment 1

[Mike Connor [:mconnor]]

please go into the web features section and check whether you have any sites
with port numbers in the allowed sites for popups, and remove them (since they
won't work anyway)

also please check the javascript console for any errors (if you see nsIURI.spec
in there its the above problem)

the problem is that you could add sites with ports previously, now the port is
ignored and the saved string from 0.7 is invalid when parsed.

Comment 2

[Jason Sinclair]

Thanks Mike, tried your suggestions... but no go.
This is a virgin installation, so there aren't any sites in WebFeatures and the JavaConsole shows nothing. 
(This is the default FireFox home page).
But... The ok button now works if I don't change pages...
(ie Opening options and press OK now works, but not if the page is changed)

Comment 3

[PikeUK]

Try setting javascript.options.showInConsole to true in about:config (type it
into the address bar) then see if anything appears in the JS console when you
press OK in options.

Comment 4

[Ricardo]

I'm getting the same problem.

It only seems to happen when I choose the 'Themes' section.

When it doesn't work, I have to remove focus from the 'Themes', close the
options window, open the JavaScript console, close it and then return to the
options window where the Ok button is working again. If I rechange to 'Themes'
it stops working again.

Oh, and there are no errors at all when I open the console.

Comment 5

[Ricardo]

Oops, I forgot that I had installed the 'Themer' extension.

I tried again with the extension disabled and the button is working again.

Comment 6

[Jason Sinclair]

By enabling javascript.options.showInConsole the 'ok' button now works _except_ when done on the 'themes' page.

ie I can open Options change pages/values and 'ok' will work. Before 'ok' would only work if I didn't change pages.

But if I try to 'ok' from the themes page I get:

  Error: invalid 'in' operand itemObject
  Source File: chrome://browser/content/pref/nsPrefWindow.js
  Line: 176

This is a virgin installation, so I don't have any extensions/plugins installed.

(I'll try to have a look at the .js file later today, work-load permitting)

Comment 7

[Bradley Chapman (not reading bugmail, still gone but not forever)]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040311 Firefox/0.8.0+
(daihard: XFT+GTK2; opt. for P4/SSE-2)

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040316 Firefox/0.8.0+
(daihard: XFT+GTK2; opt. for P4/SSE-2)

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040321 Firefox/0.8.0+
(daihard: XFT+GTK2; opt. for P4/SSE-2)

Confirmed thrice on Linux. Sometimes the window closes properly when pressing
OK, but most of the time it does not. Using the preference hack garners the
following error message in the JavaScript Console on the last build:

Error: invalid "in" operand itemObject
Source File: chrome://browser/content/pref/nsPrefWindow.js, line 176

Line 176 is:

if ( "prefstring" in itemObject && itemObject.prefstring )

which doesn't look right to me for some reason (I know very little JS).

Requesting NEW & OS: All.

Comment 8

[Jeff Walden [:Waldo]]

(In reply to comment #5)
> Oops, I forgot that I had installed the 'Themer' extension.
> 
> I tried again with the extension disabled and the button is working again.

I discovered this bug a while back when a similar Options OK bug was found in
Mozilla Firebird.  See <http://mozdev.org/bugs/show_bug.cgi?id=4634>, which has
received less than no attention since I filed it.  I've grown used to the
default theme since then, and I rarely change themes any more.

Comment 9

[Marcelo Cardoso Martinelli]

I found a way around this bug. In my systems (WinXP at home and Win2k at work),
the problem showed up when I made a change to a certain option and then switched
to a different tab heading. That is when the OK button stopped working for me. 

I found that by just changing back to the original tab where I made the
alteration the OK button worked again.

Comment 10

[Neil Parks]

Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.7b) Gecko/20040323 Firefox/0.8.0+

I have not encountered this bug with this or other recent nightlies.

Try deleting xul.mfl one time and see if the problem goes away.  (I always make
it a point to del that file before upgrading to a new release or reverting to an
older one.)

Comment 11

[Mike Connor [:mconnor]]

What option is being changed?  If a function is registered that doesn't 
properly register itself with onOK(), then you'll get errors.  Please make sure 
any extensions that modify the pref window are disabled before testing.

Comment 12

[Bradley Chapman (not reading bugmail, still gone but not forever)]

The Themer extension is to blame. Uninstalling it using JSLib removed all OK
button problems; I am unable to reproduce this issue at all.

Suggest RESOLVED INVALID, as it's an extension-induced fault.

Comment 13

[Hussam Al-Tayeb]

(In reply to comment #12)
> The Themer extension is to blame. Uninstalling it using JSLib removed all OK
> button problems; I am unable to reproduce this issue at all.
> 
> Suggest RESOLVED INVALID, as it's an extension-induced fault.

I never don't have and never installed the Themer extension and I still see this
bug sometimes.

Comment 14

[Peter van der Woude [:Peter6]]

I can confirm that, no themes ever installed but the same "problem".
The optionsmenu is still heavily bugged with mainly optical things.
see http://forums.mozillazine.org/viewtopic.php?t=64984#453214 for the list (All
confirmed bugs) 

Comment 15

[Jason Sinclair]

Again, this was done on a virgin installation.
No plugins, no extensions and no extra themes.
Mozilla and all previous versions of Phoenix/FireBird/FireFox were removed.

Comment 16

[Alexey Molchanov]

I see this bug on few machines with FF 0.8 without any extensions installed too.
OS: Win 2000/XP.

Comment 17

[Mike Connor [:mconnor]]

plussing/minusing blocker flags is reserved for people with the right to do this.

Is this a meta bug, or something specific?  There's existing bugs on a number of
reasons for this to happen, I haven't seen the operand one myself, yet.  If
someone can reduce this to a simple steps to reproduce, this can be fixed.

Comment 18

[Hussam Al-Tayeb]

Steps to reproduce:
1. Open options dialog.
2. Point to "web features"
3. Add "msn.com" (or anysite that gives popups) to whitelist in in "block popup
windows"
4. Point to "advanced" tab.
5. Press OK ( ok button won't work ).
6. Point back to "web features" and press OK ( ok button will work )

Comment 19

[Hussam Al-Tayeb]

(In reply to comment #18)
In step 3, you have to add (not remove) a site to the whitelist to see this bug.

Comment 20

[Mike Connor [:mconnor]]

ok, I get it now

-> mine, this horks at the line below.

http://lxr.mozilla.org/mozilla/source/browser/components/prefwindow/content/pref-features.js#155

I should also fix this at
http://lxr.mozilla.org/mozilla/source/browser/components/prefwindow/content/pref-features.js#179
even though we don't currently have add site hooked up (that's bug 239225)

Comment 21

[Hussam Al-Tayeb]

(In reply to comment #20)
Try "add site" in the popup blocker whitelist as in comment #18

Comment 22

[Bradley Chapman (not reading bugmail, still gone but not forever)]

(In reply to comment #20)
> ok, I get it now
> 
> -> mine, this horks at the line below.

<snip>

Cross-platform hork, confirmed with Linux via comment 18's instructions.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7b) Gecko/20040408 Firefox/0.8.0+
(daihard: XFT+GTK2; opt. for P4/SSE-2)

Comment 23

[Mike Connor [:mconnor]]

this is going to hork on any platform, because nsIPermissionManager isn't
defined while you're in another panel, the callback therefore doesn't have
access to it.  using Components.interfaces.nsIPermissionManager.ALLOW_ACTION
explicitly might be the only viable fix, I need to poke this later on this week.

Comment 24

[Mike Connor [:mconnor]]

Attachment: clean up my own mess here

check whether nsIPermissionManager is defined or not, if not, define it
fix a strict warning in EnterNewSite

this is why error checking is a good thing

Comment 25

[Mike Connor [:mconnor]]

Comment on attachment 146238 [details] [diff] [review]
clean up my own mess here

mea culpa, I think, in any case, cleanup is good

Comment 26

[Scott MacVicar]

I applied the patch to source pulled earlier today and it fixes the problem.

Comment 27

[Mike Connor [:mconnor]]

Comment on attachment 146238 [details] [diff] [review]
clean up my own mess here

Brian, this is pretty straightforward

Comment 28

[Mike Connor [:mconnor]]

if it makes 0.9, great, but its not a blocker

Comment 29

[Hussam Al-Tayeb]

(In reply to comment #27)
> (From update of attachment 146238 [details] [diff] [review])
> Brian, this is pretty straightforward
> 

Any chance of checking this patch in anytime soon?

Comment 30

[Brian Ryner (not reading)]

Comment on attachment 146238 [details] [diff] [review]
clean up my own mess here

>@@ -128,19 +128,23 @@ function enterNewSite(site) {
>     // call this function again, with the string originally entered pre-populated
>     var alertTitle = stringBundle.getString("addSiteFailedTitle");
>     var alertMsg = stringBundle.getFormattedString("addSiteFailedMessage",[host]);
>     promptService.alert(window, alertTitle, alertMsg);
>     enterNewSite(name);
>   }
>+  return false;
> }

The caller seems to be expecting a string, not a boolean.  "null" would
probably be more appropriate... but do we also want to return the return value
of the recursive enterNewSite() call above that?

Comment 31

[Mike Connor [:mconnor]]

hrm, this UI is getting replaced anyway, waiting on prefpanel landings to see
how the dust clears.

Comment 32

[Jesse Ruderman]

+  if (!nsIPermissionManager)
+    var nsIPermissionManager = Components.interfaces.nsIPermissionManager;

This is incorrect.  By using |var|, you're declaring a function-local variable.
 Thus, the "if" branch will always be taken.  (JavaScript differs from C++ here;
in C++, you would be declaring a branch-local variable, which would also be
wrong.  In C++, you would get a "shadow" warning and an "unused variable" warning.)

If the variable is supposed to be local to the function, remove the entire "if"
line.  If the variable is supposed to be global, remove the "var".

Comment 33

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

Blocking 0.9... pretty bad bug. 

Comment 34

[Mike Connor [:mconnor]]

note to self: remove the useless recursion since we never hit that part of the code

Comment 35

[Mike Connor [:mconnor]]

Attachment: better cleanup

remove the nasty recursion, since its pretty much impossible to hit.  As for
the local variable here, defining this globally is unnecessary since this is
the only real consumer of nsIPermissionManager, but if its still defined within
the context of the current pref panel we don't need to do it again.

Comment 36

[Mike Connor [:mconnor]]

Comment on attachment 147852 [details] [diff] [review]
better cleanup

cleaner, returns none instead of false, drops the recursion and fixes the
strict warning properly

Comment 37

[Michael Lefevre]

"As for the local variable here, defining this globally is unnecessary since
this is the only real consumer of nsIPermissionManager, but if its still defined
within the context of the current pref panel we don't need to do it again."

But (although my understanding is limited), that's not how JS works. All
variable declarations happen first. Your code is equivalent to:
var nsIPermissionManager;
if (!nsIPermissionManager)
  nsIPermissionManager = Components.interfaces.nsIPermissionManager;

The if statement will test your local variable, never the variable in the panel
context, and your local variable will always be uninitialised at that point.
Therefore the if statement is redundant.

Comment 38

[Mike Connor [:mconnor]]

bah, dropping the var is right anyway, since we actually call both functions 
onOK so we shouldn't have to set it again.  I have no idea why I thought this 
mattered anyway.

bryner, just assume that I'll drop the "var" on both during checkin.

Comment 39

[Mike Connor [:mconnor]]

checked in on trunk and on the aviary branch

Comment 40

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs,
filter on "beltznerLovesGoats" to get rid of this mass change