87734 2012-05-29 05:07:29 -0700 REGRESSION (r118735): svg/dom/complex-svgView-specification.html, svg/dom/SVGViewSpec.html, svg/dom/viewspec-parser.html failing on GTK Linux 64-bit Release 2013-02-26 12:20:04 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release/r118736%20(24421)/results.html Gtk, LayoutTestFailure, Regression P2 Normal --- 105212 1 zan zan danw gustavo mrobinson rakuco svillar webkit.review.bot oldest_to_newest 635904 0 zan 2012-05-29 05:07:29 -0700 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 657505 1 zan 2012-06-26 04:59:01 -0700 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 657535 2 danw 2012-06-26 05:37:49 -0700 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... 657705 3 zan 2012-06-26 10:37:08 -0700 (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. 657777 4 danw 2012-06-26 11:27:00 -0700 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. 658377 5 zan 2012-06-26 23:21:34 -0700 (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 658563 6 danw 2012-06-27 06:06:01 -0700 (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. 716926 7 zan 2012-09-11 01:08:37 -0700 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. 718180 8 163582 zan 2012-09-12 04:35:42 -0700 Created attachment 163582 Patch 787411 9 163582 mrobinson 2012-12-10 09:33:37 -0800 Comment on attachment 163582 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. 788192 10 zan 2012-12-11 01:47:11 -0800 Adjusted and landed in r137198. http://trac.webkit.org/changeset/137198 792980 11 webkit.review.bot 2012-12-17 13:30:55 -0800 Re-opened since this is blocked by bug 105212 800125 12 163582 zan 2013-01-04 03:14:01 -0800 Comment on attachment 163582 Patch Clearing the r+ flag on the committed (yet later rolled-out) patch. 810442 13 zan 2013-01-18 03:47:40 -0800 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 842259 14 zan 2013-02-26 12:20:04 -0800 (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. 163582 2012-09-12 04:35:42 -0700 2013-01-04 03:14:01 -0800 Patch bug-87734-20120912043516.patch text/plain 3995 zan U3VidmVyc2lvbiBSZXZpc2lvbjogMTI4Mjg5CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOGZmNmZhNTRlYzc4NTVk NTJjMmM4YThjYjk1ZjVjNWM4NTAwYjRjMy4uNGUzMDEwY2QzN2JjMjU0MTFjZGNjMjlhZGUyNDJk YjNhNGNmNzE1NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEyLTA5LTEyICBaYW4g RG9iZXJzZWsgIDx6YW5kb2JlcnNla0BnbWFpbC5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTiAo cjExODczNSk6IHN2Zy9kb20vY29tcGxleC1zdmdWaWV3LXNwZWNpZmljYXRpb24uaHRtbCwgc3Zn L2RvbS9TVkdWaWV3U3BlYy5odG1sLCBzdmcvZG9tL3ZpZXdzcGVjLXBhcnNlci5odG1sIGZhaWxp bmcgb24gR1RLIExpbnV4IDY0LWJpdCBSZWxlYXNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD04NzczNAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIERvbid0IG1vZGlmeSBSZXNvdXJjZVJlcXVlc3QncyBVUkwg dG8gaGF2ZSB0aGUgdmFsdWUgb2YgdGhlIChlbmNvZGVkKQorICAgICAgICBVUkkgb2YgdGhlIFNv dXBNZXNzYWdlLiBUaGUgb25seSBleGNlcHRpb24gaXMgaW4gdGhlIGNhc2Ugb2YgY2FuY2VsbGVk CisgICAgICAgIFNvdXBNZXNzYWdlIC0gdGhlIFVSTCBvZiB0aGUgUmVzb3VyY2VSZXF1ZXN0IHNo b3VsZCB0aGVuIGJlIG51bGwuCisKKyAgICAgICAgTm8gbmV3IHRlc3RzIC0gZXhpc3RpbmcgdGVz dHMgc2hvdWxkIHNob3cgbm8gcmVncmVzc2lvbnMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9uZXR3 b3JrL3NvdXAvUmVzb3VyY2VSZXF1ZXN0U291cC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSZXNv dXJjZVJlcXVlc3Q6OnVwZGF0ZUZyb21Tb3VwTWVzc2FnZSk6CisKIDIwMTItMDktMTIgIFZpdmVr IEdhbGF0YWdlICA8dml2ZWtnYWxhdGFnZUBnbWFpbC5jb20+CiAKICAgICAgICAgV2ViIEluc3Bl Y3RvcjogSW5zcGVjdG9yQmFja2VuZC5sb2FkRnJvbUpTT05JZk5lZWRlZCBzaG91bGQgdGFrZSB0 aGUgSlNPTiB1cmwgYXMgYXJndW1lbnQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3BsYXRm b3JtL25ldHdvcmsvc291cC9SZXNvdXJjZVJlcXVlc3RTb3VwLmNwcCBiL1NvdXJjZS9XZWJDb3Jl L3BsYXRmb3JtL25ldHdvcmsvc291cC9SZXNvdXJjZVJlcXVlc3RTb3VwLmNwcAppbmRleCBkZGZh MWMzOGRlZTRiM2U5NjI5ODMxZDZhMWI5NDg3YzYzZTA4YjY5Li40OGQ3OWYwNWVhYTI4NDNkMjUz ZmQ5OTgzOWNlMTI5YTNmNGFmYzk5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9y bS9uZXR3b3JrL3NvdXAvUmVzb3VyY2VSZXF1ZXN0U291cC5jcHAKKysrIGIvU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vbmV0d29yay9zb3VwL1Jlc291cmNlUmVxdWVzdFNvdXAuY3BwCkBAIC04NSwx MyArODUsOCBAQCBTb3VwTWVzc2FnZSogUmVzb3VyY2VSZXF1ZXN0Ojp0b1NvdXBNZXNzYWdlKCkg Y29uc3QKIAogdm9pZCBSZXNvdXJjZVJlcXVlc3Q6OnVwZGF0ZUZyb21Tb3VwTWVzc2FnZShTb3Vw TWVzc2FnZSogc291cE1lc3NhZ2UpCiB7Ci0gICAgYm9vbCBzaG91bGRQb3J0QmVSZXNldFRvWmVy byA9IG1fdXJsLmhhc1BvcnQoKSAmJiAhbV91cmwucG9ydCgpOwotICAgIG1fdXJsID0gc291cFVS SVRvS1VSTChzb3VwX21lc3NhZ2VfZ2V0X3VyaShzb3VwTWVzc2FnZSkpOwotCi0gICAgLy8gU291 cFVSSSBjYW5ub3QgZGlmZmVyZW5pYXRlIGJldHdlZW4gYW4gZXhwbGljaXRseSBzcGVjaWZpZWQg cG9ydCAwIGFuZAotICAgIC8vIG5vIHBvcnQgc3BlY2lmaWVkLgotICAgIGlmIChzaG91bGRQb3J0 QmVSZXNldFRvWmVybykKLSAgICAgICAgbV91cmwuc2V0UG9ydCgwKTsKKyAgICBpZiAoc291cE1l c3NhZ2UtPnN0YXR1c19jb2RlID09IFNPVVBfU1RBVFVTX0NBTkNFTExFRCkKKyAgICAgICAgc2V0 VVJMKEtVUkwoKSk7CiAKICAgICBtX2h0dHBNZXRob2QgPSBTdHJpbmc6OmZyb21VVEY4KHNvdXBN ZXNzYWdlLT5tZXRob2QpOwogCmRpZmYgLS1naXQgYS9Ub29scy9DaGFuZ2VMb2cgYi9Ub29scy9D aGFuZ2VMb2cKaW5kZXggMTg2ZjIwYTFhZWI4NTZlMjg1YmFiMGJjMzc0MWQwYjZkNTRmYjY2Ny4u NzljZGI5MmEzZmI5ZGYyZTM1Zjc1ZTNjZmIxZjIzNjZkOTM5YjY1NCAxMDA2NDQKLS0tIGEvVG9v bHMvQ2hhbmdlTG9nCisrKyBiL1Rvb2xzL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDEy LTA5LTEyICBaYW4gRG9iZXJzZWsgIDx6YW5kb2JlcnNla0BnbWFpbC5jb20+CisKKyAgICAgICAg UkVHUkVTU0lPTiAocjExODczNSk6IHN2Zy9kb20vY29tcGxleC1zdmdWaWV3LXNwZWNpZmljYXRp b24uaHRtbCwgc3ZnL2RvbS9TVkdWaWV3U3BlYy5odG1sLCBzdmcvZG9tL3ZpZXdzcGVjLXBhcnNl ci5odG1sIGZhaWxpbmcgb24gR1RLIExpbnV4IDY0LWJpdCBSZWxlYXNlCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD04NzczNAorCisgICAgICAgIFJldmll d2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdoZW4gd2lsbFNlbmRSZXF1ZXN0IHNo b3VsZCByZXR1cm4gbnVsbCBpbiB0ZXN0aW5nLCBjYW5jZWwgdGhlIFNvdXBNZXNzYWdlCisgICAg ICAgIGluc3RlYWQgb2Ygc2V0dGluZyB0aGUgcmVxdWVzdCdzIFVSSSB0byBhYm91dDpibGFuay4K KworICAgICAgICAqIER1bXBSZW5kZXJUcmVlL2d0ay9EdW1wUmVuZGVyVHJlZS5jcHA6CisgICAg ICAgICh3aWxsU2VuZFJlcXVlc3RDYWxsYmFjayk6CisKIDIwMTItMDktMTIgIENocmlzdG9waGUg RHVtZXogIDxjaHJpc3RvcGhlLmR1bWV6QGludGVsLmNvbT4KIAogICAgICAgICBbV0syXVtXS1RS XSBUZXN0UnVubmVyIG5lZWRzIHRvIGltcGxlbWVudCBkdW1wQXBwbGljYXRpb25DYWNoZURlbGVn YXRlQ2FsbGJhY2tzCmRpZmYgLS1naXQgYS9Ub29scy9EdW1wUmVuZGVyVHJlZS9ndGsvRHVtcFJl bmRlclRyZWUuY3BwIGIvVG9vbHMvRHVtcFJlbmRlclRyZWUvZ3RrL0R1bXBSZW5kZXJUcmVlLmNw cAppbmRleCBkYWJiMDlkMjkxMWM4MTJmYmU2YjIzYTk2ODgzZjYyNWYzZWNmODQ3Li40YmRjYWVi OWFmZmMyMDk2YjAzZDdlOWJmNzdiMWExNzRhZDViYThiIDEwMDY0NAotLS0gYS9Ub29scy9EdW1w UmVuZGVyVHJlZS9ndGsvRHVtcFJlbmRlclRyZWUuY3BwCisrKyBiL1Rvb2xzL0R1bXBSZW5kZXJU cmVlL2d0ay9EdW1wUmVuZGVyVHJlZS5jcHAKQEAgLTEyNTAsMTEgKzEyNTAsMTAgQEAgc3RhdGlj IENTdHJpbmcgZGVzY3JpcHRpb25TdWl0YWJsZUZvclRlc3RSZXN1bHQoV2ViS2l0TmV0d29ya1Jl c3BvbnNlKiByZXNwb25zZSkKIAogc3RhdGljIHZvaWQgd2lsbFNlbmRSZXF1ZXN0Q2FsbGJhY2so V2ViS2l0V2ViVmlldyogd2ViVmlldywgV2ViS2l0V2ViRnJhbWUqIHdlYkZyYW1lLCBXZWJLaXRX ZWJSZXNvdXJjZSogcmVzb3VyY2UsIFdlYktpdE5ldHdvcmtSZXF1ZXN0KiByZXF1ZXN0LCBXZWJL aXROZXR3b3JrUmVzcG9uc2UqIHJlc3BvbnNlKQogewotCi0KICAgICBpZiAoIWRvbmUgJiYgZ1Rl c3RSdW5uZXItPndpbGxTZW5kUmVxdWVzdFJldHVybnNOdWxsKCkpIHsKICAgICAgICAgLy8gQXMg cmVxdWVzdGVkIGJ5IHRoZSBUZXN0UnVubmVyLCBkb24ndCBwZXJmb3JtIHRoZSByZXF1ZXN0Lgot ICAgICAgICB3ZWJraXRfbmV0d29ya19yZXF1ZXN0X3NldF91cmkocmVxdWVzdCwgImFib3V0OmJs YW5rIik7CisgICAgICAgIFNvdXBNZXNzYWdlKiBtZXNzYWdlID0gd2Via2l0X25ldHdvcmtfcmVx dWVzdF9nZXRfbWVzc2FnZShyZXF1ZXN0KTsKKyAgICAgICAgc291cF9tZXNzYWdlX3NldF9zdGF0 dXMobWVzc2FnZSwgU09VUF9TVEFUVVNfQ0FOQ0VMTEVEKTsKICAgICAgICAgcmV0dXJuOwogICAg IH0KIAo=