195833 - js error in onLocationChange when trying to get hostPort of wyciwyg:// url

Status: VERIFIED FIXED

(SeaMonkey :: General, defect)

Description

[shliang]

wyciwyg:// protocol needs to be stripped off before getting hostPort 
(nsBrowserStatusHandler.js line 350 in onLocationChange, used to determind if 
popup blocked icon should be shown)

Comment 1

[shliang]

Attachment: patch

Comment 2

[shliang]

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

jag, could you r/sr? thanks

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

There are other URI implementations than wyciwyg that will fail when you try to
get their hostPort.  This patch is definitely needed, but the hostPort access
needs to be wrapped in a try/catch in any case.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

e.g. see bug 195413 for a non-wyciwyg failure case.

Comment 7

[shliang]

Attachment: adding try/catch

Comment 8

[jag (Peter Annema)]

Comment on attachment 116218 [details] [diff] [review]
adding try/catch

I don't think you should need a try/catch around |hostPort =
locationURI.hostPort|.
locationURI is either an exposableURI (and hostPort should work, n'est-ce
pas?), or it's null (due to the earlier try/catch failing), which you could
check for in the |if (blank || ...)|.

bz, am I missing something?

Comment 9

[Boris Zbarsky [:bzbarsky]]

about:mozilla is an exposable URI and has no hostport.
view-source:http://www.mozilla.org is an exposable URI and has no hostport
I can probably come up with a few other examples and distributors can trivially add others...

"has no hostport" means "the getter throws".

Again, see bug 195413...

Comment 10

[jag (Peter Annema)]

Hmm... Why are we throwing an exception for that?

nsIURI.idl specifies that when no port is given, the default port is assumed and
|port| returns -1, |hostPort| returns |host|. Shouldn't that extend to URIs for
which |port| doesn't apply?

Comment 11

[jag (Peter Annema)]

Comment on attachment 116218 [details] [diff] [review]
adding try/catch

r=jag, though I'm still curious why we're throwing exceptions.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Because we're talking about URI schemes which have no concept of a port.  See
http://lxr.mozilla.org/seamonkey/source/netwerk/base/src/nsSimpleURI.cpp#223

Comment 13

[Darin Fisher]

hmm.. do we care in this case that we can't get the info out of the URL?  the
wyciwyg protocol is meant to be transparent to users.  they should never see
those URLs in the browser UI, and along the same lines, they should never see
negative side-effects due to those URLs.  do we care in this case?

Comment 14

[Boris Zbarsky [:bzbarsky]]

Darin, the wyciwyg issue is actually resolved by getting the "exposable URI". 
The question on the table is whether the getters in nsSimpleURL should throw
(for about: uris, eg) as they do now.  If so, we need a try/catch in this code.

Comment 15

[Darin Fisher]

yeah, the getters should just return empty string (or -1 in the case of the
port).  setters for nsSimpleURI should throw, however.  i'm sure someone was
just being lazy when they made these functions throw, or maybe they didn't want
to have strdup("") all over the place (back when nsIURI used raw |string|
params).  nothing wrong with "_retval.Truncate(); return NS_OK;" IMO.

Comment 16

[jag (Peter Annema)]

Then we'll have to look through our code and see if anyone is depending on us
currently throwing (returning a non-NS_OK).

Comment 17

[Darin Fisher]

yeah.. there is that :(

Comment 18

[shliang]

resolving

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

Comment 20

[[e-mail]]

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

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

Comment 22

[Boris Zbarsky [:bzbarsky]]

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