Bug 200504 – Fix thread safety issue under JSHistory::visitAdditionalChildren()

• Status: RESOLVED
• Resolution: FIXED
• Component: Bindings

[Chris Dumez]

Fix thread safety issue under JSHistory::visitAdditionalChildren():

Thread 7 Crashed:: Heap Helper Thread
0   com.apple.JavaScriptCore      	0x0000000109c4e9f0 WTFCrash + 16 (Assertions.cpp:305)
1   com.apple.WebCore             	0x000000011a3fb70b WTFCrashWithInfo(int, char const*, char const*, int) + 27
2   com.apple.WebCore             	0x000000011a5142a9 WTF::WeakPtr<WebCore::DOMWindow>::operator->() const + 153 (WeakPtr.h:115)
3   com.apple.WebCore             	0x000000011d24133f WebCore::DOMWindowProperty::frame() const + 47 (DOMWindowProperty.cpp:42)
4   com.apple.WebCore             	0x000000011d2f17f9 WebCore::History::stateInternal() const + 25 (History.cpp:100)
5   com.apple.WebCore             	0x000000011d2f1890 WebCore::History::stateChanged() const + 32 (History.cpp:111)
6   com.apple.WebCore             	0x000000011d2f1924 WebCore::History::cachedState() + 68 (History.cpp:116)
7   com.apple.WebCore             	0x000000011c1782c4 WebCore::JSHistory::visitAdditionalChildren(JSC::SlotVisitor&) + 36 (JSHistoryCustom.cpp:49)
8   com.apple.WebCore             	0x000000011b076647 WebCore::JSHistory::visitOutputConstraints(JSC::JSCell*, JSC::SlotVisitor&) + 439 (JSHistory.cpp:405)
9   com.apple.WebCore             	0x000000011c11f9d9 WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind)::operator()(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind) const + 105 (DOMGCOutputConstraint.cpp:67)
10  com.apple.WebCore             	0x000000011c11f963 WTF::Ref<WTF::SharedTask<void (JSC::SlotVisitor&)>, WTF::DumbPtrTraits<WTF::SharedTask<void (JSC::SlotVisitor&)> > > JSC::Subspace::forEachMarkedCellInParallel<WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind)>(WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind) const&)::Task::run(JSC::SlotVisitor&)::'lambda'(unsigned long, JSC::HeapCell*, JSC::HeapCell::Kind)::operator()(unsigned long, JSC::HeapCell*, JSC::HeapCell::Kind) const + 67 (SubspaceInlines.h:105)
11  com.apple.WebCore             	0x000000011c11f820 JSC::IterationStatus JSC::MarkedBlock::Handle::forEachMarkedCell<WTF::Ref<WTF::SharedTask<void (JSC::SlotVisitor&)>, WTF::DumbPtrTraits<WTF::SharedTask<void (JSC::SlotVisitor&)> > > JSC::Subspace::forEachMarkedCellInParallel<WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind)>(WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind) const&)::Task::run(JSC::SlotVisitor&)::'lambda'(unsigned long, JSC::HeapCell*, JSC::HeapCell::Kind)>(WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind) const&) + 224 (MarkedBlockInlines.h:546)
12  com.apple.WebCore             	0x000000011c11f455 WTF::Ref<WTF::SharedTask<void (JSC::SlotVisitor&)>, WTF::DumbPtrTraits<WTF::SharedTask<void (JSC::SlotVisitor&)> > > JSC::Subspace::forEachMarkedCellInParallel<WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind)>(WebCore::DOMGCOutputConstraint::executeImpl(JSC::SlotVisitor&)::$_0::operator()(JSC::Subspace&) const::'lambda'(JSC::SlotVisitor&, JSC::HeapCell*, JSC::HeapCell::Kind) const&)::Task::run(JSC::SlotVisitor&) + 101 (SubspaceInlines.h:102)
13  com.apple.JavaScriptCore      	0x000000010ad28620 JSC::MarkingConstraint::doParallelWork(JSC::SlotVisitor&, WTF::SharedTask<void (JSC::SlotVisitor&)>&) + 64 (MarkingConstraint.cpp:87)
14  com.apple.JavaScriptCore      	0x000000010ad2a2cd JSC::MarkingConstraintSolver::runExecutionThread(JSC::SlotVisitor&, JSC::MarkingConstraintSolver::SchedulerPreference, WTF::ScopedLambda<WTF::Optional<unsigned int> ()>) + 477 (MarkingConstraintSolver.cpp:232)
15  com.apple.JavaScriptCore      	0x000000010ad39cc4 JSC::MarkingConstraintSolver::execute(JSC::MarkingConstraintSolver::SchedulerPreference, WTF::ScopedLambda<WTF::Optional<unsigned int> ()>)::$_30::operator()(JSC::SlotVisitor&) const + 68 (MarkingConstraintSolver.cpp:68)
16  com.apple.JavaScriptCore      	0x000000010ad39c54 WTF::SharedTaskFunctor<void (JSC::SlotVisitor&), JSC::MarkingConstraintSolver::execute(JSC::MarkingConstraintSolver::SchedulerPreference, WTF::ScopedLambda<WTF::Optional<unsigned int> ()>)::$_30>::run(JSC::SlotVisitor&) + 52 (SharedTask.h:91)
17  com.apple.JavaScriptCore      	0x000000010ad3db31 JSC::SlotVisitor::drainFromShared(JSC::SlotVisitor::SharedDrainMode, WTF::MonotonicTime) + 1377 (SlotVisitor.cpp:692)
18  com.apple.JavaScriptCore      	0x000000010acde7ca JSC::Heap::runBeginPhase(JSC::GCConductor)::$_19::operator()() const + 234 (Heap.cpp:1319)
19  com.apple.JavaScriptCore      	0x000000010acde669 WTF::SharedTaskFunctor<void (), JSC::Heap::runBeginPhase(JSC::GCConductor)::$_19>::run() + 25 (SharedTask.h:91)
20  com.apple.JavaScriptCore      	0x0000000109cca1f1 WTF::ParallelHelperClient::runTask(WTF::RefPtr<WTF::SharedTask<void ()>, WTF::DumbPtrTraits<WTF::SharedTask<void ()> > > const&) + 241 (ParallelHelperPool.cpp:115)
21  com.apple.JavaScriptCore      	0x0000000109ccb2aa WTF::ParallelHelperPool::Thread::work() + 42 (ParallelHelperPool.cpp:200)
22  com.apple.JavaScriptCore      	0x0000000109c65a2f WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0::operator()() const + 671 (AutomaticThread.cpp:223)
23  com.apple.JavaScriptCore      	0x0000000109c65619 WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(WTF::AbstractLocker const&)::$_0, void>::call() + 25 (Function.h:52)
24  com.apple.JavaScriptCore      	0x0000000109c78fad WTF::Function<void ()>::operator()() const + 173 (Function.h:79)
25  com.apple.JavaScriptCore      	0x0000000109d12c93 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) + 419 (Threading.cpp:148)
26  com.apple.JavaScriptCore      	0x0000000109d1b4b5 WTF::wtfThreadEntryPoint(void*) + 21 (ThreadingPOSIX.cpp:200)
27  libsystem_pthread.dylib       	0x00007fff7432c661 _pthread_body + 340
28  libsystem_pthread.dylib       	0x00007fff7432c50d _pthread_start + 377
29  libsystem_pthread.dylib       	0x00007fff7432bbf9 thread_start + 13

[Chris Dumez]

Created attachment 375699 [details]
Patch

[Darin Adler]

Comment on attachment 375699 [details]
Patch

Do other JSValueInWrappedObject clients have the same kind of mistake?

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 375699 [details]
> Patch
> 
> Do other JSValueInWrappedObject clients have the same kind of mistake?

It does not look like it, the other users seem to have simple getters for the JSValueInWrappedObject.

[WebKit Commit Bot]

Comment on attachment 375699 [details]
Patch

Clearing flags on attachment: 375699

Committed r248376: <https://trac.webkit.org/changeset/248376>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/54039531>

[Darin Adler]

This bug is evidence that the JSValueInWrappedObject idiom isn’t foolproof enough. Might be worth thinking about how to make sure people are unlikely to make mistakes like this in the future.

[Geoffrey Garen]

Comment on attachment 375699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375699&action=review

> Source/WebCore/page/History.h:56
> +    JSValueInWrappedObject& cachedStateForGC() { return m_cachedState; }

I think we decided to call functions like this cachedStateIfExists().

[Chris Dumez]

Comment on attachment 375699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375699&action=review

>> Source/WebCore/page/History.h:56
>> +    JSValueInWrappedObject& cachedStateForGC() { return m_cachedState; }
> 
> I think we decided to call functions like this cachedStateIfExists().

I prefer cachedForGC() given what it's used for. Also note that the existing cachedState() method is NOT an "ensure" method, it actually clears m_cachedState when it is out of date.

[Darin Adler]

Comment on attachment 375699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375699&action=review

>>> Source/WebCore/page/History.h:56
>>> +    JSValueInWrappedObject& cachedStateForGC() { return m_cachedState; }
>> 
>> I think we decided to call functions like this cachedStateIfExists().
> 
> I prefer cachedForGC() given what it's used for. Also note that the existing cachedState() method is NOT an "ensure" method, it actually clears m_cachedState when it is out of date.

Given the specific behavior, another accurate name besides cachedStateForGC could be cachedStateWithoutRevalidation.

As I said, it might be more important to come up with a great idiom for correctly using JSValueInWrappedObject; I’m irritated that it’s easy to accidentally use it wrong, and there might be a simple way to make it better.

[Ryosuke Niwa]

Comment on attachment 375699 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=375699&action=review

>>>> Source/WebCore/page/History.h:56
>>>> +    JSValueInWrappedObject& cachedStateForGC() { return m_cachedState; }
>>> 
>>> I think we decided to call functions like this cachedStateIfExists().
>> 
>> I prefer cachedForGC() given what it's used for. Also note that the existing cachedState() method is NOT an "ensure" method, it actually clears m_cachedState when it is out of date.
> 
> Given the specific behavior, another accurate name besides cachedStateForGC could be cachedStateWithoutRevalidation.
> 
> As I said, it might be more important to come up with a great idiom for correctly using JSValueInWrappedObject; I’m irritated that it’s easy to accidentally use it wrong, and there might be a simple way to make it better.

In the past, I've suggested using suffix "concurrently" since what's significant here is the fact it can be called concurrently not so much that it's used by GC.
Someone may come to this code and change it again to be something not thread safe in the future again if the person didn't understand that our GC does things concurrently.