REGRESSION: ASSERTION FAILED: !m_lastActiveBlock on js/function-apply.html

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

js/function-apply.html has started to sometimes assert around March 9. It's not frequent enough to pinpoint the exact revision, but it seems to be frequent enough to confuse EWS (see bug 155388).

Perhaps bug 155178?

ASSERTION FAILED: !m_lastActiveBlock
/Volumes/Data/slave/yosemite-debug/build/Source/JavaScriptCore/heap/MarkedAllocator.h(131) : void JSC::MarkedAllocator::stopAllocating()
1   0x108805030 WTFCrash
2   0x108446e1a JSC::MarkedAllocator::stopAllocating()
3   0x108446dc9 JSC::StopAllocatingFunctor::operator()(JSC::MarkedAllocator&)
4   0x108446d86 void JSC::MarkedSpace::forEachAllocator<JSC::StopAllocatingFunctor>(JSC::StopAllocatingFunctor&)
5   0x108445bd9 void JSC::MarkedSpace::forEachAllocator<JSC::StopAllocatingFunctor>()
6   0x108444c8c JSC::MarkedSpace::stopAllocating()
7   0x10844558c JSC::MarkedSpace::willStartIterating()
8   0x108107a6c JSC::Heap::willStartIterating()
9   0x107bef0d3 JSC::HeapIterationScope::HeapIterationScope(JSC::Heap&)
10  0x107bdf40d JSC::HeapIterationScope::HeapIterationScope(JSC::Heap&)
11  0x108109085 JSC::Heap::removeDeadHeapSnapshotNodes(JSC::HeapProfiler&)
12  0x10810a279 JSC::Heap::collectImpl(JSC::HeapOperation, void*, void*, int (&) [37])
13  0x108109c4d JSC::Heap::collect(JSC::HeapOperation)
14  0x107fe0805 JSC::EdenGCActivityCallback::doCollection()
15  0x1080f8620 JSC::GCActivityCallback::doWork()
16  0x1081248cf JSC::HeapTimer::timerDidFire(__CFRunLoopTimer*, void*)
17  0x7fff8ade12e4 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__

[Radar WebKit Bug Importer]

<rdar://problem/25134537>

[Geoffrey Garen]

Joe, is this you?

Are you sure it's OK for removeDeadHeapSnapshotNodes to iterate the heap during GC? Maybe you can move this to after GC instead?

[Ryan Haddad]

Marked this test as flaky to unblock EWS <http://trac.webkit.org/projects/webkit/changeset/198165>

[Joseph Pecoraro]

(In reply to comment #2)
> Joe, is this you?

Seems likely!

> Are you sure it's OK for removeDeadHeapSnapshotNodes to iterate the heap
> during GC? Maybe you can move this to after GC instead?

Yes, I think the problem is that both Heap::gatherExtraHeapSnapshotData and Heap::removeDeadHeapSnapshotNodes take a HeapIterationScope (which does a stopAllocating/resumeAllocating) but Heap::collectImpl has already done a stopAllocating and will do a resetAllocators. Duplicate stopAllocating calls looks like it would cause this ASSERT. 

I will see if it makes sense to just drop the HeapIterationScopes here given the Heap is already stopped. It looks like it would be perfectly fine to move these snapshot operations after resetAllocators where they won't mess with the state of the allocator between the normal stop/reset of collection.

Given this test is encountering the removeDeadHeapSnapshotNodes path at all, it means imply that this VM had previously run a test that created a HeapProfiler. So that would mean the bot ran an LayoutTests/inspector/heap test and then ran this LayoutTests/js/function-apply.html, so I'll try to reproduce locally with this in mind. While the HeapProfiler will still exist for the VM, the HeapSnapshots should have been cleared when the page changed so there should essentially be no work.

[Joseph Pecoraro]

(In reply to comment #4)
> I will see if it makes sense to just drop the HeapIterationScopes here given
> the Heap is already stopped. It looks like it would be perfectly fine to
> move these snapshot operations after resetAllocators where they won't mess
> with the state of the allocator between the normal stop/reset of collection.

Bah, MarkedSpace::forEachLiveCell/DeadCell expects a HeapIterationScope& in its API to ensure callers have stopped allocation. So just moving the HeapProfiler work to the end of GC (after allocators are resumed) seems easiest/safest.

[Joseph Pecoraro]

Created attachment 274053 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

(In reply to comment #4)
> So that would mean the bot ran an LayoutTests/inspector/heap
> test and then ran this LayoutTests/js/function-apply.html, so I'll try to
> reproduce locally with this in mind.

I was unable to reproduce this locally running lots of iterations of these kind of combinations.

[Alexey Proskuryakov]

Comment on attachment 274053 [details]
[PATCH] Proposed Fix

Please also unmark the test in TestExpectations when landing this.

FWIW, js/function-call-aliased.html is another affected test.

[Ryan Haddad]

Can we update the patch and land this soon? js/function-call-aliased.html is wreaking havoc on Mac EWS results.

[Mark Lam]

Comment on attachment 274053 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/heap/Heap.cpp:1201
> +    if (HeapProfiler* heapProfiler = m_vm->heapProfiler()) {
> +        gatherExtraHeapSnapshotData(*heapProfiler);
> +        removeDeadHeapSnapshotNodes(*heapProfiler);
> +    }
> +

Let's put this in didFinishCollection() (at the bottom) instead.  We do many heap iterations in there and collect statistics.  That would be the proper place for this.

[Joseph Pecoraro]

Created attachment 274121 [details]
[PATCH] Proposed Fix

[Mark Lam]

Comment on attachment 274121 [details]
[PATCH] Proposed Fix

r=me

[WebKit Commit Bot]

Comment on attachment 274121 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 274121

Committed r198229: <http://trac.webkit.org/changeset/198229>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.