Bug 171119 – ASSERTION FAILED: m_table seen with workers/wasm-hashset LayoutTests

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

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

[Radar WebKit Bug Importer]

<rdar://problem/31760635>

[Ryan Haddad]

These tests were added with http://trac.webkit.org/changeset/215353

[Saam Barati]

This looks interesting. My only guess is accesses to the hashtable are racy. Trying to figure out how that could happen.

[Saam Barati]

(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]

Created attachment 307947 [details]
patch

[Filip Pizlo]

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]

(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]

(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]

(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 on attachment 307947 [details]
patch

r=me.

[WebKit Commit Bot]

Comment on attachment 307947 [details]
patch

Clearing flags on attachment: 307947

Committed r215694: <http://trac.webkit.org/changeset/215694>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.