33971 2010-01-21 13:55:55 -0800 Check base URI validity before using in SVGImageLoader::sourceURI 2011-03-07 12:48:42 -0800 1 1 1 Unclassified WebKit SVG 528+ (Nightly build) All All RESOLVED DUPLICATE 55750 P3 Minor --- 0 bryeung webkit-unassigned ctruta fsamuel krit zimmermann oldest_to_newest 183307 0 bryeung 2010-01-21 13:55:55 -0800 If the element has no baseURI, then the sourceURI method will use an invalidated KURL object. 183308 1 47145 bryeung 2010-01-21 14:00:19 -0800 Created attachment 47145 patch checks for URL validity before using 183316 2 47145 darin 2010-01-21 14:22:17 -0800 Comment on attachment 47145 patch checks for URL validity before using The call to deprecatedParseURL needs to be on the attr value *before* invoking KURL at all. Making that change alone might fix the bug. Something like this: return KURL(element()->baseURI(), deprecatedParseURL(attr)); This bug needs a test case. review- for lack of a test case and because the fix is probably wrong. 183724 3 bryeung 2010-01-22 11:35:58 -0800 Darin: Thanks for taking the time to look at my patch, and sorry: I should have included a bit more context around this issue. I don't think that the code you suggest addresses the problem that I see: element()->baseURI() is an invalidated KURL object whenever the SVG element does not have an xml:base specified. The code works anyway, because the relative path is still available from the invalidated URL, but it seems dangerous/wrong to make use of invalidated objects. (I've created a separate bug to track this behaviour of KURL: https://bugs.webkit.org/show_bug.cgi?id=34011). Maybe I'm just misunderstanding what it means for a KURL to be invalidated? Further, all of the other subclasses of ImageLoader return the result of deprecatedParseURL. If you're suggesting that this is incorrect, it will presumably need to be fixed everywhere (and has been wrong for quite some time...since rev 36712 which introduced the sourceURI method). I completely agree about the test case: I'll update the patch in a bit to include a new layout test that has images with and without a base URI specified. Thanks again, Bryan 183735 4 bryeung 2010-01-22 12:00:33 -0800 Actually, it looks like the test case I was about to write already exists: LayoutTests/svg/W3C-SVG-1.1/struct-image-07-t.svg This tests image elements with and without an xml:base specified, and exercises both paths in my proposed change. 342812 5 ctruta 2011-01-31 14:23:10 -0800 The following comment currently exists in KURLGoogle.cpp: // When KURL encounters an error such that the URL is invalid and empty // (for example, resolving a relative URL on a non-hierarchical base), it // will produce an isNull URL, and calling setUtf8 will produce an empty // non-null URL. This is unlikely to affect anything, but we preserve this // just in case. It looks like this bug is caused by this behavior. More specifically, here is what's happening if base="" and relative="foo.svg": * KURLGooglePrivate::init assumes (incorrectly?) that url_util::ResolveRelative always sets &output, regardless whether it returns VALID or INVALID. * url_util::ResolveRelative only sets &output if url_canon::IsRelativeURL returns TRUE. * url_canon::IsRelativeURL returns FALSE: - IsRelativeURL does not allow relative URLs if the base scheme does not support it. - base="", therefore it has no scheme. While I agree that a URL composed from base="" and relative="foo.svg" is invalid, I think that the behavior of KURL to set output="foo.svg" is useful (in contrast with the behavior of KURLGooglePrivate to set output=""). In order to resolve this bug, KURLGooglePrivate must agree with KURL. 343797 6 ctruta 2011-02-01 20:18:26 -0800 The failing test that I mentioned in comment #5 is listed in the Chromium issue 42465 http://code.google.com/p/chromium/issues/detail?id=42465 363506 7 fsamuel 2011-03-07 12:48:42 -0800 *** This bug has been marked as a duplicate of bug 55750 *** 47145 2010-01-21 14:00:19 -0800 2010-06-10 20:38:47 -0700 patch checks for URL validity before using patch.txt text/plain 1075 bryeung ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MWY1NTlhOS4uMmM0MTBhOCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvV2Vi Q29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBAQAorMjAxMC0wMS0yMSAgQnJ5YW4gWWV1bmcg IDxicnlldW5nQGNocm9taXVtLm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBVcGRhdGUgc291cmNlVVJJIHRvIGNoZWNrIGZvciBpbnZhbGlkIGJh c2VVUklzLgorCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD0zMzk3MQorCisgICAgICAgICogc3ZnL1NWR0ltYWdlTG9hZGVyLmNwcDoKKyAgICAgICAgKFdl YkNvcmU6OlNWR0ltYWdlTG9hZGVyOjpzb3VyY2VVUkkpOgorCiAyMDEwLTAxLTIxICBBbmRyZWkg UG9wZXNjdSAgPGFuZHJlaXBAZ29vZ2xlLmNvbT4KIAogICAgICAgICBSZXZpZXdlZCBieSBEYXZp ZCBMZXZpbi4KZGlmZiAtLWdpdCBhL1dlYkNvcmUvc3ZnL1NWR0ltYWdlTG9hZGVyLmNwcCBiL1dl YkNvcmUvc3ZnL1NWR0ltYWdlTG9hZGVyLmNwcAppbmRleCA1ZDVkM2JjLi4yNzRlNjVkIDEwMDY0 NAotLS0gYS9XZWJDb3JlL3N2Zy9TVkdJbWFnZUxvYWRlci5jcHAKKysrIGIvV2ViQ29yZS9zdmcv U1ZHSW1hZ2VMb2FkZXIuY3BwCkBAIC01NCw3ICs1NCwxMCBAQCB2b2lkIFNWR0ltYWdlTG9hZGVy OjpkaXNwYXRjaExvYWRFdmVudCgpCiAKIFN0cmluZyBTVkdJbWFnZUxvYWRlcjo6c291cmNlVVJJ KGNvbnN0IEF0b21pY1N0cmluZyYgYXR0cikgY29uc3QKIHsKLSAgICByZXR1cm4gZGVwcmVjYXRl ZFBhcnNlVVJMKEtVUkwoZWxlbWVudCgpLT5iYXNlVVJJKCksIGF0dHIpLnN0cmluZygpKTsKKyAg ICBLVVJMIGJhc2UoZWxlbWVudCgpLT5iYXNlVVJJKCkpOworICAgIGlmICghYmFzZS5pc1ZhbGlk KCkpIHJldHVybiBkZXByZWNhdGVkUGFyc2VVUkwoYXR0cik7CisKKyAgICByZXR1cm4gZGVwcmVj YXRlZFBhcnNlVVJMKEtVUkwoYmFzZSwgYXR0cikuc3RyaW5nKCkpOwogfQogCiB9Cg==