270098 - In <navigator.js>, "Error: browser.popupUrls has no properties"; and fix the "separator" behaviour.

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a4) Gecko/20040927] (release) (W98SE)

{{
Error: browser.popupUrls has no properties
Source File: chrome://navigator/content/navigator.js
Line: 2318
}}

Steps:
1. Open 'Tools > Popup Manager > (right sub-menu)' !

Comment 1

[Serge Gautherie (:sgautherie)]

Code is
{{{
2332 function createShowPopupsMenu(parent) {
2333   while (parent.lastChild && parent.lastChild.hasAttribute("uri"))
2334     parent.removeChild(parent.lastChild);
2335 
2336   var browser = getBrowser().selectedBrowser;      
2337 
2338   for (var i = 0; i < browser.popupUrls.length; i++) {   // <-- Was line 2318 !
2339     var menuitem = document.createElement("menuitem");
2340     menuitem.setAttribute("label",
gNavigatorBundle.getFormattedString('popupMenuShow', [browser.popupUrls[i].spec]));
2341     menuitem.setAttribute("uri", browser.popupUrls[i].spec);
2342     menuitem.setAttribute("features", browser.popupFeatures[i]);
2343     parent.appendChild(menuitem);
2344   }
2345 
2346   return true;
2347 }
}}}

Comment 2

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5) Gecko/20041116] (nightly) (W98SE)

Same:
{{
Error: browser.popupUrls has no properties
Source File: chrome://navigator/content/navigator.js
Line: 2338
}}

Comment 3

[[e-mail]]

I don't understand the steps to reproduce. As far as I can see that code is only
referenced by the popup icon's context menu which is therefore only available
after a popup has been blocked and you have some urls to unblock.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> I don't understand the steps to reproduce. As far as I can see that code is only

Let's try and reproduce it first...

0. From Browser
1. Press (and release): Alt, then T, then o
2. Go to and see the JS.C. !

Comment 5

[HJ]

Take a look at these:

http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/nsBrowserStatusHandler.js#334
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/nsBrowserStatusHandler.js#345
http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigator.js#2275

so popupUrls can be NULL and [] and that is the main problem, because there is
no check for this.

Comment 6

[[e-mail]]

(In reply to comment #4)
I didn't spot the call to popupBlockerMenuShowing in cookieNavigatorOverlay :-/

Comment 7

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> I didn't spot the call to popupBlockerMenuShowing in cookieNavigatorOverlay :-/

Then what do we need ?
{{
2339   var browser = getBrowser().selectedBrowser;      
2340   if (!browser.popupUrls)  return true;  // <-- To add this code line there !?
2341   for (var i = 0; i < browser.popupUrls.length; i++) {
}}

Comment 8

[HJ]

Attachment: this works for me

I don't have CVS at the moment so I can't add a cvs diff -u patch but it works,
including a small fix for the separator (not hidden when it should).

Comment 9

[[e-mail]]

I'm not sure that does the right thing either.

We have a popup icon which is currently correctly shown and hidden. One approach
would be to use a broadcaster to show and hide both the icon and the separator
simultaneously. However the submenu still needs to be updated correctly (it
should be cleared when the icon is hidden) so you always need to call
createShowPopupsMenu and that needs to check that the popupDomain is set.

Comment 10

[HJ]

(In reply to comment #9)
> I'm not sure that does the right thing either.

That might be, but it works;)

> We have a popup icon which is currently correctly shown and hidden. One approach
> would be to use a broadcaster to show and hide both the icon and the separator

I don't think a broadcaster is not the right this either, because of
"privacy.popups.statusbar_icon_enabled" or do you want to wack that pref also?

> simultaneously. However the submenu still needs to be updated correctly (it
> should be cleared when the icon is hidden) so you always need to call
> createShowPopupsMenu and that needs to check that the popupDomain is set.

popupDomain is |null| so function createShowPopupsMenu will always be called, right?

Comment 11

[HJ]

"I don't think a broadcaster is not the right this either,"

Yeah, I want that strong hot coffee now, but make that:

"I don't think that we should use a broadcaster because of 
"privacy.popups.statusbar_icon_enabled", or do you want to wack this pref?

Comment 12

[[e-mail]]

My point about the popupDomain is that a new tab won't have a popupDomain, will
it? In which case the popup manager menu will still show the old blocked list.

Comment 13

[HJ]

(In reply to comment #12)
> My point about the popupDomain is that a new tab won't have a popupDomain, will
> it? In which case the popup manager menu will still show the old blocked list.

All new tabs (browsers) have a popupDomain set to null. Only the first tab
(browser) has no popupDomain, but only when you startup with about:blank. All
other pages i.e. your home page triggers the onLocationChange in
nsBrowserStatusHandler.js so popupDomain gets set to null.

Comment 14

[[e-mail]]

OK, so you default to a blank page; you then open a bookmark in a new tab; that
page tries to open a popup; you look at the popups menu; you switch to the blank
tab and find that the popups menu still has the old popup...

I suppose this would work if you ensured that the appropriate properties were
defined on all browsers even the initial blank browser. However in that case it
might be more reasonable to clear popupUrls to [] instead of null.

Comment 15

[Ian Neal]

Attachment: Checks for null and empty array patch v0.1

This patch adds a check for popupUrls being either null or an empty array
before trying to enter createShowPopupsMenu. The alternative is to do these
checks within createShowPopupsMenu and return false for these conditions.

Comment 16

[[e-mail]]

Comment on attachment 169959 [details] [diff] [review]
Checks for null and empty array patch v0.1

When you change to a tab with no popups you still need to remove the old
menuitems as well as hiding the separator.

Comment 17

[Ian Neal]

Attachment: separator and null/empty array check v0.1a

Changes since v0.1 as per Neil's suggestion
* Moved null and empty array check into createShowPopupsMenu function
* Check is just after old menuitems are removed
* Check returns false so the separator is hidden

Comment 18

[[e-mail]]

Comment on attachment 170022 [details] [diff] [review]
separator and null/empty array check v0.1a

Although this works I'd still prefer a solution that ensures popupDomain always
exists, thus simplifying the code to if (separator) separator.hidden =
!createShowPopupsMenu(parent);

Comment 19

[HJ]

"Although this works I'd still prefer a solution that ensures popupDomain always
exists"

I fully agree. Why not use something simple like this:

+  var hideSeparator = !createShowPopupsMenu(parent);
+
+  if (separator)
+    separator.hidden = hideSeparator;

and fix lines 333+ in nsBrowserStatusHandler.js (replacing null with [])

Comment 20

[[e-mail]]

Except that as per your own comment 13 that won't work for a new blank window.

Comment 21

[Ian Neal]

Attachment: popupDomain and separator patch v0.1b

Comment 22

[Ian Neal]

Comment on attachment 170047 [details] [diff] [review]
popupDomain and separator patch v0.1b

Did as HJ suggested and added code to Startup which sets popupDomain for blank
pages

Comment 23

[HJ]

Comment on attachment 170047 [details] [diff] [review]
popupDomain and separator patch v0.1b

r+ from me because this looks fine. Thanks.

Neil, can you sr+ this?

Comment 24

[Serge Gautherie (:sgautherie)]

Comment on attachment 170047 [details] [diff] [review]
popupDomain and separator patch v0.1b

>+  if  (!browser.popupUrls || browser.popupUrls.length == 0)

Is |!browser.popupUrls| needed, since you initialize it to |[]| ?

Comment 25

[[e-mail]]

Comment on attachment 170047 [details] [diff] [review]
popupDomain and separator patch v0.1b

I just noticed you have to reset popupFeatures to [] because onPopupBlocked
only checks if popupUrls is null (you can then remove that check too). I also
think you might be safer always setting the properties in Startup in case the
page takes time to load. And please fix Serge's nit too.

Comment 26

[Ian Neal]

Attachment: popupDomain/Urls/Features and Separator patch v0.1c

Suggested changes made

Comment 27

[HJ]

Comment on attachment 170058 [details] [diff] [review]
popupDomain/Urls/Features and Separator patch v0.1c

This one should make everyone happy I guess, r+ HJ

Comment 28

[Michiel van Leeuwen (email: mvl+moz@)]

Do we really need all this code to initialize the array? What is wrong with a
simple null-check?
And is there currently (without any of those patches) an actual problem, or is
it only an error on the console?

Comment 29

[Ian Neal]

Comment on attachment 170058 [details] [diff] [review]
popupDomain/Urls/Features and Separator patch v0.1c

Checking in navigator.js;
/cvsroot/mozilla/xpfe/browser/resources/content/navigator.js,v	<-- 
navigator.js
new revision: 1.556; previous revision: 1.555
done
Checking in nsBrowserStatusHandler.js;
/cvsroot/mozilla/xpfe/browser/resources/content/nsBrowserStatusHandler.js,v 
<--  nsBrowserStatusHandler.js
new revision: 1.62; previous revision: 1.61
done

Comment 30

[Michiel van Leeuwen (email: mvl+moz@)]

Can somebody answer my question in comment 28? (because i really don't like this
huge amount of code for just a nullcheck)

Comment 31

[HJ]

(In reply to comment #30)
> Can somebody answer my question in comment 28? (because i really don't like this
> huge amount of code for just a nullcheck)

What huge amount of code are ou talking about? Please check the patch and count
the plus/minus lines, thank you.

Comment 32

[Michiel van Leeuwen (email: mvl+moz@)]

The patch could have just added a null check and not touch the other areas. A
one line fix. (plus a few lines to fix the seperator issue, which stricly isn't
this bug)

Comment 33

[Serge Gautherie (:sgautherie)]

(In reply to comment #28)
> Do we really need all this code to initialize the array? What is wrong with a
> simple null-check?

The fix(es) seems fine to me as it is, dealing with true and potential issues;
while I understand your concern about a minimal patch,
I wonder what drawback(s) you find in the resulting code ?

> And is there currently (without any of those patches) an actual problem, or is
> it only an error on the console?

As the reporter, I found the error,
but I did not try and see if it had any associated issue (I'm not truely using
this feature);
then I'll leave it to the (super-)reviewers, and other commenters, to discuss
this matter with you.