Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue3035: Memory leak in Array.splice() #3211

Merged
merged 1 commit into from Jun 26, 2017
Merged

Issue3035: Memory leak in Array.splice() #3211

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
133 changes: 82 additions & 51 deletions lib/Runtime/Library/JavascriptArray.cpp
View file
Open in desktop
Expand Up @@ -529,6 +529,38 @@ namespace Js
VirtualTableInfo<JavascriptNativeFloatArray>::HasVirtualTable(aValue));
}

bool JavascriptArray::IsInlineSegment(SparseArraySegmentBase *seg, JavascriptArray *pArr)
Copy link
Contributor

@akroshg akroshg Jun 23, 2017 •
edited by suwc

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seg [](start = 66, length = 3)

return false when seg is null? could be a valid case. #Resolved

{
if (seg == nullptr)
{
return false;
}

SparseArraySegmentBase* inlineHeadSegment = nullptr;
if (JavascriptNativeArray::Is(pArr))
{
if (JavascriptNativeFloatArray::Is(pArr))
{
inlineHeadSegment = DetermineInlineHeadSegmentPointer<JavascriptNativeFloatArray, 0, true>((JavascriptNativeFloatArray*)pArr);
}
else
{
AssertOrFailFast(JavascriptNativeIntArray::Is(pArr));
inlineHeadSegment = DetermineInlineHeadSegmentPointer<JavascriptNativeIntArray, 0, true>((JavascriptNativeIntArray*)pArr);
}

Assert(inlineHeadSegment);
return (seg == inlineHeadSegment);
}

// This will result in false positives. It is used because DetermineInlineHeadSegmentPointer
// does not handle Arrays that change type e.g. from JavascriptNativeIntArray to JavascriptArray
// This conversion in particular is problematic because JavascriptNativeIntArray is larger than JavascriptArray
// so the returned head segment ptr never equals pArr->head. So we will default to using this and deal with
// false positives. It is better than always doing a hard copy.
return pArr->head != nullptr && HasInlineHeadSegment(pArr->head->length);
}

DynamicObjectFlags JavascriptArray::GetFlags() const
{
return GetArrayFlags();
Expand Down Expand Up @@ -5213,28 +5245,7 @@ namespace Js
template <typename T>
void JavascriptArray::CopyHeadIfInlinedHeadSegment(JavascriptArray *array, Recycler *recycler)
{
SparseArraySegmentBase* inlineHeadSegment = nullptr;
bool hasInlineSegment = false;

if (JavascriptNativeArray::Is(array))
{
if (JavascriptNativeFloatArray::Is(array))
{
inlineHeadSegment = DetermineInlineHeadSegmentPointer<JavascriptNativeFloatArray, 0, true>((JavascriptNativeFloatArray*)array);
}
else if (JavascriptNativeIntArray::Is(array))
{
inlineHeadSegment = DetermineInlineHeadSegmentPointer<JavascriptNativeIntArray, 0, true>((JavascriptNativeIntArray*)array);
}
Assert(inlineHeadSegment);
hasInlineSegment = (array->head == (SparseArraySegment<T>*)inlineHeadSegment);
}
else
{
hasInlineSegment = HasInlineHeadSegment(array->head->length);
}

if (hasInlineSegment)
if (JavascriptArray::IsInlineSegment(array->head, array))
{
AnalysisAssert(array->head);
SparseArraySegment<T>* newHeadSeg = array->ReallocNonLeafSegment((SparseArraySegment<T>*)PointerValue(array->head), array->head->next);
Expand Down Expand Up @@ -5378,8 +5389,8 @@ namespace Js
}

// During the loop below we are going to reverse the segments list. The head segment will become the last segment.
// We have to verify that the current head segment is not the inilined segement, otherwise due to shuffling below, the inlined segment will no longer
// be the head and that can create issue down the line. Create new segment if it is an inilined segment.
// We have to verify that the current head segment is not the inlined segement, otherwise due to shuffling below, the inlined segment will no longer
// be the head and that can create issue down the line. Create new segment if it is an inlined segment.
if (pArr->head && pArr->head->next)
{
if (isIntArray)
Expand Down Expand Up @@ -5581,6 +5592,9 @@ namespace Js
AssertMsg(pArr->head->size == next->left + 1, "Shift next->left overlaps current segment by more than 1 element");

SparseArraySegment<T> *head = SparseArraySegment<T>::From(pArr->head);
SparseArraySegment<T> *oldHead = head;
bool isInlineSegment = JavascriptArray::IsInlineSegment(head, pArr);
bool nextIsInlineSegment = JavascriptArray::IsInlineSegment(next, pArr);
// Merge the two adjacent segments
if (next->length != 0)
{
Expand Down Expand Up @@ -5615,8 +5629,18 @@ namespace Js
head->length = offset + next->length;
head->CheckLengthvsSize();
pArr->head = head;

if (isInlineSegment && oldHead != head)
{
pArr->ClearElements(oldHead, 0);
}
}
head->next = next->next;
if (nextIsInlineSegment)
{
pArr->ClearElements(next, 0);
}

pArr->InvalidateLastUsedSegment();
}

Expand Down Expand Up @@ -6500,7 +6524,13 @@ namespace Js
uint32 newLength = head->length + countUndefined;
if (newLength > head->size)
{
SparseArraySegmentBase *oldHead = head;
bool isInlineSegment = JavascriptArray::IsInlineSegment(head, this);
head = SparseArraySegment<Var>::From(head)->GrowByMin(recycler, newLength - head->size);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GrowByMin [](start = 60, length = 9)

Seems like this will always allocate a new segment so the check oldHead != head is not needed #Closed

if (isInlineSegment)
{
this->ClearElements(oldHead, 0);
}
}

Var undefined = scriptContext->GetLibrary()->GetUndefined();
Expand Down Expand Up @@ -6816,8 +6846,14 @@ namespace Js
// Save the deleted elements
if (headDeleteLen != 0)
{
SparseArraySegmentBase *oldHead = pnewArr->head;
bool isInlineSegment = JavascriptArray::IsInlineSegment(oldHead, pnewArr);
pnewArr->InvalidateLastUsedSegment();
pnewArr->head = SparseArraySegment<T>::CopySegment(recycler, SparseArraySegment<T>::From(pnewArr->head), 0, seg, start, headDeleteLen);
if (isInlineSegment && oldHead != pnewArr->head)
{
pnewArr->ClearElements(oldHead, 0);
}
}

if (newHeadLen != 0)
Expand Down Expand Up @@ -6908,33 +6944,7 @@ namespace Js
}
}

// handle inlined segment
SparseArraySegmentBase* inlineHeadSegment = nullptr;
bool hasInlineSegment = false;
// The following if else set is used to determine whether a shallow or hard copy is needed
if (JavascriptNativeArray::Is(pArr))
{
if (JavascriptNativeFloatArray::Is(pArr))
{
inlineHeadSegment = DetermineInlineHeadSegmentPointer<JavascriptNativeFloatArray, 0, true>((JavascriptNativeFloatArray*)pArr);
}
else if (JavascriptNativeIntArray::Is(pArr))
{
inlineHeadSegment = DetermineInlineHeadSegmentPointer<JavascriptNativeIntArray, 0, true>((JavascriptNativeIntArray*)pArr);
}
Assert(inlineHeadSegment);
hasInlineSegment = (startSeg == (SparseArraySegment<T>*)inlineHeadSegment);
}
else
{
// This will result in false positives. It is used because DetermineInlineHeadSegmentPointer
// does not handle Arrays that change type e.g. from JavascriptNativeIntArray to JavascriptArray
// This conversion in particular is problematic because JavascriptNativeIntArray is larger than JavascriptArray
// so the returned head segment ptr never equals pArr->head. So we will default to using this and deal with
// false positives. It is better than always doing a hard copy.
hasInlineSegment = HasInlineHeadSegment(pArr->head->length);
}

bool hasInlineSegment = JavascriptArray::IsInlineSegment(startSeg, pArr);
if (startSeg)
{
// Delete Phase
Expand Down Expand Up @@ -7021,6 +7031,7 @@ namespace Js

// Remove the entire segment from the original array
*prevSeg = startSeg->next;
pArr->ClearElements(startSeg, 0);
startSeg = SparseArraySegment<T>::From(startSeg->next);
}
// if we have an inline head segment with 0 elements, remove it
Expand Down Expand Up @@ -7129,6 +7140,8 @@ namespace Js
uint32 spaceLeft = segInsert->size - (start - segInsert->left);
if(spaceLeft < insertLen)
{
SparseArraySegment<T> *oldSegInsert = segInsert;
bool isInlineSegment = JavascriptArray::IsInlineSegment(segInsert, pArr);
if (!segInsert->next)
{
segInsert = segInsert->GrowByMin(recycler, insertLen - spaceLeft);
Expand All @@ -7137,6 +7150,11 @@ namespace Js
{
segInsert = segInsert->GrowByMinMax(recycler, insertLen - spaceLeft, segInsert->next->left - segInsert->left - segInsert->size);
}

if (isInlineSegment)
{
pArr->ClearElements(oldSegInsert, 0);
}
}
*prevPrevSeg = segInsert;
segInsert->length = start + insertLen - segInsert->left;
Expand Down Expand Up @@ -7265,6 +7283,8 @@ namespace Js
// of that segment we optimize splice - Fast path.
if (pArr->IsSingleSegmentArray() && pArr->head->HasIndex(start))
{
SparseArraySegmentBase *oldHead = pArr->head;
bool isInlineSegment = JavascriptArray::IsInlineSegment(oldHead, pArr);
if (isIntArray)
{
ArraySegmentSpliceHelper<int32>(newArr, SparseArraySegment<int32>::From(pArr->head), (SparseArraySegment<int32>**)&pArr->head, start, deleteLen, insertArgs, insertLen, recycler);
Expand All @@ -7278,6 +7298,11 @@ namespace Js
ArraySegmentSpliceHelper<Var>(newArr, SparseArraySegment<Var>::From(pArr->head), (SparseArraySegment<Var>**)&pArr->head, start, deleteLen, insertArgs, insertLen, recycler);
}

if (isInlineSegment && oldHead != pArr->head)
{
pArr->ClearElements(oldHead, 0);
}

// Since the start index is within the bounds of the original array's head segment, it will not acquire any new
// missing values. If the original array had missing values in the head segment, some of them may have been
// copied into the array that will be returned; otherwise, the array that is returned will also not have any
Expand Down Expand Up @@ -7569,6 +7594,8 @@ namespace Js
SparseArraySegmentBase* nextToHeadSeg = pArr->head->next;
Recycler* recycler = scriptContext->GetRecycler();

SparseArraySegmentBase *oldHead = pArr->head;
bool isInlineSegment = JavascriptArray::IsInlineSegment(oldHead, pArr);
if (nextToHeadSeg == nullptr)
{
pArr->EnsureHead<T>();
Expand All @@ -7579,6 +7606,10 @@ namespace Js
pArr->head = SparseArraySegment<T>::From(pArr->head)->GrowByMinMax(recycler, unshiftElements, ((nextToHeadSeg->left + unshiftElements) - pArr->head->left - pArr->head->size));
}

if (isInlineSegment)
{
pArr->ClearElements(oldHead, 0);
}
}

template<typename T>
Expand Down
1 change: 1 addition & 0 deletions lib/Runtime/Library/JavascriptArray.h
View file
Open in desktop
Expand Up @@ -214,6 +214,7 @@ namespace Js

static JavascriptArray* FromAnyArray(Var aValue);
static bool IsDirectAccessArray(Var aValue);
static bool IsInlineSegment(SparseArraySegmentBase *seg, JavascriptArray *pArr);

void SetLength(uint32 newLength);
BOOL SetLength(Var newLength);
Expand Down