Bug 77380 – [Qt] fast/url/path.html with Qt5 differ results on both WK1 and WK2

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Rafael Brandao]

This needs further investigation, but it looks like every port should have its own expected results. But I would expect to see at least the same results for both WK1 and WK2.

[Alexey Proskuryakov]

Why do you expect the results to be port specific (other than chromium vs. everyone else)?

[Rafael Brandao]

(In reply to comment #1)
> Why do you expect the results to be port specific (other than chromium vs. everyone else)?

I don't, I may have got confused yesterday... maybe I saw many expected results for almost all ports, but I've checked it again now and it looks like how you've just pointed out, only chromium has/should have a different result. Thanks. :)

[Rafael Brandao]

Funny story, we're getting the very same result as expected for every port (except chromium) and I just figured it out. But... now I already did all the research on this failure:

FAIL canonicalize('http://example.com／foo/') should be http://example.com%2Ffoo/. Was http://example.com/foo/.

We go through KURL::init and then we reach the static function encodeHostnames, which identifies the size of the host inside the string and then append the encoded host to the output. Note how odd this is, we don't keep any info about where the host was at this point, we calculate it there and at some point later we do it again.

As we're using ICU now, we convert this '／' to '/' and return this string containing only ascii characters back to KURL::init. As %2F is actually the percent encoded '/', this is exactly what we need (as shown in the test result). But we no longer know that it is inside the host name (that function shouldn't be static).

Later we try to identify the host name again and then this '/' mislead. We end up with host being 'example.com' and path, '/foo/'. In this case, there's no need to percent encode anything because it is not in the host name.

We could even make the code faster if we could save the host info at that point, so we won't need to waste time doing it again (wrongly) later. Alexey, Darin, do you have any thoughts on this? :-)

[Rafael Brandao]

The reason why WK1 and WK2 differ results is because of DumpRenderTree code. On WK2, we print WTF::String directly to stdout, so we get the expected result for it. But for WK1, we manage to (badly) convert to QString before printing to stdout. After that conversion, if you create a new WTF::String with the data on this QString, we may not end up with the same string as before... so this is very worrying.

The case that fails for Qt5-WK1 is: FAIL canonicalize('http://example.com/�zyx')
We produce: FAIL canonicalize('http://example.com/?zyx')

Modifying the return of QWebFrame::toPlainText to use QString::fromUtf8 should have done the trick, but the strings didn't match yet. it seems that fromUtf8 is replacing its invalid character by one of the replacement characters (as described on http://doc.qt.nokia.com/5.0-snapshot/qstring.html#fromUtf8), so it will not match when we convert back to WTF::String or when we compare the expected file with this one.

Besides we're failing on this diff, we should keep in mind that the behavior is the same internally and we canonicalize just like the other ports (which is the purpose of the test). When Jesus finish his work on bug #77450, I'll remove the skip only for Qt5-WK2.

[Hugo Parente Lima]

Created attachment 143653 [details]
Patch

[Hugo Parente Lima]

Btw, this bug must still open just in case someone decides to work on wk1.

[WebKit Review Bot]

Comment on attachment 143653 [details]
Patch

Clearing flags on attachment: 143653

Committed r118256: <http://trac.webkit.org/changeset/118256>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Rafael Brandao]

Reopening as we still have it skipped on WK1. Basically we only need to change the way DumpRenderTree prints the result to do that directly to stdout rather than concatenating everything to a QString. Another way is to fix the conversion to QString on Qt.

[Csaba Osztrogonác]

(In reply to comment #7)
> (From update of attachment 143653 [details])
> Clearing flags on attachment: 143653
> 
> Committed r118256: <http://trac.webkit.org/changeset/118256>

I don't understand this unskipping ... this test still fail on WK1 and WK2 too:

Qt 5.0-WK1 fail:
-----------------
--- /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/url/path-expected.txt
+++ /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/url/path-actual.txt
@@ -41,7 +41,7 @@
 PASS canonicalize('http://example.com/‥/foo') is 'http://example.com/%E2%80%A5/foo'
 PASS canonicalize('http://example.com//foo') is 'http://example.com/%EF%BB%BF/foo'
 PASS canonicalize('http://example.com//foo//bar') is 'http://example.com/%E2%80%AE/foo/%E2%80%AD/bar'
-FAIL canonicalize('http://example.com／foo/') should be http://example.com%2Ffoo/. Was http:/.
+FAIL canonicalize('http://example.com／foo/') should be http://example.com%2Ffoo/. Was http://example.com/foo/.
 PASS successfullyParsed is true

 TEST COMPLETE

Qt 5.0-WK2 fail:
-----------------
--- /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/url/path-expected.txt
+++ /home/oszi/WebKit/WebKitBuild/Release/layout-test-results/fast/url/path-actual.txt
@@ -37,11 +37,11 @@
 PASS canonicalize('http://example.com/%7Ffp3%3Eju%3Dduvgw%3Dd') is 'http://example.com/%7Ffp3%3Eju%3Dduvgw%3Dd'
 PASS canonicalize('http://example.com/@asdf%40') is 'http://example.com/@asdf%40'
 PASS canonicalize('http://example.com/你好你好') is 'http://example.com/%E4%BD%A0%E5%A5%BD%E4%BD%A0%E5%A5%BD'
-FAIL canonicalize('http://example.com/?zyx') should be http://example.com/%EF%BF%BDzyx. Was http://example.com/%EF%B7%90zyx.
+FAIL canonicalize('http://example.com/﷐zyx') should be http://example.com/%EF%BF%BDzyx. Was http://example.com/%EF%B7%90zyx.
 PASS canonicalize('http://example.com/‥/foo') is 'http://example.com/%E2%80%A5/foo'
 PASS canonicalize('http://example.com//foo') is 'http://example.com/%EF%BB%BF/foo'
 PASS canonicalize('http://example.com//foo//bar') is 'http://example.com/%E2%80%AE/foo/%E2%80%AD/bar'
-FAIL canonicalize('http://example.com／foo/') should be http://example.com%2Ffoo/. Was http:/.
+FAIL canonicalize('http://example.com／foo/') should be http://example.com%2Ffoo/. Was http://example.com/foo/.
 PASS successfullyParsed is true

 TEST COMPLETE

[Csaba Osztrogonác]

It should block bug79666, because it is a regression. (It passes with Qt 4.8, but fails with newer.)

[Csaba Osztrogonác]

Skipped on Qt5 again ... http://trac.webkit.org/changeset/118372

[Hugo Parente Lima]

(In reply to comment #12)
> Skipped on Qt5 again ... http://trac.webkit.org/changeset/118372

It fails on Qt5-wk2?

[Hugo Parente Lima]

(In reply to comment #13)
> (In reply to comment #12)
> > Skipped on Qt5 again ... http://trac.webkit.org/changeset/118372
> 
> It fails on Qt5-wk2?

Ossy, Rafael got it, I forgot to remove the specific Qt expected file for this test in the patch, sorry.

I'll re-upload it again.

[Hugo Parente Lima]

Created attachment 143851 [details]
Patch

Expected file moved to Qt4.8

[Lauro Moura Maranhao Neto]

This still seems to be valid. Proposed patch works for WK1 and WK2.

[WebKit Review Bot]

Comment on attachment 143851 [details]
Patch

Clearing flags on attachment: 143851

Committed r126761: <http://trac.webkit.org/changeset/126761>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.