Bug 67277 – REGRESSION (r94132): broke fast/loader/location-port.html on GTK

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Philippe Normand]

Diff:

7 PASS: internalFrame.contentWindow.location.host should be :0 and is.
 7FAIL: internalFrame.contentWindow.location.host should be :0 but instead is .

[Philippe Normand]

Skipped in http://trac.webkit.org/changeset/94173

[Martin Robinson]

Rachel or Adam, maybe you have some insight into why this test is failing on GTK+?

[Rachel Blum]

I'm not quite sure why this would be happening - GTK uses KURL as-is for URL parsing as I read it. (Please correct if I'm wrong there)

I'll try to scrounge up a GTK build environment so I can repro.

[Adam Barth]

Interesting.  If this test is using crazy unicode characters, I think we've noticed that Gtk has different unicode error handling in some cases.

[Rachel Blum]

The test is not using any unicode characters. All it is doing is setting the port property on location.

Local builds of GTK work fine both for me and mrobinson - only the bots fail on that. I believe mrobinson is looking at that?

[Philippe Normand]

I was able to reproduce this failure locally (i don't recall if it was on a Release or Debug build) and reverting that change had the effect to make the test pass again. I'll try again tomorrow!

[Philippe Normand]

I guess I missed something but I don't see how the test can pass in the first place, with and without the change to Location.cpp :(

I wonder why the failure happens only in GTK, I reverted the change to Location.cpp and the test still fails, which makes me think of a bug in the KURL parsing code. KURL::parse() is a bit scary but I'll try to hunt the possible bug down...

To sum up, the bug is that setting port to 0 on a file url, hostname becomes empty string instead of ":0".

[Rachel Blum]

pnormand: Keep in mind that it passes for Safari, which uses KURL unmodified. Also, mrobinson and I ran it on our local machines and saw the tests pass, too.

::parse treats all consecutive digits in the port field as port number, so setting to 0 should work fine. (And setPort doesn't have any exception for port 0)

Could you elaborate why you think the test shouldn't be able to pass at all?

[Philippe Normand]

(In reply to comment #8)
> pnormand: Keep in mind that it passes for Safari, which uses KURL unmodified. Also, mrobinson and I ran it on our local machines and saw the tests pass, too.
> 

Did you unskip it before running it? Because I noticed that if a test is skipped and I try to run it directly with run-webkit-tests it will just be ignored. This is a new behavior compared to old-run-webkit-tests, I think.

> ::parse treats all consecutive digits in the port field as port number, so setting to 0 should work fine. (And setPort doesn't have any exception for port 0)
> 
> Could you elaborate why you think the test shouldn't be able to pass at all?

the bug is that setting port to 0 on a file url, hostname becomes empty string instead of ":0". Location::host() returns:

return url.hasPort() ? url.host() + ":" + String::number(url.port()) : url.host();

In the location-port.html test, when port is set to 0, it is expected for host to become ":0":

in checkTest3(): internalFrame.contentWindow.location.port = 0;
in checkTest4(): shouldBe('internalFrame.contentWindow.location.host', ':0');

But this is not what I observe locally :/
Oh, well. I guess I'm just becoming crazy ;-)

[Devdatta Deshpande]

Created attachment 111947 [details]
Patch

This seems to be an issue with libSoup API. If port is 0, it is not explicitly specified in SoupURI.
<http://maemo.org/api_refs/5.0/5.0-final/libsoup-2.4/libsoup-24-SoupURI.html#soup-uri-set-port>

Hence adding an explicit check for the port before updating ResourceRequest with SoupMessage fixes this issue.

I have attached the patch for review.

[WebKit Review Bot]

Attachment 111947 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1

Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:95:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Devdatta Deshpande]

Created attachment 111957 [details]
Patch after fixing style error

Fixed the style error.

Ideally, removing following statement should also fix this issue:
    m_url = soupURIToKURL(soup_message_get_uri(soupMessage));

I am not sure if soupURIToKURL is required, as m_url is already set in ResourceRequest. Any thoughts?

[Martin Robinson]

Comment on attachment 111957 [details]
Patch after fixing style error

I think it would be better to handle this in soupURIToKURL by looking up the port part of the SoupURI nad setting it on the new KURL.

[Devdatta Deshpande]

(In reply to comment #13)
> (From update of attachment 111957 [details])
> I think it would be better to handle this in soupURIToKURL by looking up the port part of the SoupURI nad setting it on the new KURL.

The port part of SoupURI is by default 0, i.e. even if a port is not specified.

For example, consider following two URLs:
1. file:///:0/home/code/WebKit/LayoutTests/fast/loader/location-port.html
2. file:///home/code/WebKit/LayoutTests/fast/loader/location-port.html

In both the cases soup_uri_to_string would return URL #2 and port as 0. 
Whereas, for case #1 KURL will return port as 0 and KURL::hasPort would return true and for case #2 port is 0 and KURL::hasPort returns false.

Thus I don't think we can figure out if a port is set to 0 from SoupURI. So if we try to handle it in soupURIToKURL, test case #2 fails in location-port.html
i.e. shouldBe('internalFrame.contentWindow.location.port == ""', true);

As mentioned earlier, we should not be setting m_url again in ResourceRequest::updateFromSoupMessage as it is already filled. It is ok to do so in ResourceResponse.

[Martin Robinson]

Comment on attachment 111957 [details]
Patch after fixing style error

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

This is an okay work-around for now, but the real fix should be in libsoup.

> Source/WebCore/platform/network/soup/ResourceRequestSoup.cpp:97
> +    bool hasPort = m_url.hasPort();
> +    unsigned short port = m_url.port();
>      m_url = soupURIToKURL(soup_message_get_uri(soupMessage));
>  
> +    // If port is 0, SoupURI does not have an explicitly specified port.
> +    // By default the port in KURL is 0, hence hasPort is used to determine if
> +    // a port was explicitly specified.
> +    if (hasPort && !port)
> +        m_url.setPort(port);
> +

I think it would be clearer here to do:
bool shouldPortBeResetToZero = m_url.hasPort() && !m_url.port();
m_url = soupURIToKURL(soup_message_get_uri(soupMessage));

// SoupURI cannot differeniate between an explicitly specified port 0 and no port specified.
if (shouldPortBeResetToZero)
    m_url.setPort(0);

[Martin Robinson]

CC'd Dan, who might hvae another opinion on the matter.

[Dan Winship]

(In reply to comment #15)
> This is an okay work-around for now, but the real fix should be in libsoup.

It would be possible to patch SoupURI so that it remembered when a 0 was explicitly specified vs when it's using it to mean "unspecified port".

But keep in mind that no real-world URI would ever use port 0, since it's not possible to set up a listening server on that port since bind() treats 0 as meaning "pick a random port". So the code is really only going to get used for this one test case.

[Martin Robinson]

(In reply to comment #17)

> But keep in mind that no real-world URI would ever use port 0, since it's not possible to set up a listening server on that port since bind() treats 0 as meaning "pick a random port". So the code is really only going to get used for this one test case.

Okay. Seems fine to keep this work-around in WebKit then, since it's not really useful to the rest of the libsoup users in the world.

[Devdatta Deshpande]

Created attachment 113444 [details]
Updated patch

[WebKit Review Bot]

Comment on attachment 113444 [details]
Updated patch

Clearing flags on attachment: 113444

Committed r99157: <http://trac.webkit.org/changeset/99157>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.