299088 - certain href values cause failure accessing hash property (and render with wrong style)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Flynn (Oracle)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

If a link's href contains both a space and a protocol that Moz/FF knows about,
two problems occur:
 a) It is not rendered in the link style
 b) Attempting to access the DOM node's hash property results in a failure code

The same two problems arise if the href is not specified at all.

Reproducible: Always

Steps to Reproduce:
See testcase
Actual Results:  
The way links 1,3,4,8 behave:
1. Links are black, not underlined, no 'hand' cursor. DOM Inspector shows that
none of the usual browser CSS is being applied. The link is still draggable,
however.
2. Trying to access the link node's hash property via JavaScript gives
"Component returned failure code: 0x804b000a [nsIDOMNSHTMLAnchorElement.hash]" 
nsresult: "0x804b000a (<unknown>)"

Expected Results:  
The way the other links behave:
1. Links are blue, underlined, with 'hand' cursor
2. Accessing the hash property does not throw an exception

Reproduced on: Firefox 1.0.4/WinXP, Mozilla 1.7.3/WinXP, and Mozilla 1.5/Linux

Comment 1

[Peter Flynn (Oracle)]

Attachment: Testcase

Comment 2

[Peter Flynn (Oracle)]

Attachment: Testcase

Ack, sorry. This is the version I meant to upload.

Links 1,2,4,5,9 show the problem and 3,6,7,8 work as expected.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Those are not styled as links because they are not links -- that string is not a valid URI for those schemes, so clicking on the "link" would do nothing.  Given that, we should't be showing this random text to the user as a link.

Similarly, if there is no reasonable href, there is no hash.  I suppose we could return the empty string instead of throwing, though.  jst, sicking, peterv, thoughts?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Three things jump out as wrong.

1. I agree we should probably return an empty string rather then throwing
2. Seems like we try to actually load even broken uri and then fail somewhere
   along the line in a security check. We should probably do nothing when
   clicking on a link with a bad uri.
3. Why are 6 and 7 styled as links when the other bad links are not?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> 2. Seems like we try to actually load even broken uri and then fail somewhere
>   along the line in a security check.

Actually, no.  We aren't trying to load anything.  Chrome does some sort of stuff with the click event for some reason (since chrome doesn't bother checking whether this is a real link; that would be too sensible; it just goes by the localname).  So it calls CheckLoadURIStr, which throws because newURI() on that string throws.  Chrome is kinda dumb and assumes any error from CheckLoadURIStr means a security error.  Should probably file a separate bug on it.

> 3. Why are 6 and 7 styled as links when the other bad links are not?

6 because it's an unknown protocol, so we don't know what the rules for URIs in that scheme are.

7 looks like a necko bug.   Again, should be filed separately.

Sicking, basically the link is only a link if we can create an nsIURI for it.  It's that simple.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 7 looks like a necko bug.   Again, should be filed separately.

I was wrong here.  Spaces are allowed in URIs, just not in the hostname of hierarchical URIs.

So the only thing we should really change here is that the hash access should not throw.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, and see bug 229700 for the styling issue.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #6)
> > 7 looks like a necko bug.   Again, should be filed separately.
> 
> I was wrong here.  Spaces are allowed in URIs, just not in the hostname of
> hierarchical URIs.

Shouldn't spaces be escaped as %20?

Confirming the bug to fix the throw issue. The same issue probably exists trying to access the other parts of the uri.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> Shouldn't spaces be escaped as %20?

They're not allowed in the hostname, period.  Escaped or not.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, i ment, shouldn't 7 be written as href="a%20b"? I.e. that spaces should be escaped where they are allowed?

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 202176 [details] [diff] [review]
Like so

I'd be fine with always returning NS_OK when NS_NewURI fails.

r=me either way

Comment 13

[Boris Zbarsky [:bzbarsky]]

> No, i ment, shouldn't 7 be written as href="a%20b"?

In theory... in practice, enforcing that breaks sites.  :(

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 202176 [details] [diff] [review]
Like so

sr=jst

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed.