Bug 127469 – [GTK] Avoid unnecessary string duplications in FileSystemGtk

• Component: Platform
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We are using fileSystemRepresentation() everywhere internally which returns a CString that always duplicates the string.

[Carlos Garcia Campos]

Created attachment 221956 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 221956 [details]
Patch

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

Looks good to me but your changes unveil an already existing bug. See inline...

> Source/WebCore/platform/gtk/FileSystemGtk.cpp:84
> +    GUniquePtr<gchar> display(g_filename_to_utf8(filename.get(), 0, nullptr, nullptr, nullptr));

This definitely sounds like a bug to me. We're passing 0 as the string length. We should either use -1 if null terminated or the actual string length if known.

[Carlos Garcia Campos]

(In reply to comment #2)
> (From update of attachment 221956 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221956&action=review
> 
> Looks good to me but your changes unveil an already existing bug. See inline...
> 
> > Source/WebCore/platform/gtk/FileSystemGtk.cpp:84
> > +    GUniquePtr<gchar> display(g_filename_to_utf8(filename.get(), 0, nullptr, nullptr, nullptr));
> 
> This definitely sounds like a bug to me. We're passing 0 as the string length. We should either use -1 if null terminated or the actual string length if known.

You are right, that should be -1

[Carlos Garcia Campos]

Created attachment 221964 [details]
Patch

Good catch Sergio! Patch updated

[Martin Robinson]

Comment on attachment 221956 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        We are using fileSystemRepresentation() everywhere internally
> +        which returns a CString that always duplicates the string.
> +        Add unescapedFilename() internal helper function that returns a
> +        GUniquePtr and used it everywhere instead of fileSystemRepresentation().

I'm not sure this is true. CStrings contain a reference-counted internal buffer.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 221956 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221956&action=review
> 
> > Source/WebCore/ChangeLog:11
> > +        We are using fileSystemRepresentation() everywhere internally
> > +        which returns a CString that always duplicates the string.
> > +        Add unescapedFilename() internal helper function that returns a
> > +        GUniquePtr and used it everywhere instead of fileSystemRepresentation().
> 
> I'm not sure this is true. CStrings contain a reference-counted internal buffer.

The constructor always allocates memory and copies the given string.

[Carlos Garcia Campos]

Check it yourself:

http://trac.webkit.org/browser/trunk/Source/WTF/wtf/text/CString.cpp#L63

[Martin Robinson]

Comment on attachment 221964 [details]
Patch

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

You're right. I neglected to notice that fileSystemRepresentation was converting from a c string into a CString.

> Source/WebCore/platform/gtk/FileSystemGtk.cpp:62
> +    if (path.isEmpty())
> +        return nullptr;
> +    return GUniquePtr<char>(g_uri_unescape_string(path.utf8().data(), nullptr));
> +}

This doesn't appear to be doing the right thing for Windows. In Windows we just need to return path.utf8() to maintain the current behavior.

[Carlos Garcia Campos]

Comment on attachment 221964 [details]
Patch

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

>> Source/WebCore/platform/gtk/FileSystemGtk.cpp:62
>> +}
> 
> This doesn't appear to be doing the right thing for Windows. In Windows we just need to return path.utf8() to maintain the current behavior.

You are right, I forgot the windows ifdef. Will update the patch, thanks!

[Carlos Garcia Campos]

Created attachment 221998 [details]
Updated patch

Fixed the windows behaviour.

[Carlos Garcia Campos]

Committed r162921: <http://trac.webkit.org/changeset/162921>