Bug 154091 – bmalloc: large aligned allocations will put 1 or 2 free object on free list without merging with free neighbors

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

[Michael Saboff]

In the aligned version of Heap::allocateLarge(), gets a free object from the free list that may or may not be aligned to the requested alignment.  That object is large enough to create an aligned object splitting off an unused objected before and/or an unused object after the aligned object we'll return to the caller.  These unused object need to be merged before putting them back into the free list.

There is a similar issue in the unaligned version of Heap::allocateLarge() in that it might get an object from the free list that is larger than needed.  It splits off an unused object and puts in back on the free list without merging.

[Michael Saboff]

rdar://problem/24134885

[Michael Saboff]

Created attachment 271035 [details]
Patch

[Geoffrey Garen]

Comment on attachment 271035 [details]
Patch

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

r=me with one change

> Source/bmalloc/bmalloc/Heap.cpp:375
> +        LargeObject merged = nextLargeObject.merge();
> +        m_largeObjects.insert(merged);

This is a weird thing to do. We know that we just split an orphan, whose neighbors are allocated, and then we try to merge it with its neighbors, which we know will fail. I don't think we should do this. Instead, I think we should assert that nextLargeObject's neighbors are allocated, and then insert().

[Mark Lam]

Comment on attachment 271035 [details]
Patch

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

> Source/bmalloc/ChangeLog:3
> +        bmalloc: large aligned allocations will put 1 or 2 free object on free list without mergine with free neighbors

typo: mergine ==> merging

> Source/bmalloc/ChangeLog:8
> +        If we split off any ununsed free object in either public Heap::allocateLarge(), we merge them with

typo: ununsed ==> unused.

[Geoffrey Garen]

You should also add code to LargeObject::merge() that, after the fact, asserts that next and previous are in the allocated state.

[Michael Saboff]

Created attachment 271041 [details]
Patch for landing with corrections and suggested changes.

[WebKit Commit Bot]

Comment on attachment 271041 [details]
Patch for landing with corrections and suggested changes.

Clearing flags on attachment: 271041

Committed r196421: <http://trac.webkit.org/changeset/196421>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> Comment on attachment 271041 [details]
> Patch for landing with corrections and suggested changes.
> 
> Clearing flags on attachment: 271041
> 
> Committed r196421: <http://trac.webkit.org/changeset/196421>

Follow-up fix for Debug build assertions:

Committed r196424: <http://trac.webkit.org/changeset/196424>

Follow-on bug to fix more Debug assertions:

Bug 154113: BASSERTs added in r196421 are causing debug test failures
​<https://bugs.webkit.org/show_bug.cgi?id=154113>