RESOLVED FIXED 171119
ASSERTION FAILED: m_table seen with workers/wasm-hashset LayoutTests
https://bugs.webkit.org/show_bug.cgi?id=171119
Summary ASSERTION FAILED: m_table seen with workers/wasm-hashset LayoutTests
Ryan Haddad
Reported 2017-04-21 10:36:09 PDT
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
Attachments
patch (3.36 KB, patch)
2017-04-23 19:16 PDT, Saam Barati
no flags
Radar WebKit Bug Importer
Comment 1 2017-04-21 10:36:52 PDT
Ryan Haddad
Comment 2 2017-04-21 11:35:23 PDT
These tests were added with http://trac.webkit.org/changeset/215353
Saam Barati
Comment 3 2017-04-21 12:56:22 PDT
This looks interesting. My only guess is accesses to the hashtable are racy. Trying to figure out how that could happen.
Saam Barati
Comment 4 2017-04-21 12:58:51 PDT
(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.
Saam Barati
Comment 5 2017-04-23 19:16:55 PDT
Filip Pizlo
Comment 6 2017-04-23 19:19:53 PDT
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.
Saam Barati
Comment 7 2017-04-23 19:24:26 PDT
(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.
Saam Barati
Comment 8 2017-04-23 19:29:29 PDT
(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.
Filip Pizlo
Comment 9 2017-04-23 19:30:29 PDT
(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.
Keith Miller
Comment 10 2017-04-24 13:29:06 PDT
Comment on attachment 307947 [details] patch r=me.
WebKit Commit Bot
Comment 11 2017-04-24 14:11:47 PDT
Comment on attachment 307947 [details] patch Clearing flags on attachment: 307947 Committed r215694: <http://trac.webkit.org/changeset/215694>
WebKit Commit Bot
Comment 12 2017-04-24 14:11:49 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.