Bug 135750 – REGRESSION(r172129): ftlopt branch merge made performance tests flakey crash

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Csaba Osztrogonác]

The ftlopt branch merge (https://trac.webkit.org/changeset/172129)
made some performance tests flakey crash everywhere.

logs from the Apple ML bot:
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/9681
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/9687
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/9689
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/9691
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/9692
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/9695
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/9696
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/9697
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/9698
http://build.webkit.org/builders/Apple%20MountainLion%20Release%20%28Perf%29/builds/9699

logs from the Apple Mavericks bot:
http://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Perf%29/builds/2208
http://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Perf%29/builds/2209
http://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Perf%29/builds/2219
http://build.webkit.org/builders/Apple%20Mavericks%20Release%20%28Perf%29/builds/2221

logs from the EFL bot:
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/2823
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/2826
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/2827
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/2851
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/2852
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/2853
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/2854
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/2857
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/2858
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/2859
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release%20WK2%20%28Perf%29/builds/2860

[Csaba Osztrogonác]

Unfortunately the Apple bots don't provide crash backtrace, but EFL bots does.

After checking the history, it seems only 2 tests 
crash because of this regression:
- Dromaeo/dromaeo-object-regexp.html
- Speedometer/Full.html

Running Dromaeo/dromaeo-object-regexp.html (58 of 139)
error: Dromaeo/dromaeo-object-regexp.html
1   0x7f38ed37fee0
2   0x7f38ed714ff0
3   0x7f38ecef0e40 JSC::WatchpointSet::add(JSC::Watchpoint*)
4   0x7f38ecf85fc1 JSC::DFG::DesiredWatchpoints::reallyAdd(JSC::CodeBlock*, JSC::DFG::CommonData&)
5   0x7f38ecfeedb0 JSC::DFG::Plan::reallyAdd(JSC::DFG::CommonData*)
6   0x7f38ecfeeec3 JSC::DFG::Plan::finalizeWithoutNotifyingCallback()
7   0x7f38ecfeef54 JSC::DFG::Plan::finalizeAndNotifyCallback()
8   0x7f38ed063e9e JSC::DFG::Worklist::completeAllReadyPlansForVM(JSC::VM&, JSC::DFG::CompilationKey)
9   0x7f38ed10cd4e
10  0x7f388c041059

FAILED
Finished: 944.707192 s


Running Speedometer/Full.html (139 of 139)
error: Speedometer/Full.html
1   0x7f2360a632a0
2   0x7f2360dfbff0
3   0x7f23605c46a0 JSC::WatchpointSet::add(JSC::Watchpoint*)
4   0x7f236065c659 JSC::DFG::DesiredWatchpoints::reallyAdd(JSC::CodeBlock*, JSC::DFG::CommonData&)
5   0x7f23606c5a10 JSC::DFG::Plan::reallyAdd(JSC::DFG::CommonData*)
6   0x7f23606c5b23 JSC::DFG::Plan::finalizeWithoutNotifyingCallback()
7   0x7f23606c5bb4 JSC::DFG::Plan::finalizeAndNotifyCallback()
8   0x7f236073d40e JSC::DFG::Worklist::completeAllReadyPlansForVM(JSC::VM&, JSC::DFG::CompilationKey)
9   0x7f23607e6b7e
10  0x7f23000fdaa6

FAILED
Finished: 600.103697 s

[Geoffrey Garen]

<rdar://problem/17793974>

[Mark Lam]

This crash seems to be associated with the ones that crashed in BasicRawSentinelNode called from SentinelLinkedList::push():

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000110f33934 WTF::BasicRawSentinelNode<JSC::Watchpoint>::setPrev(WTF::BasicRawSentinelNode<JSC::Watchpoint>*) + 20 (SentinelLinkedList.h:59)
1   com.apple.JavaScriptCore      	0x0000000111683b4f WTF::SentinelLinkedList<JSC::Watchpoint, WTF::BasicRawSentinelNode<JSC::Watchpoint> >::push(JSC::Watchpoint*) + 479 (SentinelLinkedList.h:139)
2   com.apple.JavaScriptCore      	0x0000000111683589 JSC::WatchpointSet::add(JSC::Watchpoint*) + 185 (Watchpoint.cpp:68)
3   com.apple.JavaScriptCore      	0x0000000111683705 JSC::InlineWatchpointSet::add(JSC::Watchpoint*) + 37 (Watchpoint.cpp:96)
4   com.apple.JavaScriptCore      	0x00000001110706d1 JSC::DFG::GenericSetAdaptor<JSC::InlineWatchpointSet>::add(JSC::CodeBlock*, JSC::InlineWatchpointSet*, JSC::Watchpoint*) + 33 (DFGDesiredWatchpoints.h:48)
5   com.apple.JavaScriptCore      	0x000000011106cb85 JSC::DFG::GenericDesiredWatchpoints<JSC::InlineWatchpointSet, JSC::DFG::GenericSetAdaptor<JSC::InlineWatchpointSet> >::reallyAdd(JSC::CodeBlock*, JSC::DFG::CommonData&) + 277 (DFGDesiredWatchpoints.h:87)
6   com.apple.JavaScriptCore      	0x000000011106c641 JSC::DFG::DesiredWatchpoints::reallyAdd(JSC::CodeBlock*, JSC::DFG::CommonData&) + 81 (DFGDesiredWatchpoints.cpp:77)
7   com.apple.JavaScriptCore      	0x000000011111f1c7 JSC::DFG::Plan::reallyAdd(JSC::DFG::CommonData*) + 71 (DFGPlan.cpp:427)
8   com.apple.JavaScriptCore      	0x000000011111f3f9 JSC::DFG::Plan::finalizeWithoutNotifyingCallback() + 249 (DFGPlan.cpp:463)
9   com.apple.JavaScriptCore      	0x000000011111f474 JSC::DFG::Plan::finalizeAndNotifyCallback() + 84 (DFGPlan.cpp:470)
10  com.apple.JavaScriptCore      	0x00000001111ce4cb JSC::DFG::Worklist::completeAllReadyPlansForVM(JSC::VM&, JSC::DFG::CompilationKey) + 315 (DFGWorklist.cpp:188)
11  com.apple.JavaScriptCore      	0x000000011135ce3e operationOptimize + 974 (JITOperations.cpp:1102)
12  ???                           	0x00003a5d3941bbfc 0 + 64172066978812
13  com.apple.JavaScriptCore      	0x00000001114b4a2a callToJavaScript + 346
...

For example, http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK2%20(Tests)/r172513%20(6220)/js/getter-setter-gc-crash-log.txt

[Mark Lam]

I was able to get the crash to manifest fairly immediately when I run Safari as follows:

$ JSC_useFTLJIT=false JSC_reportCompileTimes=true JSC_slowPathAllocsBetweenGCs=10 JSC_bytecodeRangeToDFGCompile=42:67 ./Tools/Scripts/run-safari --release

And load OpenSource/PerformanceTests/Speedometer/Full.html but have doctored OpenSource/PerformanceTests/Speedometer/resources/tests.js to only run the last test.

However, if I use a white list for functions to DFG compile (with the list from the functions that are generated from the above repro case), the issue stops reproducing.  Similarly, if I change the GC frequency, the issue also stops reproducing.

If I run with a debug build, the issue also stops reproducing.

[Mark Lam]

The next step I took is to add memory fences and release asserts to the SentinelLinkedList, which led me to adding these fences and asserts to the HandleSet and HandleNode classes since HandleSet uses the SentinelLinkedList.  With these traps, I saw some an interesting crash here:

(lldb) bt 10
* thread #1: tid = 0x6971ba, 0x00000001072f187a JavaScriptCore`WTFCrash + 42 at Assertions.cpp:329, queue = 'com.apple.main-thread, stop reason = EXC_BAD_ACCESS (code=1, address=0xbbadbeef)
    frame #0: 0x00000001072f187a JavaScriptCore`WTFCrash + 42 at Assertions.cpp:329
    frame #1: 0x0000000106ee41f2 JavaScriptCore`JSC::HandleNode::validate(this=0x0000000116cb8020) const + 82 at HandleSet.cpp:41
    frame #2: 0x0000000106afaa57 JavaScriptCore`WTF::SentinelLinkedList<JSC::HandleNode, JSC::HandleNode>::push(this=0x00007f7fab0517d8, node=0x0000000116cb8068) + 247 at SentinelLinkedList.h:192
    frame #3: 0x0000000106ee47d1 JavaScriptCore`JSC::HandleSet::writeBarrier(this=0x00007f7fab0517c0, slot=0x0000000116cb8068, value=0x00007fff5afd5078) + 337 at HandleSet.cpp:101
    frame #4: 0x00000001072a3543 JavaScriptCore`JSC::Strong<JSC::Structure>::set(this=0x00007f7fab051ab0, externalType=0x0000000116ffff20) + 147 at Strong.h:137
    frame #5: 0x0000000107299bef JavaScriptCore`JSC::Strong<JSC::Structure>::set(this=0x00007f7fab051ab0, vm=0x00007f7fab046800, value=0x0000000116ffff20) + 111 at StrongInlines.h:52
    frame #6: 0x0000000107294b45 JavaScriptCore`JSC::VM::VM(this=0x00007f7fab046800, vmType=Default, heapType=LargeHeap) + 3749 at VM.cpp:208
    frame #7: 0x0000000107293c91 JavaScriptCore`JSC::VM::VM(this=0x00007f7fab046800, vmType=Default, heapType=LargeHeap) + 33 at VM.cpp:291
    frame #8: 0x000000010729773c JavaScriptCore`JSC::VM::create(heapType=LargeHeap) + 60 at VM.cpp:346
    frame #9: 0x000000010729777a JavaScriptCore`JSC::VM::createLeaked(heapType=LargeHeap) + 26 at VM.cpp:351
    ...

The interesting thing here is that HandleSet::writeBarrier() will push a HandleSlot (from a Strong ref) onto a SentinelLinkedList.  However, a Strong ref only has storage for a single JSValue, but the SentinelLinkedList expects at least 2 pointers for next and pref pointers.  The SentinelLinkedList is expecting a node of shape BasicRawSentinelNode.  For the HandleSet, the HandleNode satisfies that requirement.  However, HandleSet::writeBarrier() is casting a HandleSlot* into a HandleNode* and pushing it onto the SentinelLinkedList.  When the SentinelLinkedList sets the node's prev and next pointers, it will smash memory.  The fences and assertions I added basically detected this issue.

It seems strange that this issue has not caused more havoc sooner.  I will double check the code and my reasoning in case I made a mistake.  I'm also not sure if this is the only issue at play here because we haven't even gotten to WatchpointSets yet.  Regardless, I will need to resolve this immediate issue before I can continue testing for other scenarios.

[Mark Lam]

Looks like there is an error in my analysis so far.  The missing piece of info is that Strong refs are allocated from HandleNodes and hence should have the proper size to be used by HandleSets.  My checksum test on the HandleNodes did not account for its JSValue field being modified directly by clients (via a HandleSlot) without going thru the HandleNode methods.  Will update and continue with testing.

[Mark Lam]

I have evidence that at least part of the issue is that DFG::DesiredWatchpoints::m_sets is holding onto WatchpointSet pointers without using a RefPtr.  WatchpointSet implements ThreadSafeRefCounted<WatchpointSet>.  When the original owner of the WatchpointSet destructs, so goes the WatchpointSet.  The DFG::DesiredWatchpoints::m_sets would be left holding a dangling pointer, and using it results in smashing memory and/or reading from freed memory.

Now looking into what the proper fix should be.

[Mark Lam]

Turns out it wasn't that difficult to change DFG:;DesiredWatchpoints::m_set to be of type GenericDesiredWatchpoints<RefPtr<WatchpointSet>> instead of GenericDesiredWatchpoints<WatchpointSet*>.  With that change, the SentinelLinkedList crashes no longer manifest.

Currently prepping for a performance test.  Will upload the patch once the perf test is done.

[Mark Lam]

(In reply to comment #7)
> I have evidence that at least part of the issue is that DFG::DesiredWatchpoints::m_sets is holding onto WatchpointSet pointers without using a RefPtr.  WatchpointSet implements ThreadSafeRefCounted<WatchpointSet>.  When the original owner of the WatchpointSet destructs, so goes the WatchpointSet.  The DFG::DesiredWatchpoints::m_sets would be left holding a dangling pointer, and using it results in smashing memory and/or reading from freed memory.

Here's the evidence I spoke of above:
I basically added prints in the WatchpointSet constructor and destructor, and set up lldb to automatically dump the backtrace when the relevant WatchpointSet destructor is called.  I also have a lot of debug code that asserts that we're not accessing a WatchpointSet that has been destructed.  At the point of the assertion failure, I confirmed that we are indeed accessing a destructed WatchpointSet.  I did this by having its destructor set some fields to 0xdead... values, and confirmed that I am seeing these 0xdead... values at the assertion failure.

For there, I have the this pointer of the destructed WatchpointSet, and search backwards in the lldb buffer to find the backtrace when the WatchpointSet was destructed.  I also searched the stdout output for my printfs that indicate that the WatchpointSet has been destructed.

Here's the backtrace at the point of WatchpointSet destruction:

* thread #1: tid = 0x10b48c, 0x000000010fdb6e44 JavaScriptCore`JSC::WatchpointSet::~WatchpointSet(this=0x00007fd884893220) + 68 at Watchpoint.cpp:59, queue = 'com.apple.main-thread, stop reason = breakpoint 4.1
    frame #0: 0x000000010fdb6e44 JavaScriptCore`JSC::WatchpointSet::~WatchpointSet(this=0x00007fd884893220) + 68 at Watchpoint.cpp:59
    frame #1: 0x000000010fdb6df5 JavaScriptCore`JSC::WatchpointSet::~WatchpointSet(this=0x00007fd884893220) + 21 at Watchpoint.cpp:56

    frame #2: 0x000000010f5c4c59 JavaScriptCore`WTF::ThreadSafeRefCounted<JSC::WatchpointSet>::deref(this=0x00007fd884893220) + 73 at ThreadSafeRefCounted.h:112
    frame #3: 0x000000010f5c4c09 JavaScriptCore`void WTF::derefIfNotNull<JSC::WatchpointSet>(ptr=0x00007fd884893220) + 57 at PassRefPtr.h:39
    frame #4: 0x000000010f5c50c8 JavaScriptCore`WTF::RefPtr<JSC::WatchpointSet>::~RefPtr(this=0x00007fd884890398) + 24 at RefPtr.h:55
    frame #5: 0x000000010f5c50a5 JavaScriptCore`WTF::RefPtr<JSC::WatchpointSet>::~RefPtr(this=0x00007fd884890398) + 21 at RefPtr.h:55
    frame #6: 0x000000010fd799ac JavaScriptCore`WTF::KeyValuePair<int, WTF::RefPtr<JSC::WatchpointSet> >::~KeyValuePair(this=0x00007fd884890390) + 28 at HashTraits.h:169
    frame #7: 0x000000010fd79985 JavaScriptCore`WTF::KeyValuePair<int, WTF::RefPtr<JSC::WatchpointSet> >::~KeyValuePair(this=0x00007fd884890390) + 21 at HashTraits.h:169

    frame #8: 0x000000010fd79948 JavaScriptCore`WTF::HashTable<int, WTF::KeyValuePair<int, WTF::RefPtr<JSC::WatchpointSet> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<int, WTF::RefPtr<JSC::WatchpointSet> > >, WTF::IntHash<int>, WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > >::KeyValuePairTraits, WTF::UnsignedWithZeroKeyHashTraits<int> >::deallocateTable(table=0x00007fd884890340, size=8) + 88 at HashTable.h:1078
    frame #9: 0x000000010fd79f0c JavaScriptCore`WTF::HashTable<int, WTF::KeyValuePair<int, WTF::RefPtr<JSC::WatchpointSet> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<int, WTF::RefPtr<JSC::WatchpointSet> > >, WTF::IntHash<int>, WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > >::KeyValuePairTraits, WTF::UnsignedWithZeroKeyHashTraits<int> >::~HashTable(this=0x00007fd884892e10) + 60 at HashTable.h:353
    frame #10: 0x000000010fd79ec5 JavaScriptCore`WTF::HashTable<int, WTF::KeyValuePair<int, WTF::RefPtr<JSC::WatchpointSet> >, WTF::KeyValuePairKeyExtractor<WTF::KeyValuePair<int, WTF::RefPtr<JSC::WatchpointSet> > >, WTF::IntHash<int>, WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > >::KeyValuePairTraits, WTF::UnsignedWithZeroKeyHashTraits<int> >::~HashTable(this=0x00007fd884892e10) + 21 at HashTable.h:350
    frame #11: 0x000000010fd79ea5 JavaScriptCore`WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > >::~HashMap(this=0x00007fd884892e10) + 21 at HashMap.h:36
    frame #12: 0x000000010fd79e55 JavaScriptCore`WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > >::~HashMap(this=0x00007fd884892e10) + 21 at HashMap.h:36
    frame #13: 0x000000010fd7fdc8 JavaScriptCore`JSC::StructureRareData::~StructureRareData() [inlined] std::__1::default_delete<WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > > >::operator(this=0x00000001182ea698, this=0x00000001182ea698, __ptr=0x00007fd884892e10)(WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > >*) const + 30 at memory:2488
    frame #14: 0x000000010fd7fdaa JavaScriptCore`JSC::StructureRareData::~StructureRareData() [inlined] std::__1::unique_ptr<WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > >, std::__1::default_delete<WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > > > >::reset(this=0x00000001182ea698, __p=0x0000000000000000) + 80 at memory:2687
    frame #15: 0x000000010fd7fd5a JavaScriptCore`JSC::StructureRareData::~StructureRareData() [inlined] std::__1::unique_ptr<WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > >, std::__1::default_delete<WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > > > >::~unique_ptr(this=0x00000001182ea698) at memory:2655
    frame #16: 0x000000010fd7fd5a JavaScriptCore`JSC::StructureRareData::~StructureRareData() [inlined] std::__1::unique_ptr<WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > >, std::__1::default_delete<WTF::HashMap<int, WTF::RefPtr<JSC::WatchpointSet>, WTF::IntHash<int>, WTF::UnsignedWithZeroKeyHashTraits<int>, WTF::HashTraits<WTF::RefPtr<JSC::WatchpointSet> > > > >::~unique_ptr(this=0x00000001182ea698) at memory:2655

    frame #17: 0x000000010fd7fd5a JavaScriptCore`JSC::StructureRareData::~StructureRareData(this=0x00000001182ea670) + 74 at StructureRareData.h:39
    frame #18: 0x000000010fd7f825 JavaScriptCore`JSC::StructureRareData::~StructureRareData(this=0x00000001182ea670) + 21 at StructureRareData.h:39
    frame #19: 0x000000010fd7f2a5 JavaScriptCore`JSC::StructureRareData::destroy(cell=0x00000001182ea670) + 21 at StructureRareData.cpp:51

    frame #20: 0x000000010fc04a65 JavaScriptCore`void JSC::MarkedBlock::callDestructor<(this=0x00000001182e0000, cell=0x00000001182ea670)1>(JSC::JSCell*) + 165 at MarkedBlock.cpp:68
    frame #21: 0x000000010fc0495b JavaScriptCore`JSC::MarkedBlock::FreeList JSC::MarkedBlock::specializedSweep<(this=0x00000001182e0000)3, (JSC::MarkedBlock::SweepMode)0, (JSC::MarkedBlock::DestructorType)1>() + 235 at MarkedBlock.cpp:92
    frame #22: 0x000000010fc0320e JavaScriptCore`JSC::MarkedBlock::FreeList JSC::MarkedBlock::sweepHelper<(this=0x00000001182e0000, sweepMode=SweepOnly)1>(JSC::MarkedBlock::SweepMode) + 302 at MarkedBlock.cpp:146
    frame #23: 0x000000010fc02b5c JavaScriptCore`JSC::MarkedBlock::sweep(this=0x00000001182e0000, sweepMode=SweepOnly) + 188 at MarkedBlock.cpp:122
    frame #24: 0x000000010fc08461 JavaScriptCore`JSC::ZombifySweep::operator(this=0x00007fff58019ce0, block=0x00000001182e0000)(JSC::MarkedBlock*) + 49 at MarkedSpace.h:70
    frame #25: 0x000000010fc083c6 JavaScriptCore`void JSC::MarkedAllocator::forEachBlock<JSC::ZombifySweep>(this=0x00007fd883033ba0, functor=0x00007fff58019ce0) + 86 at MarkedAllocator.h:138
    frame #26: 0x000000010fc082aa JavaScriptCore`JSC::ZombifySweep::ReturnType JSC::MarkedSpace::forEachBlock<JSC::ZombifySweep>(this=0x00007fd883030328, functor=0x00007fff58019ce0) + 586 at MarkedSpace.h:269
    frame #27: 0x000000010fc06029 JavaScriptCore`JSC::ZombifySweep::ReturnType JSC::MarkedSpace::forEachBlock<JSC::ZombifySweep>(this=0x00007fd883030328) + 25 at MarkedSpace.h:280
    frame #28: 0x000000010fc050ff JavaScriptCore`JSC::MarkedSpace::zombifySweep(this=0x00007fd883030328) + 79 at MarkedSpace.cpp:133
    frame #29: 0x000000010f9f629f JavaScriptCore`JSC::Heap::zombifyDeadObjects(this=0x00007fd883030018) + 79 at Heap.cpp:1354
    frame #30: 0x000000010f9f5f89 JavaScriptCore`JSC::Heap::didFinishCollection(this=0x00007fd883030018, gcStartTime=102700.874627688) + 249 at Heap.cpp:1229
    frame #31: 0x000000010f9f54f1 JavaScriptCore`JSC::Heap::collect(this=0x00007fd883030018, collectionType=FullCollection) + 753 at Heap.cpp:1024
    frame #32: 0x000000010f9f51a4 JavaScriptCore`JSC::Heap::collectAllGarbage(this=0x00007fd883030018) + 52 at Heap.cpp:958
    frame #33: 0x000000010fc0052c JavaScriptCore`JSC::MarkedAllocator::doTestCollectionsIfNeeded(this=0x00007fd883030460) + 92 at MarkedAllocator.cpp:155
    frame #34: 0x000000010fbffe83 JavaScriptCore`JSC::MarkedAllocator::allocateSlowCase(this=0x00007fd883030460, bytes=64) + 99 at MarkedAllocator.cpp:165
    frame #35: 0x000000010f5b33bf JavaScriptCore`JSC::MarkedAllocator::allocate(this=0x00007fd883030460, bytes=64) + 79 at MarkedAllocator.h:95
    frame #36: 0x000000010f5d5149 JavaScriptCore`JSC::MarkedSpace::allocateWithNormalDestructor(this=0x00007fd883030328, bytes=64) + 41 at MarkedSpace.h:251
    frame #37: 0x000000010f5d5116 JavaScriptCore`JSC::Heap::allocateWithNormalDestructor(this=0x00007fd883030018, bytes=64) + 118 at HeapInlines.h:187
    frame #38: 0x000000010f6acbd7 JavaScriptCore`void* JSC::allocateCell<JSC::JSFunction>(heap=0x00007fd883030018, size=64) + 151 at JSCellInlines.h:123
    frame #39: 0x000000010f6ac6af JavaScriptCore`void* JSC::allocateCell<JSC::JSFunction>(heap=0x00007fd883030018) + 31 at JSCellInlines.h:137
    frame #40: 0x000000010f6a8a27 JavaScriptCore`JSC::JSFunction::create(vm=0x00007fd883030000, executable=0x000000011a5fef70, scope=0x00000001182be270) + 39 at JSFunction.h:65
    frame #41: 0x000000010f834184 JavaScriptCore`operationNewFunctionNoCheck(exec=0x00007fff5801a060, functionExecutable=0x000000011a5fef70) + 164 at DFGOperations.cpp:823
    frame #42: 0x000043a38dc3b208
    frame #43: 0x000043a38dc1e307
    frame #44: 0x000043a38dc4d1f1
    frame #45: 0x000043a38dc1ed59
    frame #46: 0x000043a38dc505b1
    frame #47: 0x000043a38dc1ed59
    ...

Note that  the WatchpointSet came from a HashMap that was in a StructureRareData.  The only HashMap of WatchpointSets in StructureRareData is this:

    typedef HashMap<PropertyOffset, RefPtr<WatchpointSet>, WTF::IntHash<PropertyOffset>, WTF::UnsignedWithZeroKeyHashTraits<PropertyOffset>> PropertyWatchpointMap;
    std::unique_ptr<PropertyWatchpointMap> m_replacementWatchpointSets;

This gives me confirmation that the destructed WatchpointSet came from the StructureRareData.  However, I haven't figured out how this WatchpointSet got added to the DFGPlan's DesiredWatchpoints::m_sets.  I'll need to dig a little further for that.

[Geoffrey Garen]

> This gives me confirmation that the destructed WatchpointSet came from the StructureRareData.  However, I haven't figured out how this WatchpointSet got added to the DFGPlan's DesiredWatchpoints::m_sets.  I'll need to dig a little further for that.

In particular, what we want to know is, when the DFG added a desired watchpoint W for Structure S, did the DFG (or should it have) also put S into a data structure that should visit S during garbage collection?

If so, why wasn't S visited during garbage collection?

If not, why not?

[Mark Lam]

(In reply to comment #9)
> (In reply to comment #7)
> This gives me confirmation that the destructed WatchpointSet came from the StructureRareData.  However, I haven't figured out how this WatchpointSet got added to the DFGPlan's DesiredWatchpoints::m_sets.  I'll need to dig a little further for that.

It was right under my nose.  I have been staring at this code so many times but just could not see it.  Anyway, a few tricks with the help of lldb gave me the following backtrace when the WatchpointSet of interest was added to the DFGPlan's  DesiredWatchpoints:

  thread #15: tid = 0x140329, 0x000000010b3bbf6c JavaScriptCore`JSC::DFG::DesiredWatchpoints::addLazily_WS(this=0x00007fc6a2835918, set=0x00007fc6a600a890) + 188 at DFGDesiredWatchpoints.cpp:66, name = 'DFG Worklist Worker Thread, stop reason = breakpoint 6.1
    frame #0: 0x000000010b3bbf6c JavaScriptCore`JSC::DFG::DesiredWatchpoints::addLazily_WS(this=0x00007fc6a2835918, set=0x00007fc6a600a890) + 188 at DFGDesiredWatchpoints.cpp:66
    frame #1: 0x000000010b3e3160 JavaScriptCore`JSC::DFG::Graph::tryGetConstantProperty1(this=0x000000011643e5d0, base=JSValue at 0x0000000116439138, structureSet=0x000000011643a068, offset=114) + 368 at DFGGraph.cpp:856
    frame #2: 0x000000010b318d1c JavaScriptCore`JSC::DFG::ByteCodeParser::handleGetByOffset(this=0x000000011643dc40, prediction=0, base=0x0000000116b99500, structureSet=0x000000011643a068, identifierNumber=18, offset=114, op=GetByOffset) + 124 at DFGByteCodeParser.cpp:1705
    frame #3: 0x000000010b32119f JavaScriptCore`JSC::DFG::ByteCodeParser::parseBlock(this=0x000000011643dc40, limit=103) + 27247 at DFGByteCodeParser.cpp:2979
    frame #4: 0x000000010b318390 JavaScriptCore`JSC::DFG::ByteCodeParser::parseCodeBlock(this=0x000000011643dc40) + 2016 at DFGByteCodeParser.cpp:3577
    frame #5: 0x000000010b3172af JavaScriptCore`JSC::DFG::ByteCodeParser::handleInlining(this=0x000000011643dc40, callTargetNode=0x0000000116b96c80, resultOperand=-7, callLinkStatus=0x000000011643b720, registerOffset=-16, argumentCountIncludingThis=2, nextOffset=1934, kind=Call) + 1583 at DFGByteCodeParser.cpp:1265
    frame #6: 0x000000010b31518c JavaScriptCore`JSC::DFG::ByteCodeParser::handleCall(this=0x000000011643dc40, result=-7, op=Call, kind=Call, instructionSize=9, callTarget=0x0000000116b96c80, argumentCountIncludingThis=2, registerOffset=-16, callLinkStatus=CallLinkStatus at 0x000000011643b720, prediction=268435456) + 812 at DFGByteCodeParser.cpp:1066
    frame #7: 0x000000010b314e45 JavaScriptCore`JSC::DFG::ByteCodeParser::handleCall(this=0x000000011643dc40, result=-7, op=Call, kind=Call, instructionSize=9, callTarget=0x0000000116b96c80, argumentCountIncludingThis=2, registerOffset=-16, callLinkStatus=CallLinkStatus at 0x000000011643b7e0) + 229 at DFGByteCodeParser.cpp:1013
    frame #8: 0x000000010b314d52 JavaScriptCore`JSC::DFG::ByteCodeParser::handleCall(this=0x000000011643dc40, result=-7, op=Call, kind=CodeForCall, instructionSize=9, callee=-7, argumentCountIncludingThis=2, registerOffset=-16) + 418 at DFGByteCodeParser.cpp:1002
    frame #9: 0x000000010b314ba7 JavaScriptCore`JSC::DFG::ByteCodeParser::handleCall(this=0x000000011643dc40, pc=0x00007fc6a582ac30, op=Call, kind=CodeForCall) + 87 at DFGByteCodeParser.cpp:986
    frame #10: 0x000000010b320499 JavaScriptCore`JSC::DFG::ByteCodeParser::parseBlock(this=0x000000011643dc40, limit=1962) + 23913 at DFGByteCodeParser.cpp:2844
    frame #11: 0x000000010b318390 JavaScriptCore`JSC::DFG::ByteCodeParser::parseCodeBlock(this=0x000000011643dc40) + 2016 at DFGByteCodeParser.cpp:3577
    frame #12: 0x000000010b324db8 JavaScriptCore`JSC::DFG::ByteCodeParser::parse(this=0x000000011643dc40) + 968 at DFGByteCodeParser.cpp:3637
    frame #13: 0x000000010b3250ee JavaScriptCore`JSC::DFG::parse(graph=0x000000011643e5d0) + 62 at DFGByteCodeParser.cpp:3662
    frame #14: 0x000000010b46e7b1 JavaScriptCore`JSC::DFG::Plan::compileInThreadImpl(this=0x00007fc6a28357f0, longLivedState=0x000000011643ed08) + 193 at DFGPlan.cpp:205
    frame #15: 0x000000010b46e3c6 JavaScriptCore`JSC::DFG::Plan::compileInThread(this=0x00007fc6a28357f0, longLivedState=0x000000011643ed08, threadData=0x00007fc6a2f48860) + 390 at DFGPlan.cpp:162
    frame #16: 0x000000010b51f639 JavaScriptCore`JSC::DFG::Worklist::runThread(this=0x00007fc6a2f30000, data=0x00007fc6a2f48860) + 745 at DFGWorklist.cpp:358
    frame #17: 0x000000010b51db64 JavaScriptCore`JSC::DFG::Worklist::threadFunction(argument=0x00007fc6a2f48860) + 36 at DFGWorklist.cpp:400
    ...

Nothing exotic here.  The WatchpointSet came right out of the structure itself.  Next step, find out how the DFGPlan keeps relevant Structures alive.

Also need to find out if all WatchpointSets come from Structures (I presume not).  If not, is there any lifecycle issue there as well?

[Mark Lam]

Here's the back trace where the WatchpointSet gets added to the DFGPlan's DesiredWatchpoints:

Command #2 'c' continued the target.
  thread #17: tid = 0x2730ce, 0x0000000109f3ab5c JavaScriptCore`JSC::DFG::DesiredWatchpoints::addLazily_WS(this=0x00007fa998638da8, set=0x00007fa99a15c760) + 188 at DFGDesiredWatchpoints.cpp:66, name = 'DFG Worklist Worker Thread, stop reason = breakpoint 2.1
    frame #0: 0x0000000109f3ab5c JavaScriptCore`JSC::DFG::DesiredWatchpoints::addLazily_WS(this=0x00007fa998638da8, set=0x00007fa99a15c760) + 188 at DFGDesiredWatchpoints.cpp:66
    frame #1: 0x0000000109f61db0 JavaScriptCore`JSC::DFG::Graph::tryGetConstantProperty1(this=0x0000000114b805d0, base=JSValue at 0x0000000114b7b0b8, structureSet=0x0000000114b7c028, offset=114) + 368 at DFGGraph.cpp:856
    frame #2: 0x0000000109e9782c JavaScriptCore`JSC::DFG::ByteCodeParser::handleGetByOffset(this=0x0000000114b7fc40, prediction=0, base=0x0000000114b96780, structureSet=0x0000000114b7c028, identifierNumber=10, offset=114, op=GetByOffset) + 124 at DFGByteCodeParser.cpp:1705
    frame #3: 0x0000000109e9fe6e JavaScriptCore`JSC::DFG::ByteCodeParser::parseBlock(this=0x0000000114b7fc40, limit=147) + 27694 at DFGByteCodeParser.cpp:2990
    frame #4: 0x0000000109e96ea0 JavaScriptCore`JSC::DFG::ByteCodeParser::parseCodeBlock(this=0x0000000114b7fc40) + 2016 at DFGByteCodeParser.cpp:3588
    frame #5: 0x0000000109e95dbf JavaScriptCore`JSC::DFG::ByteCodeParser::handleInlining(this=0x0000000114b7fc40, callTargetNode=0x0000000114b93680, resultOperand=-3, callLinkStatus=0x0000000114b7d6e0, registerOffset=-12, argumentCountIncludingThis=2, nextOffset=180, kind=Call) + 1583 at DFGByteCodeParser.cpp:1265
    frame #6: 0x0000000109e93c9c JavaScriptCore`JSC::DFG::ByteCodeParser::handleCall(this=0x0000000114b7fc40, result=-3, op=Call, kind=Call, instructionSize=9, callTarget=0x0000000114b93680, argumentCountIncludingThis=2, registerOffset=-12, callLinkStatus=CallLinkStatus at 0x0000000114b7d6e0, prediction=268435456) + 812 at DFGByteCodeParser.cpp:1066
    frame #7: 0x0000000109e93955 JavaScriptCore`JSC::DFG::ByteCodeParser::handleCall(this=0x0000000114b7fc40, result=-3, op=Call, kind=Call, instructionSize=9, callTarget=0x0000000114b93680, argumentCountIncludingThis=2, registerOffset=-12, callLinkStatus=CallLinkStatus at 0x0000000114b7d7a0) + 229 at DFGByteCodeParser.cpp:1013
    frame #8: 0x0000000109e93862 JavaScriptCore`JSC::DFG::ByteCodeParser::handleCall(this=0x0000000114b7fc40, result=-3, op=Call, kind=CodeForCall, instructionSize=9, callee=-3, argumentCountIncludingThis=2, registerOffset=-12) + 418 at DFGByteCodeParser.cpp:1002
    frame #9: 0x0000000109e936b7 JavaScriptCore`JSC::DFG::ByteCodeParser::handleCall(this=0x0000000114b7fc40, pc=0x00007fa99b84db60, op=Call, kind=CodeForCall) + 87 at DFGByteCodeParser.cpp:986
    frame #10: 0x0000000109e9f168 JavaScriptCore`JSC::DFG::ByteCodeParser::parseBlock(this=0x0000000114b7fc40, limit=208) + 24360 at DFGByteCodeParser.cpp:2855
    frame #11: 0x0000000109e96ea0 JavaScriptCore`JSC::DFG::ByteCodeParser::parseCodeBlock(this=0x0000000114b7fc40) + 2016 at DFGByteCodeParser.cpp:3588
    frame #12: 0x0000000109ea3a78 JavaScriptCore`JSC::DFG::ByteCodeParser::parse(this=0x0000000114b7fc40) + 968 at DFGByteCodeParser.cpp:3648
    frame #13: 0x0000000109ea3dae JavaScriptCore`JSC::DFG::parse(graph=0x0000000114b805d0) + 62 at DFGByteCodeParser.cpp:3673
    frame #14: 0x0000000109fed581 JavaScriptCore`JSC::DFG::Plan::compileInThreadImpl(this=0x00007fa998638c80, longLivedState=0x0000000114b80d08) + 193 at DFGPlan.cpp:207
    frame #15: 0x0000000109fed196 JavaScriptCore`JSC::DFG::Plan::compileInThread(this=0x00007fa998638c80, longLivedState=0x0000000114b80d08, threadData=0x00007fa99a14aad0) + 390 at DFGPlan.cpp:164
    frame #16: 0x000000010a09e349 JavaScriptCore`JSC::DFG::Worklist::runThread(this=0x00007fa99a14a8e0, data=0x00007fa99a14aad0) + 745 at DFGWorklist.cpp:358
    frame #17: 0x000000010a09c874 JavaScriptCore`JSC::DFG::Worklist::threadFunction(argument=0x00007fa99a14aad0) + 36 at DFGWorklist.cpp:400
    frame #18: 0x000000010a5d5d90 JavaScriptCore`WTF::threadEntryPoint(contextData=0x00007fa99a14ab30) + 144 at Threading.cpp:67
    frame #19: 0x000000010a5d6a98 JavaScriptCore`WTF::wtfThreadEntryPoint(param=0x00007fa99a149bd0) + 296 at ThreadingPthreads.cpp:170
    ...

Filip says that it looks like the issue is that WatchpointSets get added because some code uses the structure.  However, later the code that does that check gets optimized out or elided.  This results in the relevant structure not being scanned by the GC.  This results in the structure and its WatchpointSets being collected.  However, the DFGPlan's DesiredWatchpoints still has the stale WatchpointSet pointer.  Filip is now working on a fix.

[Filip Pizlo]

Created attachment 236779 [details]
possible fix

[Mark Lam]

Comment on attachment 236779 [details]
possible fix

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

r=me with comment fixes.

> Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:76
>              // We should be able to assume that the watchpoint for this has already been set.
>              // But we can't because our view of what structure a value has keeps changing. That's
>              // why we call consider().

Does this comment need to be updated now that we're not calling consider() here?

> Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:47
>          // These are pretty dumb, but needed to placate subsequent assertions. We con't actually

/con't/don't/

[Filip Pizlo]

(In reply to comment #14)
> (From update of attachment 236779 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=236779&action=review
> 
> r=me with comment fixes.
> 
> > Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:76
> >              // We should be able to assume that the watchpoint for this has already been set.
> >              // But we can't because our view of what structure a value has keeps changing. That's
> >              // why we call consider().
> 
> Does this comment need to be updated now that we're not calling consider() here?
> 
> > Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:47
> >          // These are pretty dumb, but needed to placate subsequent assertions. We con't actually
> 
> /con't/don't/

Thanks for the review.  I'm still running benchmarks.  Hopefully I can land this later tonight.

[Filip Pizlo]

(In reply to comment #15)
> (In reply to comment #14)
> > (From update of attachment 236779 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=236779&action=review
> > 
> > r=me with comment fixes.
> > 
> > > Source/JavaScriptCore/dfg/DFGAbstractValue.cpp:76
> > >              // We should be able to assume that the watchpoint for this has already been set.
> > >              // But we can't because our view of what structure a value has keeps changing. That's
> > >              // why we call consider().
> > 
> > Does this comment need to be updated now that we're not calling consider() here?

Turns out the comment was wrong and the code is probably dead.  It's a benign bug.  Filed:

https://bugs.webkit.org/show_bug.cgi?id=136055

I'll add a FIXME and link to that bug.

> > 
> > > Source/JavaScriptCore/dfg/DFGStructureRegistrationPhase.cpp:47
> > >          // These are pretty dumb, but needed to placate subsequent assertions. We con't actually
> > 
> > /con't/don't/
> 
> Thanks for the review.  I'm still running benchmarks.  Hopefully I can land this later tonight.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/172737

[Mark Lam]

Build fix for non-Mac build landed in r172739: <http://trac.webkit.org/r172739>.