ASSERTION FAILED: m_table /Volumes/Data/slave/sierra-debug/build/WebKitBuild/Debug/usr/local/include/wtf/HashTable.h(212) : void WTF::HashTableConstIterator<WTF::RefPtr<WTF::SharedTask<void ()> >, WTF::RefPtr<WTF::SharedTask<void ()> >, WTF::IdentityExtractor, WTF::PtrHash<WTF::RefPtr<WTF::SharedTask<void ()> > >, WTF::HashTraits<WTF::RefPtr<WTF::SharedTask<void ()> > >, WTF::HashTraits<WTF::RefPtr<WTF::SharedTask<void ()> > > >::checkValidity() const [Key = WTF::RefPtr<WTF::SharedTask<void ()> >, Value = WTF::RefPtr<WTF::SharedTask<void ()> >, Extractor = WTF::IdentityExtractor, HashFunctions = WTF::PtrHash<WTF::RefPtr<WTF::SharedTask<void ()> > >, Traits = WTF::HashTraits<WTF::RefPtr<WTF::SharedTask<void ()> > >, KeyTraits = WTF::HashTraits<WTF::RefPtr<WTF::SharedTask<void ()> > >] 1 0x103e7644d WTFCrash 2 0x1034f1f07 WTF::HashTableConstIterator<WTF::RefPtr<WTF::SharedTask<void ()> >, WTF::RefPtr<WTF::SharedTask<void ()> >, WTF::IdentityExtractor, WTF::PtrHash<WTF::RefPtr<WTF::SharedTask<void ()> > >, WTF::HashTraits<WTF::RefPtr<WTF::SharedTask<void ()> > >, WTF::HashTraits<WTF::RefPtr<WTF::SharedTask<void ()> > > >::checkValidity() const 3 0x1034f1e59 WTF::HashTableConstIterator<WTF::RefPtr<WTF::SharedTask<void ()> >, WTF::RefPtr<WTF::SharedTask<void ()> >, WTF::IdentityExtractor, WTF::PtrHash<WTF::RefPtr<WTF::SharedTask<void ()> > >, WTF::HashTraits<WTF::RefPtr<WTF::SharedTask<void ()> > >, WTF::HashTraits<WTF::RefPtr<WTF::SharedTask<void ()> > > >::operator++() 4 0x1034f03e3 WTF::HashTableConstIteratorAdapter<WTF::HashTable<WTF::RefPtr<WTF::SharedTask<void ()> >, WTF::RefPtr<WTF::SharedTask<void ()> >, WTF::IdentityExtractor, WTF::PtrHash<WTF::RefPtr<WTF::SharedTask<void ()> > >, WTF::HashTraits<WTF::RefPtr<WTF::SharedTask<void ()> > >, WTF::HashTraits<WTF::RefPtr<WTF::SharedTask<void ()> > > >, WTF::RefPtr<WTF::SharedTask<void ()> > >::operator++() 5 0x1034f0266 JSC::JSRunLoopTimer::scheduleTimer(WTF::Seconds) 6 0x103be7094 JSC::PromiseDeferredTimer::scheduleWorkSoon(JSC::JSPromiseDeferred*, std::__1::function<void ()>&&) 7 0x103dd5b17 JSC::webAssemblyCompileFunc(JSC::ExecState*)::$_0::operator()(JSC::VM&, WTF::Expected<WTF::RefPtr<JSC::Wasm::Module>, WTF::String>&&) 8 0x103dd5994 WTF::SharedTaskFunctor<void (JSC::VM&, WTF::Expected<WTF::RefPtr<JSC::Wasm::Module>, WTF::String>&&), JSC::webAssemblyCompileFunc(JSC::ExecState*)::$_0>::run(JSC::VM&, WTF::Expected<WTF::RefPtr<JSC::Wasm::Module>, WTF::String>&&) 9 0x10350573d JSC::Wasm::makeValidationCallback(WTF::RefPtr<WTF::SharedTask<void (JSC::VM&, WTF::Expected<WTF::RefPtr<JSC::Wasm::Module>, WTF::String>&&)> >&&)::$_0::operator()(JSC::VM&, JSC::Wasm::Plan&) const 10 0x103505614 WTF::SharedTaskFunctor<void (JSC::VM&, JSC::Wasm::Plan&), JSC::Wasm::makeValidationCallback(WTF::RefPtr<WTF::SharedTask<void (JSC::VM&, WTF::Expected<WTF::RefPtr<JSC::Wasm::Module>, WTF::String>&&)> >&&)::$_0>::run(JSC::VM&, JSC::Wasm::Plan&) 11 0x103d8890b JSC::Wasm::Plan::complete(WTF::AbstractLocker const&) 12 0x103d89023 JSC::Wasm::Plan::parseAndValidateModule() 13 0x102d8d35d JSC::Wasm::Worklist::Thread::work() 14 0x103e84817 WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0::operator()() const 15 0x103e845bd void std::__1::__invoke_void_return_wrapper<void>::__call<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0&>(WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0&&&) 16 0x103e84359 std::__1::__function::__func<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0, std::__1::allocator<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0>, void ()>::operator()() 17 0x10339172a std::__1::function<void ()>::operator()() const 18 0x103eeb1e7 WTF::threadEntryPoint(void*) 19 0x103eed292 WTF::wtfThreadEntryPoint(void*) 20 0x7fffb3d089af _pthread_body 21 0x7fffb3d088fb _pthread_body 22 0x7fffb3d08101 thread_start https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK1%20(Tests)/r215598%20(702)/results.html https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=workers%2Fwasm-hashset
<rdar://problem/31760635>
These tests were added with http://trac.webkit.org/changeset/215353
This looks interesting. My only guess is accesses to the hashtable are racy. Trying to figure out how that could happen.
(In reply to Saam Barati from comment #3) > This looks interesting. My only guess is accesses to the hashtable are racy. > Trying to figure out how that could happen. Actually this seems fairly obvious: wasm code finishes compiling at arbitrary times, which could cause it to schedule a timer. Scheduling a timer will cause it to add to the hashmap. However, we might be adding/removing our callback from the hashmap as this happens in the worker run loop.
Created attachment 307947 [details] patch
Comment on attachment 307947 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=307947&action=review > Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:109 > + auto locker = holdLock(m_timerCallbacksLock); > for (auto& task : m_timerSetCallbacks) > task->run(); Are we sure that the tasks are safe to run while holding this lock? Would it be possible to do this instead: Vector<Thingy> tasks; { auto locker = holdLock(m_timerCallbacksLock); for (auto& task : m_timerSetCallbacks) tasks.append(task); } for (auto& task : tasks) task->run(); If a task can run arbitrary JS, then you definitely must do this, since arbitary JS may do something that wants to graph the timerCallbacksLock.
(In reply to Filip Pizlo from comment #6) > Comment on attachment 307947 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=307947&action=review > > > Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:109 > > + auto locker = holdLock(m_timerCallbacksLock); > > for (auto& task : m_timerSetCallbacks) > > task->run(); > > Are we sure that the tasks are safe to run while holding this lock? > > Would it be possible to do this instead: > > Vector<Thingy> tasks; > { > auto locker = holdLock(m_timerCallbacksLock); > for (auto& task : m_timerSetCallbacks) > tasks.append(task); > } > for (auto& task : tasks) > task->run(); > > If a task can run arbitrary JS, then you definitely must do this, since > arbitary JS may do something that wants to graph the timerCallbacksLock. The tasks are not used to run arbitrary JS at the moment. Currently, the only use of this API is inside WorkerRunLoop. The notification is used to notify the WorkerRunLoop that it needs to loop around to ensure it has a proper time out. When writing the patch, I thought about doing your exact suggestion, but thought it was overkill since we don't need it right now. Perhaps it's worth doing it just to future proof ourselves from the foot gun.
(In reply to Saam Barati from comment #7) > (In reply to Filip Pizlo from comment #6) > > Comment on attachment 307947 [details] > > patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=307947&action=review > > > > > Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:109 > > > + auto locker = holdLock(m_timerCallbacksLock); > > > for (auto& task : m_timerSetCallbacks) > > > task->run(); > > > > Are we sure that the tasks are safe to run while holding this lock? > > > > Would it be possible to do this instead: > > > > Vector<Thingy> tasks; > > { > > auto locker = holdLock(m_timerCallbacksLock); > > for (auto& task : m_timerSetCallbacks) > > tasks.append(task); > > } > > for (auto& task : tasks) > > task->run(); > > > > If a task can run arbitrary JS, then you definitely must do this, since > > arbitary JS may do something that wants to graph the timerCallbacksLock. > > The tasks are not used to run arbitrary JS at the moment. Currently, the > only use of this API is inside WorkerRunLoop. The notification is used to > notify the WorkerRunLoop that it needs to loop around to ensure it has a > proper time out. > > When writing the patch, I thought about doing your exact suggestion, but > thought it was overkill since we don't need it right now. Perhaps it's worth > doing it just to future proof ourselves from the foot gun. Actually, this brings up a weird corner case: you can ask for your callback to be removed, but it could run after removal. I'm not sure which semantics we prefer.
(In reply to Saam Barati from comment #8) > (In reply to Saam Barati from comment #7) > > (In reply to Filip Pizlo from comment #6) > > > Comment on attachment 307947 [details] > > > patch > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=307947&action=review > > > > > > > Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp:109 > > > > + auto locker = holdLock(m_timerCallbacksLock); > > > > for (auto& task : m_timerSetCallbacks) > > > > task->run(); > > > > > > Are we sure that the tasks are safe to run while holding this lock? > > > > > > Would it be possible to do this instead: > > > > > > Vector<Thingy> tasks; > > > { > > > auto locker = holdLock(m_timerCallbacksLock); > > > for (auto& task : m_timerSetCallbacks) > > > tasks.append(task); > > > } > > > for (auto& task : tasks) > > > task->run(); > > > > > > If a task can run arbitrary JS, then you definitely must do this, since > > > arbitary JS may do something that wants to graph the timerCallbacksLock. > > > > The tasks are not used to run arbitrary JS at the moment. Currently, the > > only use of this API is inside WorkerRunLoop. The notification is used to > > notify the WorkerRunLoop that it needs to loop around to ensure it has a > > proper time out. > > > > When writing the patch, I thought about doing your exact suggestion, but > > thought it was overkill since we don't need it right now. Perhaps it's worth > > doing it just to future proof ourselves from the foot gun. > > Actually, this brings up a weird corner case: > you can ask for your callback to be removed, but it could run after removal. > I'm not sure which semantics we prefer. OK - I don't want to overengineer it either. Seems like it might not be so helpful to do the snapshot vector.
Comment on attachment 307947 [details] patch r=me.
Comment on attachment 307947 [details] patch Clearing flags on attachment: 307947 Committed r215694: <http://trac.webkit.org/changeset/215694>
All reviewed patches have been landed. Closing bug.