217611 - Page Info can be opened multiple times

Status: RESOLVED FIXED

(Firefox :: Page Info Window, defect)

Description

[dani]

User-Agent:       Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.5b) Gecko/20030827 Mozilla Firebird/0.6.1+
Build Identifier: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; rv:1.5b) Gecko/20030827 Mozilla Firebird/0.6.1+

If you go to a secure site, you get a little lock icon down in the status bar.
If you click on this icon, you go to the "Page Info" window with the "Security"
tab selected. However, if you click on the icon a second/third/fourth time, you
get a second/third/fourth "Page Info" window, instead of just giving focus to
the one currently on the screen.

Reproducible: Always

Steps to Reproduce:
1. Go to a secure site (https).
2. Click on the lock icon in the status bar.
3. Click on the lock icon in the status bar again.

Actual Results:  
A second "Page Info" window came up.

Expected Results:  
The first "Page Info" window gets focus.

Comment 1

[Simon Paquet [:sipaq]]

Confirming with 20030830 build on W2K and taking QA Contact.

When looking at the code in browser.js it shows the following:

|function displayPageInfo()
|{
|    window.openDialog("chrome://browser/content/pageInfo.xul", "_blank",
|                      "dialog=no", null, "securityTab");
|}

The similar function for the popup-blocking icon has the following code:

|function displayPageReport()
|{
|    window.openDialog("chrome://browser/content/pageReport.xul", "_blank",
|                      "dialog,modal,resizable");
|}

I fiddled a little bit around in the displayPageInfo() function code, but with
no noticeable better results.

Comment 2

[Daniel Brooks [:db48x]]

right, what you would have to do is look through the list of windows to get a
list of page info windows, then look through that to see if there's one open for
the current page. If there is, just bring that to focus and cause it to refresh
all the information. If not, create a new one like it currently does. It should
also be noted that this is a duplicate of an existing bug for seamonkey, so you
should check out the discussion there. You could also make the lock icon trap
double click events, that would catch the most common way to hit this bug. 

Comment 3

[Steffen Wilberg]

This is not limited to the lock icon.

Comment 4

[Steffen Wilberg]

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

Comment 5

[Jason Barnabe (np)]

To get the URI of the document, would the fix for this need to just get the
value of the address textbox, or is there another way to get the URI, or would
we want to store it somehow (for example, as a property of the window) so that
the same method could be used to achieve this behaviour for many types of windows?

Comment 6

[Jason Barnabe (np)]

Attachment: patch

I added a new function in browser.js, toOpenDialogByTypeAndUrl. It's similar to
toOpenWindowByType, except it also accepts a "relatedUrl" parameter. It uses
this to check windows of the given type to see if they have a matching
relatedUrl. If a match is found, it focuses that window. If not, it opens a new
dialog. The patch also includes code to set the relatedUrl attribute for the
Page Info window. View Source could also make use of this.

Comment 7

[Mike Connor [:mconnor]]

Comment on attachment 173515 [details] [diff] [review]
patch


> 
>+function toOpenDialogByTypeAndUrl(inType, relatedUrl, windowUri, features, extraArgument)
>+{
>+  var windowManager = Components.classes['@mozilla.org/appshell/window-mediator;1'].getService();
>+  var windowManagerInterface = windowManager.QueryInterface(Components.interfaces.nsIWindowMediator);
>+  var windows = windowManagerInterface.getEnumerator(inType);
>+
>+  // Check for windows matching the url  
>+  while (windows.hasMoreElements()) {
>+    var currentWindow = windows.getNext();
>+    if (currentWindow.document.firstChild.getAttribute("relatedUrl") == relatedUrl) {
>+    	currentWindow.focus();
>+    	return;
>+    }
>+  }

this will yield a js strict error, you need to declare currentWindow outside of
the loop.

other than that, r=me.	If you could do this for View Source as well, that'd be
great!

Comment 8

[Jason Barnabe (np)]

(In reply to comment #7)
> this will yield a js strict error
Err... no it won't.

Can someone check this in?

Comment 9

[Mike Connor [:mconnor]]

not a blocker, please request approval-aviary1.1a for the patch and I'll get it in.

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 173515 [details] [diff] [review]
patch

a=asa

Comment 11

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050426
Firefox/1.0+ 10:51PDT build (this patch included)

I can still reproduce this comment #0
(both from the statusbar lock as the locationbar lock)

Comment 12

[Jason Barnabe (np)]

Yup, because this hasn't been fixed yet(In reply to comment #11)
> I can still reproduce this comment #0
> (both from the statusbar lock as the locationbar lock)
Yup, because the fix hasn't been checked in yet. Real close though.

Comment 13

[Peter van der Woude [:Peter6]]

(In reply to comment #12)
> Yup, because this hasn't been fixed yet(In reply to comment #11)
> > I can still reproduce this comment #0
> > (both from the statusbar lock as the locationbar lock)
> Yup, because the fix hasn't been checked in yet. Real close though.
> 

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=2005-04-26+09%3A54%3A00&maxdate=2005-04-26+09%3A56%3A00&cvsroot=%2Fcvsroot

Comment 14

[Mike Connor [:mconnor]]

need an additional patch for the security method of opening this:
http://lxr.mozilla.org/mozilla/source/browser/base/content/browser.js#3534

Comment 15

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

Why not just put this in page info's onload instad of trying to get every caller?

Comment 16

[Mike Connor [:mconnor]]

because then we've already opened a second window?

Comment 17

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

You can do window.close() in an onload handler, no?

Comment 18

[Mike Connor [:mconnor]]

Sure, but that's still a terrible idea.

The window would appear and disappear, instead of just focusing the existing
window.  Think of the visual distraction of a window flickering in and out of
focus.  TBE used to do this for their single window mode approach, and it was
not appealing.

The right fix here is to add an argument to BrowserPageInfo() to specify the tab
(optional, of course) and have the security callers use that.

Comment 19

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

(In reply to comment #18)
> The window would appear and disappear

Really? I tested with dummyWindow.xul (the only testing I can do at the moment,
I'm at work), and I didn't see a window open and close, which is why I suggested
it. Did TBE use the onload handler, or did it just close the window after it loaded?

In any case, you're right, what I've suggested is unnecessary. I misunderstood
the problem.

Comment 20

[Jason Barnabe (np)]

(In reply to comment #18)
> The right fix here is to add an argument to BrowserPageInfo() to specify the tab
> (optional, of course) and have the security callers use that.

The problem is that when you call openDialog, the extra arguments are rolled
into window.arguments which page info uses, but calling toOpen...() doesn't do
that, so it loses any extra arguments. Even if it did do that, we'd just end up
passing an array to openDialog and we'd get window.arguments[0][0] and
window.arguments[0][1] in page info instead of window.arguments[0] and
window.arguments[1].

The solution to that is to pass one argument (array or object) that contains the
info. So I'm trying to do that, and I've changed pageInfo.js to accept the
object. But I found that the Security tab on Page Info is overlayed. I can't
figure out which of these two files

/extensions/p3p/resources/content/pageInfoOverlay.xul
/security/manager/pki/resources/content/PageInfoOverlay.xul

overlays Firefox page info, and whether or not making the change to the Firefox
one would also affect Seamonkey.

Comment 21

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

I don't even really understand why toOpenDialogByTypeAndUrl was created, since
it doesn't seem like anything we could use for anything else. Why not move it's
functionality in to browserpageinfo() and make it work only for page info?

(In reply to comment #20)
> so it loses any extra arguments

If you're going to keep the intermediary function, just add another
extraArgument, but as I said, I don't see the need for an extra generic function
that's only used for one thing.

> But I found that the Security tab on Page Info is overlayed. I can't
> figure out which of these two files
> 
> /extensions/p3p/resources/content/pageInfoOverlay.xul
> /security/manager/pki/resources/content/PageInfoOverlay.xul

The latter is used in both Firefox and Seamonkey, but I don't see why you need
to change the overlay at all. Seamonkey's page info already does this without
messing with the overlay.

See:
http://lxr.mozilla.org/mozilla/source/xpfe/browser/resources/content/pageInfo.js#306

Comment 22

[Jason Barnabe (np)]

(In reply to comment #21)
> I don't even really understand why toOpenDialogByTypeAndUrl was created, since
> it doesn't seem like anything we could use for anything else.
View Source. DOM Inspector. Any extensions that have a dialog that is related to
a page.

> If you're going to keep the intermediary function, just add another
> extraArgument, but as I said, I don't see the need for an extra generic function
> that's only used for one thing.
And then something else comes along that requires three arguments and they have
to change it. That's crap.

> The latter is used in both Firefox and Seamonkey, but I don't see why you need
> to change the overlay at all. Seamonkey's page info already does this without
> messing with the overlay.
> 
> See:
>
http://lxr.mozilla.org/mozilla/source/xpfe/browser/resources/content/pageInfo.js#306
> 
The problem isn't with the tab selection. It's that the tab *doesn't even show
up*, because this code 
http://lxr.mozilla.org/seamonkey/source/security/manager/pki/resources/content/PageInfoOverlay.xul#67
expects window.arguments[0] to be a document or null, and that's not possible
based on what I said in comment 20.

Anyway, I'm wondering if the solution is to ifdef PageInfoOverlay.js or just put
in a check to see what's in windows.arguments[0].

Comment 23

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

(In reply to comment #22)
> View Source. DOM Inspector. Any extensions that have a dialog that is related to
> a page.

I don't think view source or DOM inspector should be one window per page, and
extensions that need this can do it themselves without much trouble.

> And then something else comes along that requires three arguments and they have
> to change it. That's crap.

I agree. I was suggesting it be used only with page info, but I didn't make that
clear.

> The problem isn't with the tab selection. It's that the tab *doesn't even show
> up*, because this code 
>
http://lxr.mozilla.org/seamonkey/source/security/manager/pki/resources/content/PageInfoOverlay.xul#67
> expects window.arguments[0] to be a document or null, and that's not possible
> based on what I said in comment 20.

Ok, I understand what you're saying. Is making this a general helper function
worth the trouble? I guess it could be. If so, I'd say go with the ifdef in the
overlay.

Anyways, it's really not my call, and reading through my comments on this bug
gives me the impression that I sound a little too nitpicky. Thanks for being
patient with me, I think I need a break :).

Comment 24

[Jason Barnabe (np)]

Attachment: patch2

Note that View Frame Info doesn't supposed to show the Security tab (bug
149207), so all the ifdef in PageInfoOverlay.xul does is avoid a Javascript
error.

(Sorry for not catching the lock aspect. I had assumed it called the same code,
then I forgot about it while testing.)

Comment 25

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

Comment on attachment 182137 [details] [diff] [review]
patch2

>Index: browser/base/content/browser.js

>+  var args = {
>+    doc:        ((doc === undefined) ? null : doc),
>+    initialTab: ((initialTab === undefined) ? null : initialTab)
>+  };

Why bother with these checks? You're just passing it to the other function
where it gets checked again.

>+  if (args !== undefined && args.doc != null)

>+  if (args !== undefined && args.initialTab != null)

>Index: security/manager/pki/resources/content/PageInfoOverlay.xul
>+  	if (window.arguments[0] !== undefined && window.arguments[0].doc != null)

Why not |if (foo && foo.bar)|?

Comment 26

[Jason Barnabe (np)]

You can't do a "if (foo)" if foo is undefined; that's an error.

I made it so that you can still open page info directly with no arguments
(window.arguments[0] === undefined), or if you bother to make an object (like
BrowserPageInfo() does) you can make either of the attributes null
(window.arguments[0].doc != null), but you must include them.

All that's documented here:
+ *  window.arguments[0] - (optional) an object consisting of
+ *                         - doc: document to use for source, or null for the
calling 
+ *                                window's document
+ *                         - initialTab: name of the inital tab to display, or null
+ *                                for the first tab
+ */

"if (foo != null)" could indeed be replaced by "if (foo)", but I'm not sure what
the preferred style is.

Comment 27

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

(In reply to comment #26)
> You can't do a "if (foo)" if foo is undefined; that's an error.

No, it's not. Javascript is type sloppy, both undefined and null evaluate to
false in a boolean expression. See
http://www.croczilla.com/~alex/reference/javascript_guide/ident.html#1012659 .

> "if (foo != null)" could indeed be replaced by "if (foo)", but I'm not sure what
> the preferred style is.

Most Firefox/Toolkit code that I've seen uses |if (foo)|.

Comment 28

[Jason Barnabe (np)]

Ah, I never knew that. *If the variable was declared*, undefined does indeed
resolve to false, and in all these cases the variable exists. I'll update the
patch when I get home.

Comment 29

[Jason Barnabe (np)]

Attachment: updated patch2

Comment 30

[Jason Barnabe (np)]

Can someone check this in?

Comment 31

[Boris Zbarsky [:bzbarsky]]

So I have a few questions about this patch:

-  if ("arguments" in window && window.arguments.length >= 1 && window.arguments[0])
+  var args = window.arguments[0];

Won't this:

1)  Break if window.arguments is undefined and
2)  Give a strict warning if there are no arguments?

Similar elsewhere...

> Index: security/manager/pki/resources/content/PageInfoOverlay.xul

What's up with the weird indent?  What's up with setting |w| to a document
object (it should be a window object, in general)?

Kaie probably needs to sign off on the PSM changes.

Comment 32

[Jason Barnabe (np)]

(In reply to comment #31)
> Won't this:
> 
> 1)  Break if window.arguments is undefined and
> 2)  Give a strict warning if there are no arguments?

window.arguments is never undefined, even if you don't pass in arguments. If
there are no arguments, window.arguments[0] is undefined, and no error or
warning results (see comment 26 and comment 27)

> What's up with the weird indent?

Looks like I put tabs instead of spaces. I'll fix it.

>  What's up with setting |w| to a document
> object (it should be a window object, in general)?

That's what the code did before.

Comment 33

[Jason Barnabe (np)]

Attachment: updated patch2 (with fixed whitespace)

Comment 34

[Boris Zbarsky [:bzbarsky]]

> and no error or warning results

With strict warnings on?  In general, accessing a non-existent property with
strict warnings on gives:

  Warning: reference to undefined property object.property
  Source File: ...

Comment 35

[Jason Barnabe (np)]

This is a warning:
alert(window.foo);

This is not:
var bar = window.foo;
alert(bar);

Comment 36

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

This needs module owner approval for the changes in /security. Kaie?

Comment 37

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

Oops, wrong Kaie.

Comment 38

[Kai Engert (:KaiE:)]

Comment on attachment 189959 [details] [diff] [review]
updated patch2 (with fixed whitespace)

moa=kaie
for changes in security

Comment 39

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

> window.arguments is never undefined, even if you don't pass in arguments. If

This isn't true. Try loading this URL:
data:application/vnd.mozilla.xul+xml,<?xml version="1.0"?><window
xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
onload="alert(window.arguments);"></window>

Comment 40

[Jason Barnabe (np)]

> (From update of attachment 189959 [details] [diff] [review] [edit])
> moa=kaie
> for changes in security

Care to fill me in on what that means?

Comment 41

[Boris Zbarsky [:bzbarsky]]

moa == Module Owner Approval

Comment 42

[Jason Barnabe (np)]

Thanks for the explanation and the approval.

As for comment 39, I'm absolutely sure I tested passing no parameters to the
Page Info dialog and it worked fine. I will test again tomorrow and if necessary
add the extra check.

Also, before someone mentions it, I spelled "initial" wrong once in the patch.
I'll fix that too.

Comment 43

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

(In reply to comment #42)
> As for comment 39, I'm absolutely sure I tested passing no parameters to the
> Page Info dialog and it worked fine. I will test again tomorrow and if necessary
> add the extra check.

Probably wouldn't hurt to add the check, given that it's present pretty much
anywhere else in the tree where window.arguments is used...

Comment 44

[Jason Barnabe (np)]

Attachment: same as before, only more paranoid

I tested once again, and I couldn't get window.arguments to be undefined even
if I didn't pass any arguments. In any case, I've re-added the paranoid checks,
fixed my spelling mistake, and unbitrotted the patch. I assume mconnor's
review+ still stands, so can someone check this in?

Comment 45

[Daniel Brooks [:db48x]]

fix checked in. Any chance you could do the same for the xpfe version of Page Info?

Comment 46

[Bryan]

Is it possible to check this into the branch as well?

~B

Comment 47

[Mike Schroepfer]

Comment on attachment 196734 [details] [diff] [review]
same as before, only more paranoid

Approved per 9/26 bug triage meeting.

Comment 48

[Asaf Romano (gone)]

1.8 branch:
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.479.2.33; previous revision: 1.479.2.32
done
Checking in browser/base/content/pageInfo.js;
/cvsroot/mozilla/browser/base/content/pageInfo.js,v  <--  pageInfo.js
new revision: 1.25.2.2; previous revision: 1.25.2.1
done
Checking in security/manager/pki/resources/jar.mn;
/cvsroot/mozilla/security/manager/pki/resources/jar.mn,v  <--  jar.mn
new revision: 1.45.4.1; previous revision: 1.45
done
Checking in security/manager/pki/resources/content/PageInfoOverlay.xul;
/cvsroot/mozilla/security/manager/pki/resources/content/PageInfoOverlay.xul,v 
<--  PageInfoOverlay.xul
new revision: 1.20.20.2; previous revision: 1.20.20.1
done