370342 - popup blocker code assumes nsIURI.hostPort is safe (bad assumption for data:)

Status: RESOLVED WORKSFORME

(SeaMonkey :: General, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Load the URL in the URL field, and the result is:

Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.hostPort]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://navigator/content/navigator.js :: onPopupBlocked :: line 2378"  data: no]
Source File: chrome://navigator/content/navigator.js
Line: 2378

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

      var hostPort = browser.currentURI.hostPort;

We probably need to wrap that in a try {}, unless nsIURI has a safe way to check ahead of time whether it's safe.

Then we need to figure out what we actually want to set hostPort to, since the popup blocker tracks popups from previous pages on the same site (i.e. if www.m.o/foo opens a popup, and you click a link to www.m.o/bar, you can unblock the popup from /foo if you want).  Note that nsBrowserStatusHandler.js:onLocationChange wraps its hostPort in a try{}, and on failure uses "".

I wonder if that still works.  I suspect I broke it in revision 1.604 of navigator.js on trunk (and broke it on both branches the same way).

Comment 2

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #1)
> I wonder if that still works.  I suspect I broke it in revision 1.604 of
> navigator.js on trunk (and broke it on both branches the same way).

Someone may want to check whether bug 345973 introduced the same change in Firefox and whether it was intentional.

Comment 3

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

(In reply to comment #1)
>       var hostPort = browser.currentURI.hostPort;
> 
> We probably need to wrap that in a try {}, unless nsIURI has a safe way to
> check ahead of time whether it's safe.

You could QI (or instanceof) to nsIURL to check whether you have an nsStandardURL or an nsSimpleURI, but I guess that's really more of a hack - theoretically something could implement nsIURL and still have a throwing GetHostPort impl.

(In reply to comment #2)
> Someone may want to check whether bug 345973 introduced the same change in
> Firefox and whether it was intentional.

Wrong bug #?

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #3)
> (In reply to comment #2)
> > Someone may want to check whether bug 345973 introduced the same change in
> > Firefox and whether it was intentional.
> 
> Wrong bug #?
> 

Sorry, bug 354973.

Comment 5

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

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > Someone may want to check whether bug 345973 introduced the same change in
> > > Firefox and whether it was intentional.
> > 
> > Wrong bug #?
> 
> Sorry, bug 354973.

Ah, no. Firefox uses host, and it's wrapped in a try/catch. I don't think bug 354973 affected that in any way.

Comment 6

[Serge Gautherie (:sgautherie)]

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1.13) Gecko/20080313 SeaMonkey/1.1.9] (release) (W2Ksp4)

{{
Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.hostPort]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://navigator/content/navigator.js :: onPopupBlocked :: line 2379"  data: no]
Source File: chrome://navigator/content/navigator.js
Line: 2379
}}

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a5pre) Gecko/20070515 SeaMonkey/1.5a] (nightly) (W2Ksp4)

{{
Error: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIURI.hostPort]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://navigator/content/navigator.js :: onPopupBlocked :: line 2419"  data: no]
Source File: chrome://navigator/content/navigator.js
Line: 2419
}}

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9pre) Gecko/2008061601 SeaMonkey/2.0a1pre] (nightly) (W2Ksp4)

Get a "popup" "infobar": no exception.
Open the link: works fine.

***

1.9 trunk was fixed by the SM 1.5a to SM 2.0a1 switch.
Do we want to fix this on 1.8.1 branch too ? (if possible)

Comment 7

[[e-mail]]

> 1.9 trunk was fixed by the SM 1.5a to SM 2.0a1 switch.
Actually it was fixed indirectly by the switch to the infobar.

Comment 8

[[e-mail]]

(In reply to comment #6)
> Do we want to fix this on 1.8.1 branch too ? (if possible)
Sure, this should be low enough risk.

Comment 9

[Philip Chee]

Bug 393120 rewrote the popup blocker code and onPopupBlocked no longer exists.