Bug 169042 – REGRESSION (r213202?): Assertion failed: (!"initialized()"), function operator()

• Component: New Bugs
• Status: RESOLVED FIXED

[Ryan Haddad]

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

[Ryan Haddad]

Created attachment 303075 [details]
Crash log

[Ryan Haddad]

Related to https://trac.webkit.org/changeset/213202?

[Ryan Haddad]

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

[Mark Lam]

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.

[Mark Lam]

Thanks for the review.  Landed in r213229: <http://trac.webkit.org/r213229>.

[Mark Lam]

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.

[Mark Lam]

I've rolled out the relevant patches in r213231: <http://trac.webkit.org/r213231>.

[Saam Barati]

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

[Saam Barati]

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

[Saam Barati]

(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