Bug 66081 – The "port" property of an <a> whose href does not specify a port returns the wrong value

• Component: DOM
• Status: RESOLVED FIXED

[Corey Frang]

Chrome Version       : 15.0.847.0 canary (also affects Safari)
URLs (if applicable) : http://jsfiddle.net/gnarf/8gQgS/2/show/
http://jsfiddle.net/gnarf/8gQgS/2/ for the code

Other browsers tested:
     Safari 5: FAIL
  Firefox 4.x: OK
       IE 7/8/9: OK

What steps will reproduce the problem?
1. a = document.createElement( "a" );
2. a.port === "0";

What is the expected result?

a.port === "";

It seems that location.port === "", so a.port should also be an empty string.


What happens instead?

a.port is '0'

Please provide any additional information below. Attach a screenshot if
possible.

http://www.whatwg.org/specs/web-apps/current-work/#url-decomposition-idl-attributes states that it should return the "empty string"

[Rachel Blum]

Created attachment 104916 [details]
Patch

[Adam Barth]

Comment on attachment 104916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104916&action=review

> Source/WebCore/html/HTMLAnchorElement.cpp:410
>  String HTMLAnchorElement::port() const
>  {
> -    return String::number(href().port());
> +    if (href().hasPort())
> +        return String::number(href().port());
> +
> +    return String("");
>  }

Do we need to do the same thing to Location.cpp ?

[Adam Barth]

Comment on attachment 104916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104916&action=review

I feel like I get in trouble whenever I review these sorts of patches, but this patch seems good.

> Source/WebCore/html/HTMLAnchorElement.cpp:409
> +    return String("");

You can just return "" or emptyString().

[Adam Barth]

> Safari 5: FAIL

^^^ Do we need to update the Apple-Mac results as well?

[Rachel Blum]

(In reply to comment #4)
> > Safari 5: FAIL
> 
> ^^^ Do we need to update the Apple-Mac results as well?

I'm confused - where did you see that result? At least for me, bugs.webkit still shows mac tests as white (i.e. not run)

(In reply to comment #2)
> Do we need to do the same thing to Location.cpp ?

Not sure - I'll go re-read the HTML5 spec and file a separate bug if necessary. 

(In reply to comment #3)
> You can just return "" or emptyString(). 

will do. 

Also, please note that I'm waiting for word from brettw on KURLGoogle. So please don't commit before he approved.

[Eric Seidel (no email)]

The original reporter said this bug exists in Safari too:

Chrome Version       : 15.0.847.0 canary (also affects Safari)
URLs (if applicable) : http://jsfiddle.net/gnarf/8gQgS/2/show/
http://jsfiddle.net/gnarf/8gQgS/2/ for the code

So we will need to fix KURL.cpp too, or?  Why only KURLGoogle.cpp?

[Eric Seidel (no email)]

Comment on attachment 104916 [details]
Patch

I don't undersatnd why you need/wnat the KURLGoogle part of your change?

[Eric Seidel (no email)]

The r- was just because we need to confirm that we're fixing this for normal (non-chromium) webkit ports as well as Chromium here, since this was reported as not just a Chromium bug. :)

[Adam Barth]

> The r- was just because we need to confirm that we're fixing this for normal (non-chromium) webkit ports as well as Chromium here, since this was reported as not just a Chromium bug. :)

That's what the tests are for.  :)

[Adam Barth]

(In reply to comment #5)
> (In reply to comment #4)
> > > Safari 5: FAIL
> > 
> > ^^^ Do we need to update the Apple-Mac results as well?
> 
> I'm confused - where did you see that result? At least for me, bugs.webkit still shows mac tests as white (i.e. not run)

The "mac" bubble doesn't run tests.  It just builds.  You'll either need to test the patch on Mac yourself or find a friend who can test it for you.

We've asked the Apple folks to run a pre-commit testing bot, but they haven't done so yet.

[Rachel Blum]

Comment #6:

The bug as originally described is fixed without KURLGoogle. I.e. the described symptoms are completely fixed by just the change to HTMLAnchorElement.cpp.

But - and this also answers comment #7 -  the HTML spec calls for the setter to set the port to 0 when you pass an empty/invalid string. That was broken in KURLGoogle - see the previous LayoutTests for chromium.

And since this bug is ultimately about fixing the port attribute to conform to the HTML5 spec, I've added this in here. 

There is no fix to KURL, because KURL allows setting the port to 0, while KURLGoogle used to clear the port instead of setting it to 0 in that case.

So this patch fixes *two* issues with the port property, and the second one existed only for the Chromium build.

If it'd help, I'd be happy to split the Chromium part into a second bug - it just seemed close enough for a single patch

[Adam Barth]

Comment on attachment 104916 [details]
Patch

Ah, that explains why that test doesn't need to be updated for Mac.  Thanks for the explanation.  In the future, that's great information to put into the ChangeLog entry.

[Brett Wilson (Google)]

KURLGoogle change looks good. It looks like they're added removePort and removed the "0 = clear" behavior.

[WebKit Review Bot]

Comment on attachment 104916 [details]
Patch

Rejecting attachment 104916 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-01', '--port..." exit_code: 2

Last 500 characters of output:

  fast/url/trivial-segments.html = TEXT

Regressions: Unexpected image mismatch : (5)
  fast/text/atsui-multiple-renderers.html = IMAGE
  fast/text/international/danda-space.html = IMAGE
  fast/text/international/thai-baht-space.html = IMAGE
  fast/text/international/thai-line-breaks.html = IMAGE
  platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE

Regressions: Unexpected image and text mismatch : (1)
  svg/custom/svg-fonts-word-spacing.html = IMAGE+TEXT



Full output: http://queues.webkit.org/results/9479748

[Adam Barth]

>   fast/url/trivial-segments.html = TEXT

^^^ We need to clean up the output from the commit-bot, but this is the problematic test.

[Rachel Blum]

Created attachment 105120 [details]
Patch

[Rachel Blum]

Created attachment 105122 [details]
Patch

[Darin Adler]

Comment on attachment 105122 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=105122&action=review

> Source/WebCore/page/Location.cpp:89
> -    return url.port() ? url.host() + ":" + String::number(url.port()) : url.host();
> +    return url.hasPort() ? url.host() + ":" + String::number(url.port()) : url.host();

What test covers this change?

> Source/WebCore/page/Location.cpp:206
> -    if (port < 0 || port > 0xFFFF)
> +    if (port < 0 || port > 0xFFFF || portString.isEmpty())

What test covers this change?

> Source/WebCore/platform/KURLGoogle.cpp:677
> +    portStr = String::number(i);
> +    replacements.SetPort(
> +        reinterpret_cast<const url_parse::UTF16Char*>(portStr.characters()),
> +        url_parse::Component(0, portStr.length()));

What test covers this change? That same test could verify that we don't have the bug with KURL.cpp.

[Rachel Blum]

Source/WebCore/page/Location.cpp:89 - new test coming in next patch (new checkTest4 in location-port.html)

Source/WebCore/page/Location.cpp:206 - covered by checkTest2 in location-port.

Source/WebCore/platform/KURLGoogle.cpp:677 - this is covered by "Set port to 0" in set-href-port-attribute. See the FAIL in the chromium version that changed to PASS, and the existing PASS in the webkit version of the test

[Rachel Blum]

Created attachment 105220 [details]
Patch

[WebKit Review Bot]

Comment on attachment 105220 [details]
Patch

Attachment 105220 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9508644

New failing tests:
fast/loader/location-port.html

[Rachel Blum]

Created attachment 105405 [details]
Patch

[Adam Barth]

Comment on attachment 105405 [details]
Patch

This looks great, except for the random iframe-scaling-with-scroll-expected.png result.  :)

[Rachel Blum]

Created attachment 105696 [details]
Patch

[WebKit Review Bot]

Comment on attachment 105696 [details]
Patch

Clearing flags on attachment: 105696

Committed r94132: <http://trac.webkit.org/changeset/94132>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.