Bug 225551 - [GLIB] REGRESSION(r277158) imported/w3c/web-platform-tests/xhr/FormData-append.html is crashing
Summary: [GLIB] REGRESSION(r277158) imported/w3c/web-platform-tests/xhr/FormData-appen...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Diego Pino
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-07 18:00 PDT by Diego Pino
Modified: 2021-05-08 09:55 PDT (History)
8 users (show)

See Also:


Attachments
Patch (1.33 KB, patch)
2021-05-07 18:05 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (2.01 KB, patch)
2021-05-07 20:32 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (2.00 KB, patch)
2021-05-07 20:40 PDT, Diego Pino
no flags Details | Formatted Diff | Diff
Patch (2.05 KB, patch)
2021-05-08 03:30 PDT, Diego Pino
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Diego Pino 2021-05-07 18:00:57 PDT
The test started crashing after r277158:

https://results.webkit.org/?suite=layout-tests&test=imported%2Fw3c%2Fweb-platform-tests%2Fxhr%2FFormData-append.html&platform=GTK&platform=WPE&platform=ios&platform=mac

Stack trace:

https://build.webkit.org/results/GTK-Linux-64-bit-Debug-Tests/r277196%20(1013)/imported/w3c/web-platform-tests/xhr/FormData-append-crash-log.txt

Thread 1 (Thread 0x7f29685acec0 (LWP 6316)):
#0  __strlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:65
#1  0x00007f29724d311c in std::char_traits<char>::length(char const*) (__s=0x0) at /usr/include/c++/10.2.0/bits/char_traits.h:357
#2  0x00007f2973895546 in std::basic_string_view<char, std::char_traits<char> >::basic_string_view(char const*) (this=0x7ffc1ed0ede0, __str=0x0) at /usr/include/c++/10.2.0/string_view:128
#3  0x00007f296ed0cc5f in std::filesystem::__cxx11::path::_S_convert(char const*, std::filesystem::__cxx11::__detail::__null_terminated) (__src=0x0) at /usr/include/c++/10.2.0/bits/fs_path.h:541
#4  0x00007f296ed0daa7 in std::filesystem::__cxx11::path::path<char const*, std::filesystem::__cxx11::path>(char const* const&, std::filesystem::__cxx11::path::format) (this=0x7ffc1ed0eec0, __source=@0x7ffc1ed0eea8: 0x0) at /usr/include/c++/10.2.0/bits/fs_path.h:225
#5  0x00007f296ed0b9f5 in WTF::FileSystemImpl::getFileModificationTime(WTF::String const&) (path=...) at ../../Source/WTF/wtf/FileSystem.cpp:667
#6  0x00007f29757cdc39 in WebCore::File::lastModified() const (this=0x7f29248ce3b0) at ../../Source/WebCore/fileapi/File.cpp:113
#7  0x00007f2973d39329 in WebCore::jsFile_lastModifiedGetter(JSC::JSGlobalObject&, WebCore::JSFile&) (lexicalGlobalObject=..., thisObject=...) at WebCore/DerivedSources/JSFile.cpp:285
#8  0x00007f2973d437e9 in WebCore::IDLAttribute<WebCore::JSFile>::get<WebCore::jsFile_lastModifiedGetter, (WebCore::CastedThisErrorBehavior)3>(JSC::JSGlobalObject&, JSC::EncodedJSValue, JSC::PropertyName) (lexicalGlobalObject=..., thisValue=139814311776008, attributeName=...) at ../../Source/WebCore/bindings/js/JSDOMAttribute.h:90
#9  0x00007f2973d3939f in WebCore::jsFile_lastModified(JSC::JSGlobalObject*, JSC::EncodedJSValue, JSC::PropertyName) (lexicalGlobalObject=0x7f290c2e0000, thisValue=139814311776008, attributeName=...) at WebCore/DerivedSources/JSFile.cpp:290
#10 0x00007f296e88685a in JSC::PropertySlot::customGetter(JSC::JSGlobalObject*, JSC::PropertyName) const (this=0x7ffc1ed0f360, globalObject=0x7f290c2e0000, propertyName=...) at ../../Source/JavaScriptCore/runtime/PropertySlot.cpp:46
#11 0x00007f296d248bd3 in JSC::PropertySlot::getValue(JSC::JSGlobalObject*, JSC::PropertyName) const (this=0x7ffc1ed0f360, globalObject=0x7f290c2e0000, propertyName=...) at ../../Source/JavaScriptCore/runtime/PropertySlot.h:408
#12 0x00007f296d2695f5 in JSC::JSValue::get(JSC::JSGlobalObject*, JSC::PropertyName, JSC::PropertySlot&) const (this=0x7ffc1ed0f2a0, globalObject=0x7f290c2e0000, propertyName=..., slot=...) at ../../Source/JavaScriptCore/runtime/JSCJSValueInlines.h:950
#13 0x00007f296e3af5db in JSC::LLInt::performLLIntGetByID(JSC::Instruction const*, JSC::CodeBlock*, JSC::JSGlobalObject*, JSC::JSValue, JSC::Identifier const&, JSC::GetByIdModeMetadata&) (pc=0x7f290c4f55d5, codeBlock=0x7f290d88e2e0, globalObject=0x7f290c2e0000, baseValue=..., ident=..., metadata=...) at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:760
#14 0x00007f296e3afdf9 in JSC::LLInt::llint_slow_path_get_by_id(JSC::CallFrame*, JSC::Instruction const*) (callFrame=0x7ffc1ed0f5b0, pc=0x7f290c4f55d5) at ../../Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:834
#15 0x00007f296d215359 in llint_op_get_by_id () at /app/webkit/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm:97
Comment 1 Diego Pino 2021-05-07 18:05:17 PDT
Created attachment 428068 [details]
Patch
Comment 2 Chris Dumez 2021-05-07 19:14:10 PDT
Note that fileSystemRepresentation() seems wrong for the glib port. It returns CString() instead of CString("") when the string is the empty string. It should only return CString() is the String is null.
Comment 3 Chris Dumez 2021-05-07 19:15:46 PDT
(In reply to Chris Dumez from comment #2)
> Note that fileSystemRepresentation() seems wrong for the glib port. It
> returns CString() instead of CString("") when the string is the empty
> string. It should only return CString() is the String is null.

Maybe fixing Glib's fileSystemRepresentation() would mean we wouldn't need those isEmpty() checks in all FileSystem functions, unless glib ports are really calling those functions will null Strings..
Comment 4 Diego Pino 2021-05-07 20:32:14 PDT
Created attachment 428076 [details]
Patch
Comment 5 Diego Pino 2021-05-07 20:33:58 PDT
(In reply to Chris Dumez from comment #3)
> (In reply to Chris Dumez from comment #2)
> > Note that fileSystemRepresentation() seems wrong for the glib port. It
> > returns CString() instead of CString("") when the string is the empty
> > string. It should only return CString() is the String is null.
> 
> Maybe fixing Glib's fileSystemRepresentation() would mean we wouldn't need
> those isEmpty() checks in all FileSystem functions, unless glib ports are
> really calling those functions will null Strings..

Thanks for the pointer. I also agree fixing this error at GLib is a better fix. Patch updated.
Comment 6 Diego Pino 2021-05-07 20:40:24 PDT
Created attachment 428077 [details]
Patch
Comment 7 Chris Dumez 2021-05-07 20:58:53 PDT
Comment on attachment 428077 [details]
Patch

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

> Source/WTF/wtf/glib/FileSystemGlib.cpp:77
> +        return CString("", 0);

I think this should be:
If (path.isNull())
  Return { };
If (path.isEmpty())
  Return CString(“”);

Seems weird to return an empty string if the input string was null.
Comment 8 Chris Dumez 2021-05-07 20:58:54 PDT
Comment on attachment 428077 [details]
Patch

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

> Source/WTF/wtf/glib/FileSystemGlib.cpp:77
> +        return CString("", 0);

I think this should be:
If (path.isNull())
  Return { };
If (path.isEmpty())
  Return CString(“”);

Seems weird to return an empty string if the input string was null.
Comment 9 Diego Pino 2021-05-08 03:30:33 PDT
Created attachment 428083 [details]
Patch
Comment 10 EWS 2021-05-08 09:54:16 PDT
Committed r277229 (237498@main): <https://commits.webkit.org/237498@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428083 [details].
Comment 11 Radar WebKit Bug Importer 2021-05-08 09:55:14 PDT
<rdar://problem/77697036>