Bug 33971 – Check base URI validity before using in SVGImageLoader::sourceURI

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: SVG

[Bryan Yeung]

If the element has no baseURI, then the sourceURI method will use an invalidated KURL object.

[Bryan Yeung]

Created attachment 47145 [details]
patch checks for URL validity before using

[Darin Adler]

Comment on attachment 47145 [details]
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.

[Bryan Yeung]

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

[Bryan Yeung]

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.

[Cosmin Truta]

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.

[Cosmin Truta]

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

[Fady Samuel]

*** This bug has been marked as a duplicate of bug 55750 ***