Thread 19 Crashed:: WTF::AutomaticThread 0 libsystem_kernel.dylib 0x00007fff84e6df06 __pthread_kill + 10 1 libsystem_pthread.dylib 0x00007fff83f7b4ec pthread_kill + 90 2 libsystem_c.dylib 0x00007fff903366df abort + 129 3 libsystem_c.dylib 0x00007fff902fddd8 __assert_rtn + 321 4 com.apple.JavaScriptCore 0x0000000105e3a41e std::optional<unsigned int>::operator*() const::'lambda'()::operator()() const + 46 5 com.apple.JavaScriptCore 0x0000000105e3a3b1 std::optional<unsigned int>::operator*() const + 65 (Optional.h:595) 6 com.apple.JavaScriptCore 0x0000000105e3a31f bool std::operator==<unsigned int>(std::optional<unsigned int> const&, unsigned int const&) + 47 (Optional.h:863) 7 com.apple.JavaScriptCore 0x0000000105e3a2e0 JSC::JSLock::currentThreadIsHoldingLock() + 48 (JSLock.h:97) 8 com.apple.JavaScriptCore 0x0000000105e3a284 JSC::VM::currentThreadIsHoldingAPILock() const + 36 (VM.h:629) 9 com.apple.JavaScriptCore 0x0000000105e39dde JSC::JSCell::classInfo(JSC::VM&) const + 62 (JSCellInlines.h:283) 10 com.apple.JavaScriptCore 0x0000000106fd0845 JSC::validate(JSC::JSCell*) + 181 (SlotVisitor.cpp:61) 11 com.apple.JavaScriptCore 0x0000000106fd06aa JSC::SlotVisitor::setMarkedAndAppendToMarkStack(JSC::JSCell*) + 138 (SlotVisitor.cpp:256) 12 com.apple.JavaScriptCore 0x0000000106fd0615 JSC::SlotVisitor::appendUnbarriered(JSC::JSValue) + 181 (SlotVisitor.cpp:234) 13 com.apple.JavaScriptCore 0x0000000105e4e985 JSC::SlotVisitor::appendUnbarriered(JSC::JSCell*) + 53 (SlotVisitorInlines.h:43) 14 com.apple.JavaScriptCore 0x00000001070056f0 void JSC::SlotVisitor::append<JSC::PropertyTable>(JSC::WriteBarrierBase<JSC::PropertyTable> const&) + 48 (SlotVisitorInlines.h:55) 15 com.apple.JavaScriptCore 0x0000000106ffe468 JSC::Structure::visitChildren(JSC::JSCell*, JSC::SlotVisitor&) + 552 (Structure.cpp:1095) 16 com.apple.JavaScriptCore 0x0000000106fd37f4 JSC::SlotVisitor::visitChildren(JSC::JSCell const*) + 292 (SlotVisitor.cpp:400) 17 com.apple.JavaScriptCore 0x0000000106fd2e0e JSC::SlotVisitor::drain(WTF::MonotonicTime)::$_3::operator()(JSC::MarkStackArray&) const + 206 (SlotVisitor.cpp:494) 18 com.apple.JavaScriptCore 0x0000000106fd0f51 JSC::IterationStatus JSC::SlotVisitor::forEachMarkStack<JSC::SlotVisitor::drain(WTF::MonotonicTime)::$_3>(JSC::SlotVisitor::drain(WTF::MonotonicTime)::$_3 const&) + 33 (SlotVisitorInlines.h:116) 19 com.apple.JavaScriptCore 0x0000000106fd0edb JSC::SlotVisitor::drain(WTF::MonotonicTime) + 187 (SlotVisitor.cpp:485) 20 com.apple.JavaScriptCore 0x0000000106fd1a21 JSC::SlotVisitor::drainFromShared(JSC::SlotVisitor::SharedDrainMode, WTF::MonotonicTime) + 1153 (SlotVisitor.cpp:665) 21 com.apple.JavaScriptCore 0x00000001069eb610 JSC::Heap::runBeginPhase(JSC::GCConductor)::$_11::operator()() const + 1056 (Heap.cpp:1152) 22 com.apple.JavaScriptCore 0x00000001069eb1bc WTF::SharedTaskFunctor<void (), JSC::Heap::runBeginPhase(JSC::GCConductor)::$_11>::run() + 28 (SharedTask.h:90) 23 com.apple.JavaScriptCore 0x0000000107394390 WTF::ParallelHelperClient::runTask(WTF::RefPtr<WTF::SharedTask<void ()> >) + 176 (ParallelHelperPool.cpp:115) 24 com.apple.JavaScriptCore 0x00000001073953b2 WTF::ParallelHelperPool::Thread::work() + 66 (ParallelHelperPool.cpp:194) 25 com.apple.JavaScriptCore 0x000000010735b6ee WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0::operator()() const + 606 (AutomaticThread.cpp:215) 26 com.apple.JavaScriptCore 0x000000010735b47d void std::__1::__invoke_void_return_wrapper<void>::__call<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0&>(WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0&&&) + 45 (__functional_base:441) 27 com.apple.JavaScriptCore 0x000000010735b26c std::__1::__function::__func<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0, std::__1::allocator<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0>, void ()>::operator()() + 44 (functional:1407) 28 com.apple.JavaScriptCore 0x000000010677b67a std::__1::function<void ()>::operator()() const + 26 (functional:1793) 29 com.apple.JavaScriptCore 0x00000001073cc60e WTF::threadEntryPoint(void*) + 158 (Threading.cpp:92) 30 com.apple.JavaScriptCore 0x00000001073ce2c1 WTF::wtfThreadEntryPoint(void*) + 289 (ThreadingPthreads.cpp:168) 31 libsystem_pthread.dylib 0x00007fff83f7899d _pthread_body + 131 32 libsystem_pthread.dylib 0x00007fff83f7891a _pthread_start + 168 33 libsystem_pthread.dylib 0x00007fff83f76351 thread_start + 13 Seen here with imported/w3c/web-platform-tests/dom/nodes/Document-constructor.html and imported/w3c/web-platform-tests/streams/readable-streams/bad-underlying-sources.dedicatedworker.html https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r213221%20(11340)/results.html
Created attachment 303075 [details] Crash log
Related to https://trac.webkit.org/changeset/213202?
Also here with imported/w3c/web-platform-tests/IndexedDB/interfaces.worker.html: https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK1%20(Tests)/r213223%20(11858)/results.html
Created attachment 303076 [details] proposed patch. According to my reading of the spec, it should be legal to compare an uninitialized optional with a value. The result should be less than i.e. not equal. Hence, I think the implementation of Optional is incorrect. But let's just workaround this for now in JSLock while I re-read the spec for optional more carefully.
Thanks for the review. Landed in r213229: <http://trac.webkit.org/r213229>.
I found the issue. Our implementation of std::optional does correctly handle comparisons using uninitialized optionals. The real issue here is that optionals are not thread safe. The equality test is implemented as: step 1: check if the optional is initialized. If initialized, goto step 2, else return false. step 2: compare the value in the optional. The crash is due to the following scenario: thread 2: checks and sees that the optional is initialized. thread 1: resets the optional i..e now uninitialized. thread 2: compares the optional's value ... and finds it uninitialized, and hence, crash on a failed assertion. I will roll out my patches until I have a proper fix for this.
I've rolled out the relevant patches in r213231: <http://trac.webkit.org/r213231>.
(In reply to Mark Lam from comment #6) > I found the issue. Our implementation of std::optional does correctly > handle comparisons using uninitialized optionals. The real issue here is > that optionals are not thread safe. The equality test is implemented as: > > step 1: check if the optional is initialized. If initialized, goto step 2, > else return false. > step 2: compare the value in the optional. > > The crash is due to the following scenario: > thread 2: checks and sees that the optional is initialized. > thread 1: resets the optional i..e now uninitialized. > thread 2: compares the optional's value ... and finds it uninitialized, and > hence, crash on a failed assertion. > > I will roll out my patches until I have a proper fix for this. Why optional<RefPtr? Seems like a null RefPtr is what you want....
(In reply to Saam Barati from comment #8) > (In reply to Mark Lam from comment #6) > > I found the issue. Our implementation of std::optional does correctly > > handle comparisons using uninitialized optionals. The real issue here is > > that optionals are not thread safe. The equality test is implemented as: > > > > step 1: check if the optional is initialized. If initialized, goto step 2, > > else return false. > > step 2: compare the value in the optional. > > > > The crash is due to the following scenario: > > thread 2: checks and sees that the optional is initialized. > > thread 1: resets the optional i..e now uninitialized. > > thread 2: compares the optional's value ... and finds it uninitialized, and > > hence, crash on a failed assertion. > > > > I will roll out my patches until I have a proper fix for this. > > Why optional<RefPtr? > Seems like a null RefPtr is what you want.... Nevermind, I see the function just returns a optional<RefPtr, which is still kind of weird...
(In reply to Saam Barati from comment #9) > (In reply to Saam Barati from comment #8) > > (In reply to Mark Lam from comment #6) > > > I found the issue. Our implementation of std::optional does correctly > > > handle comparisons using uninitialized optionals. The real issue here is > > > that optionals are not thread safe. The equality test is implemented as: > > > > > > step 1: check if the optional is initialized. If initialized, goto step 2, > > > else return false. > > > step 2: compare the value in the optional. > > > > > > The crash is due to the following scenario: > > > thread 2: checks and sees that the optional is initialized. > > > thread 1: resets the optional i..e now uninitialized. > > > thread 2: compares the optional's value ... and finds it uninitialized, and > > > hence, crash on a failed assertion. > > > > > > I will roll out my patches until I have a proper fix for this. > > > > Why optional<RefPtr? > > Seems like a null RefPtr is what you want.... > > Nevermind, I see the function just returns a optional<RefPtr, which is still > kind of weird... RefPtr can be null