171119 2017-04-21 10:36:09 -0700 ASSERTION FAILED: m_table seen with workers/wasm-hashset LayoutTests 2017-04-24 14:11:49 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 ryanhaddad saam benjamin commit-queue fpizlo ggaren gskachkov jfbastien keith_miller mark.lam msaboff saam ticaiolima webkit-bug-importer ysuzuki oldest_to_newest 1299682 0 ryanhaddad 2017-04-21 10:36:09 -0700 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 1299683 1 webkit-bug-importer 2017-04-21 10:36:52 -0700 <rdar://problem/31760635> 1299727 2 ryanhaddad 2017-04-21 11:35:23 -0700 These tests were added with http://trac.webkit.org/changeset/215353 1299784 3 saam 2017-04-21 12:56:22 -0700 This looks interesting. My only guess is accesses to the hashtable are racy. Trying to figure out how that could happen. 1299787 4 saam 2017-04-21 12:58:51 -0700 (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. 1300394 5 307947 saam 2017-04-23 19:16:55 -0700 Created attachment 307947 patch 1300395 6 307947 fpizlo 2017-04-23 19:19:53 -0700 Comment on attachment 307947 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. 1300397 7 saam 2017-04-23 19:24:26 -0700 (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. 1300398 8 saam 2017-04-23 19:29:29 -0700 (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. 1300399 9 fpizlo 2017-04-23 19:30:29 -0700 (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. 1300639 10 307947 keith_miller 2017-04-24 13:29:06 -0700 Comment on attachment 307947 patch r=me. 1300656 11 307947 commit-queue 2017-04-24 14:11:47 -0700 Comment on attachment 307947 patch Clearing flags on attachment: 307947 Committed r215694: <http://trac.webkit.org/changeset/215694> 1300657 12 commit-queue 2017-04-24 14:11:49 -0700 All reviewed patches have been landed. Closing bug. 307947 2017-04-23 19:16:55 -0700 2017-04-24 14:11:47 -0700 patch c-backup.diff text/plain 3442 saam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMjE1Njc5KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBA CisyMDE3LTA0LTIzICBTYWFtIEJhcmF0aSAgPHNiYXJhdGlAYXBwbGUuY29tPgorCisgICAgICAg IEFTU0VSVElPTiBGQUlMRUQ6IG1fdGFibGUgc2VlbiB3aXRoIHdvcmtlcnMvd2FzbS1oYXNoc2V0 IExheW91dFRlc3RzCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xNzExMTkKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzMxNzYwNjM1PgorCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBIYXNoU2V0IG9mIHRp bWVyIHNldCBub3RpZmljYXRpb24gY2FsbGJhY2tzIGNhbiBiZSBhY2Nlc3NlZAorICAgICAgICBh bmQgYXVnbWVudGVkIHNpbXVsdGFuZW91c2x5IGZyb20gZGlmZmVyZW50IHRocmVhZHMuIGUuZywg dGhlIHdvcmtlcgorICAgICAgICB0aHJlYWQgY2FuIGF1Z21lbnQgaXQgd2hpbGUgdGhlIHdhc20g Y29tcGlsYXRpb24gdGhyZWFkIHdpbGwKKyAgICAgICAgYWNjZXNzIGl0LiBUaGVyZWZvcmUsIGFj Y2Vzc2VzIG11c3QgYmUgZ3VhcmRlZCBieSBhIGxvY2suCisKKyAgICAgICAgKiBydW50aW1lL0pT UnVuTG9vcFRpbWVyLmNwcDoKKyAgICAgICAgKEpTQzo6SlNSdW5Mb29wVGltZXI6OnNjaGVkdWxl VGltZXIpOgorICAgICAgICAoSlNDOjpKU1J1bkxvb3BUaW1lcjo6YWRkVGltZXJTZXROb3RpZmlj YXRpb24pOgorICAgICAgICAoSlNDOjpKU1J1bkxvb3BUaW1lcjo6cmVtb3ZlVGltZXJTZXROb3Rp ZmljYXRpb24pOgorICAgICAgICAqIHJ1bnRpbWUvSlNSdW5Mb29wVGltZXIuaDoKKwogMjAxNy0w NC0yMyAgSm9zZXBoIFBlY29yYXJvICA8cGVjb3Jhcm9AYXBwbGUuY29tPgogCiAgICAgICAgIHRl c3QyNjI6IHRlc3QyNjIvdGVzdC9idWlsdC1pbnMvTnVtYmVyL3Byb3RvdHlwZS90b1ByZWNpc2lv bi9uYW4uanMKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTUnVuTG9vcFRp bWVyLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvcnVudGltZS9KU1J1 bkxvb3BUaW1lci5jcHAJKHJldmlzaW9uIDIxNTY2MikKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29y ZS9ydW50aW1lL0pTUnVuTG9vcFRpbWVyLmNwcAkod29ya2luZyBjb3B5KQpAQCAtMTA0LDYgKzEw NCw3IEBAIHZvaWQgSlNSdW5Mb29wVGltZXI6OnNjaGVkdWxlVGltZXIoU2Vjb24KIHsKICAgICBD RlJ1bkxvb3BUaW1lclNldE5leHRGaXJlRGF0ZShtX3RpbWVyLmdldCgpLCBDRkFic29sdXRlVGlt ZUdldEN1cnJlbnQoKSArIGludGVydmFsSW5TZWNvbmRzLnNlY29uZHMoKSk7CiAgICAgbV9pc1Nj aGVkdWxlZCA9IHRydWU7CisgICAgYXV0byBsb2NrZXIgPSBob2xkTG9jayhtX3RpbWVyQ2FsbGJh Y2tzTG9jayk7CiAgICAgZm9yIChhdXRvJiB0YXNrIDogbV90aW1lclNldENhbGxiYWNrcykKICAg ICAgICAgdGFzay0+cnVuKCk7CiB9CkBAIC0xNDIsNiArMTQzLDggQEAgdm9pZCBKU1J1bkxvb3BU aW1lcjo6c2NoZWR1bGVUaW1lcihTZWNvbgogewogICAgIG1fdGltZXIuc3RhcnRPbmVTaG90KGlu dGVydmFsSW5TZWNvbmRzKTsKICAgICBtX2lzU2NoZWR1bGVkID0gdHJ1ZTsKKworICAgIGF1dG8g bG9ja2VyID0gaG9sZExvY2sobV90aW1lckNhbGxiYWNrc0xvY2spOwogICAgIGZvciAoYXV0byYg dGFzayA6IG1fdGltZXJTZXRDYWxsYmFja3MpCiAgICAgICAgIHRhc2stPnJ1bigpOwogfQpAQCAt MTU2LDExICsxNTksMTMgQEAgdm9pZCBKU1J1bkxvb3BUaW1lcjo6Y2FuY2VsVGltZXIoKQogCiB2 b2lkIEpTUnVuTG9vcFRpbWVyOjphZGRUaW1lclNldE5vdGlmaWNhdGlvbihUaW1lck5vdGlmaWNh dGlvbkNhbGxiYWNrIGNhbGxiYWNrKQogeworICAgIGF1dG8gbG9ja2VyID0gaG9sZExvY2sobV90 aW1lckNhbGxiYWNrc0xvY2spOwogICAgIG1fdGltZXJTZXRDYWxsYmFja3MuYWRkKGNhbGxiYWNr KTsKIH0KIAogdm9pZCBKU1J1bkxvb3BUaW1lcjo6cmVtb3ZlVGltZXJTZXROb3RpZmljYXRpb24o VGltZXJOb3RpZmljYXRpb25DYWxsYmFjayBjYWxsYmFjaykKIHsKKyAgICBhdXRvIGxvY2tlciA9 IGhvbGRMb2NrKG1fdGltZXJDYWxsYmFja3NMb2NrKTsKICAgICBtX3RpbWVyU2V0Q2FsbGJhY2tz LnJlbW92ZShjYWxsYmFjayk7CiB9CiAKSW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9ydW50 aW1lL0pTUnVuTG9vcFRpbWVyLmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3Jl L3J1bnRpbWUvSlNSdW5Mb29wVGltZXIuaAkocmV2aXNpb24gMjE1NjYyKQorKysgU291cmNlL0ph dmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNSdW5Mb29wVGltZXIuaAkod29ya2luZyBjb3B5KQpAQCAt NjIsNiArNjIsMTAgQEAgcHVibGljOgogICAgIHZvaWQgY2FuY2VsVGltZXIoKTsKICAgICBib29s IGlzU2NoZWR1bGVkKCkgY29uc3QgeyByZXR1cm4gbV9pc1NjaGVkdWxlZDsgfQogCisgICAgLy8g Tm90ZTogVGhlIG9ubHkgdGhpbmcgdGhlIHRpbWVyIG5vdGlmaWNhdGlvbiBjYWxsYmFjayBjYW5u b3QgZG8gaXMKKyAgICAvLyBjYWxsIHNjaGVkdWxlVGltZXIoKS4gVGhpcyB3aWxsIGNhdXNlIGEg ZGVhZGxvY2suIEl0IHdvdWxkIG5vdAorICAgIC8vIGJlIGhhcmQgdG8gbWFrZSB0aGlzIHdvcmss IGhvd2V2ZXIsIHRoZXJlIGFyZSBubyBjbGllbnRzIHRoYXQgbmVlZAorICAgIC8vIHRoaXMgYmVo YXZpb3IuIFdlIHNob3VsZCBpbXBsZW1lbnQgaXQgb25seSBpZiB3ZSBmaW5kIHRoYXQgd2UgbmVl ZCBpdC4KICAgICBKU19FWFBPUlRfUFJJVkFURSB2b2lkIGFkZFRpbWVyU2V0Tm90aWZpY2F0aW9u KFRpbWVyTm90aWZpY2F0aW9uQ2FsbGJhY2spOwogICAgIEpTX0VYUE9SVF9QUklWQVRFIHZvaWQg cmVtb3ZlVGltZXJTZXROb3RpZmljYXRpb24oVGltZXJOb3RpZmljYXRpb25DYWxsYmFjayk7CiAK QEAgLTg3LDYgKzkxLDcgQEAgcHJvdGVjdGVkOgogICAgIFJ1bkxvb3A6OlRpbWVyPEpTUnVuTG9v cFRpbWVyPiBtX3RpbWVyOwogI2VuZGlmCiAKKyAgICBMb2NrIG1fdGltZXJDYWxsYmFja3NMb2Nr OwogICAgIEhhc2hTZXQ8VGltZXJOb3RpZmljYXRpb25DYWxsYmFjaz4gbV90aW1lclNldENhbGxi YWNrczsKICAgICAKIHByaXZhdGU6Cg==