We are using fileSystemRepresentation() everywhere internally which returns a CString that always duplicates the string.
Created attachment 221956 [details] Patch
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.
(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
Created attachment 221964 [details] Patch Good catch Sergio! Patch updated
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.
(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.
Check it yourself: http://trac.webkit.org/browser/trunk/Source/WTF/wtf/text/CString.cpp#L63
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.
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!
Created attachment 221998 [details] Updated patch Fixed the windows behaviour.
Committed r162921: <http://trac.webkit.org/changeset/162921>