Bug 87734 – REGRESSION (r118735): svg/dom/complex-svgView-specification.html, svg/dom/SVGViewSpec.html, svg/dom/viewspec-parser.html failing on GTK Linux 64-bit Release

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Zan Dobersek]

The following tests started failing on GTK Linux 64-bit Release between r118735 and r118736 (inclusive):

    svg/dom/complex-svgView-specification.html
    svg/dom/SVGViewSpec.html
    svg/dom/viewspec-parser.html

http://trac.webkit.org/log/trunk?rev=118736&stop_rev=118735&limit=3

http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r118734%20(24420)/results.html passed
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r118736%20(24421)/results.html failed

Also failing on other Gtk builders:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=svg%2Fdom%2Fcomplex-svgView-specification.html%2Csvg%2Fdom%2FSVGViewSpec.html%2Csvg%2Fdom%2Fviewspec-parser.html

[Zan Dobersek]

The cause of these test failures is that the fragment identifier passed into SVGViewSpec::parseViewSpec[1] contains percent-encoded spaces (%20).

The solution to this could be resetting the fragment identifier in soupURIToKURL[2] to the return value of soup_uri_normalize called on the SoupURI's fragment, with a space passed into that function call as an extra character to unescape.

This would also require to modify soup_uri_normalize behavior a bit. In uri_normalized_copy[3], the normalized value is marked for a fixup when it comes upon a \0 character (as g_ascii_isgraph returns false for that character). This should be avoided as in the fixup process, spaces are changed back into their percent-encoded form.
Also, %00 characters shouldn't be changed to \0 as this cuts off the string and can cause problems (http/tests/security/xssAuditor/anchor-url-dom-write-location-inline-event-null-char.html is a test depending on this not to happen).

[1] - http://trac.webkit.org/browser/trunk/Source/WebCore/svg/SVGSVGElement.cpp#L680
[2] - http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/soup/SoupURIUtils.cpp#L32
[3] - http://git.gnome.org/browse/libsoup/tree/libsoup/soup-uri.c#n702
[4] - http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/security/xssAuditor/anchor-url-dom-write-location-inline-event-null-char.html

[Dan Winship]

so are you saying SVGViewSpec::parseViewSpec expects some characters to be %-escaped, but requires other characters to not be? That seems like a bad API if so...

[Zan Dobersek]

(In reply to comment #2)
> so are you saying SVGViewSpec::parseViewSpec expects some characters to be %-escaped, but requires other characters to not be? That seems like a bad API if so...

No, as far as I understand the method expects no %-escaped characters in the string that it parses. The problem is though that soup backend doesn't remove these characters from the fragment.

[Dan Winship]

ah, ok, if you want *no* %-escaping, you should use soup_uri_decode(), not soup_uri_normalize().

It appears that other browsers decode "%00" to "" rather than "\0"? If so I guess libsoup should be "fixed" to do that too.

[Zan Dobersek]

(In reply to comment #4)
> ah, ok, if you want *no* %-escaping, you should use soup_uri_decode(), not soup_uri_normalize().

Using soup_uri_decode causes regressions, for instance %3D gets converted back to = and similar (see test cases [1] and [2]) so using that obviously isn't good. soup_uri_normalize, with the space character as an extra unreserved character to decode, does the trick with no test regressions, but I'm not sure it's the correct way of handling the issue.

Basically, comment #1 describes all the changes required for these tests to pass without any other regression.

> 
> It appears that other browsers decode "%00" to "" rather than "\0"? If so I guess libsoup should be "fixed" to do that too.

This works as well.


[1] http://trac.webkit.org/browser/trunk/LayoutTests/media/media-fragments/TC0030-TC0039-expected.txt#L30
[2] http://trac.webkit.org/browser/trunk/LayoutTests/http/tests/navigation/anchor-frames-gbk.html

[Dan Winship]

(In reply to comment #5)
> (In reply to comment #4)
> > ah, ok, if you want *no* %-escaping, you should use soup_uri_decode(), not soup_uri_normalize().
> 
> Using soup_uri_decode causes regressions, for instance %3D gets converted back to = and similar (see test cases [1] and [2])

So you *don't* want no %-escaping. :)

> soup_uri_normalize, with the space character as an extra unreserved character to decode, does the trick with no test regressions, but I'm not sure it's the correct way of handling the issue.

It's at least right-ish. There may be other characters you need in the "extra unreserved" list that just don't get tested by the current set of tests... It seems like this would be something that it would be good to have SVGViewSpec::parseViewSpec() explicitly document.

[Zan Dobersek]

Interestingly the tests behave OK on WK2 builds. This basically means that this is possible to solve without any changes to libsoup but that there's rather a problem in the WK1 layer. I'll investigate this a bit to see what WK2 layer does that the WK1 layer doesn't.

[Zan Dobersek]

Created attachment 163582 [details]
Patch

[Martin Robinson]

Comment on attachment 163582 [details]
Patch

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

> Tools/DumpRenderTree/gtk/DumpRenderTree.cpp:1256
> -        webkit_network_request_set_uri(request, "about:blank");
> +        SoupMessage* message = webkit_network_request_get_message(request);
> +        soup_message_set_status(message, SOUP_STATUS_CANCELLED);

This can just be one line.

[Zan Dobersek]

Adjusted and landed in r137198.
http://trac.webkit.org/changeset/137198

[WebKit Review Bot]

Re-opened since this is blocked by bug 105212

[Zan Dobersek]

Comment on attachment 163582 [details]
Patch

Clearing the r+ flag on the committed (yet later rolled-out) patch.

[Zan Dobersek]

This somehow gets fixed by bug #105667.
It already landed in r140005, but later got rolled out. Will remove expectations when it gets relanded and close this bug.
http://trac.webkit.org/changeset/140005

[Zan Dobersek]

(In reply to comment #13)
> This somehow gets fixed by bug #105667.
> It already landed in r140005, but later got rolled out. Will remove expectations when it gets relanded and close this bug.
> http://trac.webkit.org/changeset/140005

That landed and the tests pass now. Closing.