<rdar://problem/36320331>
Created attachment 338582 [details] Patch
Comment on attachment 338582 [details] Patch Clearing flags on attachment: 338582 Committed r231197: <https://trac.webkit.org/changeset/231197>
All reviewed patches have been landed. Closing bug.
The test added with this change is failing on the 32-bit JSC bot: ** The following JSC stress test failures have been introduced: slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.default slowMicrobenchmarks.yaml/slowMicrobenchmarks/function-constructor-with-huge-strings.js.no-cjit https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1802
Crashed Thread: 0 Dispatch queue: com.apple.main-thread Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x00000000bbadbeef Exception Note: EXC_CORPSE_NOTIFY Termination Signal: Segmentation fault: 11 Termination Reason: Namespace SIGNAL, Code 0xb Terminating Process: exc handler [0] VM Regions Near 0xbbadbeef: Stack 00000000b084d000-00000000b08ce000 [ 516K] rw-/rwx SM=COW --> Stack Guard 00000000bbfea000-00000000bbfeb000 [ 4K] ---/rwx SM=NUL Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.JavaScriptCore 0x003651e6 bmalloc::Heap::allocateLarge(std::__1::unique_lock<bmalloc::Mutex>&, unsigned long, unsigned long) + 86 (Heap.cpp:596) 1 com.apple.JavaScriptCore 0x0035856f bmalloc::Allocator::allocateLarge(unsigned long) + 127 (Allocator.cpp:172) 2 com.apple.JavaScriptCore 0x00358859 bmalloc::Allocator::allocateSlowCase(unsigned long) + 345 (Allocator.cpp:199) 3 com.apple.JavaScriptCore 0x002c3738 bmalloc::Allocator::allocate(unsigned long) + 72 (Allocator.h:91) 4 com.apple.JavaScriptCore 0x00357ff6 bmalloc::Allocator::reallocate(void*, unsigned long) + 694 (Allocator.cpp:126) 5 com.apple.JavaScriptCore 0x002c3a35 bmalloc::Cache::reallocate(bmalloc::HeapKind, void*, unsigned long) + 213 (Cache.h:111) 6 com.apple.JavaScriptCore 0x002c2f31 bmalloc::api::realloc(void*, unsigned long, bmalloc::HeapKind) + 49 (bmalloc.h:69) 7 com.apple.JavaScriptCore 0x00323a64 WTF::stringRealloc(void*, unsigned long) + 52 (StringMalloc.cpp:73) 8 com.apple.JavaScriptCore 0x0031574f WTF::Ref<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> > WTF::StringImpl::reallocateInternal<unsigned char>(WTF::Ref<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >&&, unsigned int, unsigned char*&) + 383 (StringImpl.cpp:224) 9 com.apple.JavaScriptCore 0x003155b1 WTF::StringImpl::reallocate(WTF::Ref<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >&&, unsigned int, unsigned char*&) + 177 (StringImpl.cpp:233) 10 com.apple.JavaScriptCore 0x003105eb void WTF::StringBuilder::reallocateBuffer<unsigned char>(unsigned int) + 395 (StringBuilder.cpp:163) 11 com.apple.JavaScriptCore 0x003128b5 unsigned char* WTF::StringBuilder::appendUninitializedSlow<unsigned char>(unsigned int) + 277 (StringBuilder.cpp:247) 12 com.apple.JavaScriptCore 0x00311666 unsigned char* WTF::StringBuilder::appendUninitialized<unsigned char>(unsigned int) + 454 (StringBuilder.cpp:232) 13 com.apple.JavaScriptCore 0x00311039 WTF::StringBuilder::tryAppend(unsigned char const*, unsigned int) + 169 (StringBuilder.cpp:320) 14 com.apple.JavaScriptCore 0x0112b39c WTF::StringBuilder::tryAppend(WTF::StringView) + 92 (StringBuilder.h:121) 15 com.apple.JavaScriptCore 0x0112efaf JSC::constructFunctionSkippingEvalEnabledCheck(JSC::ExecState*, JSC::JSGlobalObject*, JSC::ArgList const&, JSC::Identifier const&, JSC::SourceOrigin const&, WTF::String const&, WTF::TextPosition const&, int, JSC::FunctionConstructionMode, JSC::JSValue) + 1935 (FunctionConstructor.cpp:141) 16 com.apple.JavaScriptCore 0x0112e7f3 JSC::constructFunction(JSC::ExecState*, JSC::JSGlobalObject*, JSC::ArgList const&, JSC::Identifier const&, JSC::SourceOrigin const&, WTF::String const&, WTF::TextPosition const&, JSC::FunctionConstructionMode, JSC::JSValue) + 499 (FunctionConstructor.cpp:75) 17 com.apple.JavaScriptCore 0x011300d1 JSC::constructFunction(JSC::ExecState*, JSC::JSGlobalObject*, JSC::ArgList const&, JSC::FunctionConstructionMode, JSC::JSValue) + 273 (FunctionConstructor.cpp:222) 18 com.apple.JavaScriptCore 0x0112e3ce JSC::constructWithFunctionConstructor(JSC::ExecState*) + 126 (FunctionConstructor.cpp:44) 19 ??? 0xd1ea807b 0 + 3521806459 20 com.apple.JavaScriptCore 0x0037e0ae llint_entry + 25064 (LowLevelInterpreter.asm:828) 21 com.apple.JavaScriptCore 0x00377d10 vmEntryToJavaScript + 292 (LowLevelInterpreter.asm:555) 22 com.apple.JavaScriptCore 0x00e38829 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 185 (JITCode.cpp:75) 23 com.apple.JavaScriptCore 0x00dcfd93 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) + 6995 (Interpreter.cpp:969) 24 com.apple.JavaScriptCore 0x010fa8b2 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 626 (Completion.cpp:103) 25 jsc 0x000663f0 runWithOptions(GlobalObject*, CommandLine&, bool&) + 2576 (jsc.cpp:2359) 26 jsc 0x00037d8a jscmain(int, char**)::$_3::operator()(JSC::VM&, GlobalObject*, bool&) const + 58 (jsc.cpp:2764) 27 jsc 0x0001d87a int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool, jscmain(int, char**)::$_3 const&) + 1258 (jsc.cpp:2665) 28 jsc 0x0001c010 jscmain(int, char**) + 192 (jsc.cpp:2760) 29 jsc 0x0001bf37 main + 55 (jsc.cpp:2190) 30 libdyld.dylib 0xa73f4611 start + 1
Reverted r231197 for reason: The test added with this change crashes on the 32-bit JSC bot. Committed r231310: <https://trac.webkit.org/changeset/231310>
Created attachment 353269 [details] work in progress. Let's try this on the EWS.
Comment on attachment 353269 [details] work in progress. Attachment 353269 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9763167 Number of test failures exceeded the failure limit.
Created attachment 353270 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 353269 [details] work in progress. Attachment 353269 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9763170 Number of test failures exceeded the failure limit.
Created attachment 353271 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 353269 [details] work in progress. Attachment 353269 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9763149 Number of test failures exceeded the failure limit.
Created attachment 353272 [details] Archive of layout-test-results from ews115 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews115 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 353290 [details] proposed patch.
Comment on attachment 353290 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=353290&action=review > Source/WTF/ChangeLog:40 > + However, I was not able to get clang to export the explicitly instantiated > + instances (despite the methods having being decorated with WTF_EXPORT_PRIVATE). > + Since the StringBuilder is a transient object (not stored for a long time on > + the heap), and the runtime check of the boolean is not that costly compared > + to other work that StringBuilder routinely does (e.g. memcpy), using the > + new ConditionalCrashOnOverflow (which does a runtime check to determine to > + crash or not on overflow) is fine. I don't think this is the end of the world, but it seems like this would be strictly better if you made it a template parameter. Can you at least file a bug? Not doing something because it's tricky to get it to compile correctly isn't a very convincing reason. > Source/WTF/wtf/CheckedArithmetic.h:231 > + typedef typename RemoveChecked<T>::CleanType CleanType; nit: I'd use 'using' here: using CleanType = typename RemoveChecked<T>::CleanType; > Source/WTF/wtf/text/StringBuilder.cpp:40 > + return std::max(requiredLength, std::max(minimumCapacity, std::min(capacity * 2, static_cast<unsigned>(roundUpToMultipleOf<16>(String::MaxLength))))); Why 16? Just because that's the minimum? If so, you should use it by name. Why not just round to multiple of 2? > Source/WTF/wtf/text/StringBuilder.cpp:43 > + please revert
Comment on attachment 353290 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=353290&action=review Thanks for the review. >> Source/WTF/ChangeLog:40 >> + crash or not on overflow) is fine. > > I don't think this is the end of the world, but it seems like this would be strictly better if you made it a template parameter. Can you at least file a bug? Not doing something because it's tricky to get it to compile correctly isn't a very convincing reason. Oh I got it to compile. It's the linking that is the issue. I've spent 1 day googling for a solution to this issue, but couldn't find one. There were other folks who also encountered the issue, and there were folks who came forth with supposed solutions for clang on linux, but no one verified a solution for Mac. The linux solution (which was to declare the equivalent of WTF_EXPORT_PRIVATE after the function declarations) did not work for me. After that, I just decided to move on with this solution. I'll file a bug. >> Source/WTF/wtf/CheckedArithmetic.h:231 >> + typedef typename RemoveChecked<T>::CleanType CleanType; > > nit: I'd use 'using' here: > using CleanType = typename RemoveChecked<T>::CleanType; Will fix. >> Source/WTF/wtf/text/StringBuilder.cpp:40 >> + return std::max(requiredLength, std::max(minimumCapacity, std::min(capacity * 2, static_cast<unsigned>(roundUpToMultipleOf<16>(String::MaxLength))))); > > Why 16? Just because that's the minimum? If so, you should use it by name. > > Why not just round to multiple of 2? I was thinking that a multiple of the minimum capacity is more appropriate than 2, but in the end, all I really care about here is that StringMax is INT_MAX which is 0x7fffffff, and that seems like a bad to pick as a max capacity. So, 2 will actually work, or I can define a maxCapacity constant that is String::MaxLength + 1. I'll go with this maxCapacity constant. >> Source/WTF/wtf/text/StringBuilder.cpp:43 >> + > > please revert Will fix.
Created attachment 353341 [details] patch for landing.
Comment on attachment 353341 [details] patch for landing. Clearing flags on attachment: 353341 Committed r237577: <https://trac.webkit.org/changeset/237577>