Bug 154113 – BASSERTs added in r196421 are causing debug test failures

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

Two consistency BASSERTs where added to LargeObject::merge in r196421.  Those asserts are firing in four tests.
  fast/workers/dedicated-worker-lifecycle.html [ Crash ]
  inspector/model/remote-object-weak-collection.html [ Crash ]
  js/basic-map.html [ Crash ]
  js/duplicate-param-gc-crash.html [ Crash ]

The crash is due to scavenging, either from the concurrent scavenger or when scavenging when called via WTF::releaseFastMallocFreeMemory().  The stack traces look like:
0   com.apple.JavaScriptCore      	0x000000011060010d bmalloc::LargeObject::merge() const + 829
1   com.apple.JavaScriptCore      	0x00000001105ff1b6 bmalloc::VMHeap::deallocateLargeObject(std::__1::unique_lock<bmalloc::StaticMutex>&, bmalloc::LargeObject&) + 54
2   com.apple.JavaScriptCore      	0x00000001105fd112 bmalloc::Heap::scavengeLargeObjects(std::__1::unique_lock<bmalloc::StaticMutex>&, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l> >) + 98
3   com.apple.JavaScriptCore      	0x00000001105fcf04 bmalloc::Heap::scavenge(std::__1::unique_lock<bmalloc::StaticMutex>&, std::__1::chrono::duration<long long, std::__1::ratio<1l, 1000l> >) + 148

[Michael Saboff]

Created attachment 271188 [details]
Patch

[Michael Saboff]

Created attachment 271189 [details]
Really the patch

[Michael Saboff]

rdar://problem/24630709

[Geoffrey Garen]

Comment on attachment 271189 [details]
Really the patch

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

I think we need a more explicit policy about moving memory from VMHeap back to heap. In that case, change of Owner can reveal merging opportunities, which might turn into fragmentation or ASSERTs later.

> Source/bmalloc/bmalloc/LargeObject.h:152
> +inline bool LargeObject::prevIsFreeAndOwnedBy(Owner owner) const
> +{
> +    EndTag* prev = m_beginTag->prev();
> +
> +    return prev->isFree() && prev->owner() == owner;
> +}
> +
> +inline bool LargeObject::nextIsFreeAndOwnedBy(Owner owner) const
> +{
> +    BeginTag* next = m_endTag->next();
> +
> +    return next->isFree() && next->owner() == owner;
> +}

Let's call these two functions and the two above them "prevCanMerge" and "nextCanMerge". That's the question we're trying to ask.

> Source/bmalloc/bmalloc/LargeObject.h:239
> +    LargeObject result(beginTag, endTag, range.begin());
> +    BASSERT(result.prevIsAllocated());
> +    BASSERT(result.nextIsAllocated());

I think we shouldn't assert here. We don't thing we ever do this, but it wouldn't be so wrong to do a bunch of splits and then a bunch of merges -- as long as you never put the fragments back into the free list before merging them back together.

> Source/bmalloc/bmalloc/VMHeap.h:110
> +    // we don't hit an assert that we are inserting a free object next to another.

Be sure to set the owner for the object we return before inserting the leftover back into the free list. The free list asserts that we never insert an object that could have merged with its neighbor.

> Source/bmalloc/bmalloc/VMHeap.h:162
>  inline void VMHeap::deallocateLargeObject(std::unique_lock<StaticMutex>& lock, LargeObject& largeObject)

Let's take this argument by value, and then not make a copy with a different name.

> Source/bmalloc/bmalloc/VMHeap.h:182
> +    } while (objectToDeallocate.prevIsFreeAndOwnedBy(Owner::VMHeap) || objectToDeallocate.nextIsFreeAndOwnedBy(Owner::VMHeap));

No need to add a version of the function that takes an explicit owner. Our owner is VMHeap.

Might be nice to add a helper that calls both for you: canMerge().

I guess we should add a comment explaining that multiple threads might scavenge concurrently, meaning that new merging opportunities become visible after we reacquire the lock.

[Geoffrey Garen]

Let's establish an invariant that the Heap never splits an object returned by the VMHeap. Our current splitting behavior for aligned allocation needlessly commits some dirty memory. Also, extra splitting might create surprising merge opportunities when moving objects between heaps, causing our fragmentation ASSERTs to fire.

I think we can take the "split and allocate" code in Heap and move it into helper functions. Then, both Heap and VMHeap can have identical "split and allocate" logic, and Heap can just immediately return whatever VMHeap returns, without worrying about further splits. It looks like the inputs to the function are always a large object and a segregated free list.

[Michael Saboff]

(In reply to comment #5)
> Let's establish an invariant that the Heap never splits an object returned
> by the VMHeap. Our current splitting behavior for aligned allocation
> needlessly commits some dirty memory. Also, extra splitting might create
> surprising merge opportunities when moving objects between heaps, causing
> our fragmentation ASSERTs to fire.
> 
> I think we can take the "split and allocate" code in Heap and move it into
> helper functions. Then, both Heap and VMHeap can have identical "split and
> allocate" logic, and Heap can just immediately return whatever VMHeap
> returns, without worrying about further splits. It looks like the inputs to
> the function are always a large object and a segregated free list.

Will handle this with a separate bug: <https://bugs.webkit.org/show_bug.cgi?id=154192> - bmalloc: Use common "split and allocate" code for large allocations in VMHeap and Heap

[Michael Saboff]

Created attachment 271213 [details]
Patch for Landing

[WebKit Commit Bot]

Attachment 271213 [details] did not pass style-queue:


ERROR: Source/bmalloc/bmalloc/VMHeap.h:170:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Saboff]

Created attachment 271217 [details]
patch for Landing - Fixed comment style complaint

[Michael Saboff]

Comment on attachment 271217 [details]
patch for Landing - Fixed comment style complaint

Forgot to make change to VMHeap::deallocateLargeObject()

[Michael Saboff]

Created attachment 271242 [details]
Last Patch for Landing

[WebKit Commit Bot]

Comment on attachment 271242 [details]
Last Patch for Landing

Clearing flags on attachment: 271242

Committed r196536: <http://trac.webkit.org/changeset/196536>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.