Bug 120389 – JSArray::shiftCountWithArrayStorage doesn't change indexBias when shifting the last element in m_vector

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

[Mark Hahnenberg]

This leads to the JSArray forgetting it's true size (it thinks it has 8 bytes less than it actually does).

[Mark Hahnenberg]

Created attachment 209836 [details]
Patch

[Geoffrey Garen]

Comment on attachment 209836 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        This leads to the JSArray forgetting it's true size (it thinks it has 8 bytes less 

Should be "its true size".

[Mark Hahnenberg]

(In reply to comment #2)
> (From update of attachment 209836 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209836&action=review
> 
> > Source/JavaScriptCore/ChangeLog:8
> > +        This leads to the JSArray forgetting it's true size (it thinks it has 8 bytes less 
> 
> Should be "its true size".

Will fix.

[Michael Saboff]

Comment on attachment 209836 [details]
Patch

r=me - With change log fix.

[Filip Pizlo]

Comment on attachment 209836 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSArray.cpp:729
> +        return true;
>      }
> +
> +    storage->m_indexBias += count;
>      return true;

This looks like it's introducing a new bug: the (startIndex < usedVectorLength - (startIndex + count)) == true case already adds count to indexBias.  So now you're adding it twice.

[Mark Hahnenberg]

Comment on attachment 209836 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSArray.cpp:729
>>      return true;
> 
> This looks like it's introducing a new bug: the (startIndex < usedVectorLength - (startIndex + count)) == true case already adds count to indexBias.  So now you're adding it twice.

I don't think so. There's an early return inside the if block.

[Mark Hahnenberg]

(In reply to comment #6)
> (From update of attachment 209836 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209836&action=review
> 
> >> Source/JavaScriptCore/runtime/JSArray.cpp:729
> >>      return true;
> > 
> > This looks like it's introducing a new bug: the (startIndex < usedVectorLength - (startIndex + count)) == true case already adds count to indexBias.  So now you're adding it twice.
> 
> I don't think so. There's an early return inside the if block.

But it does look like the other branch of the if-statement isn't modifying indexBias either, and it appears that it should.

[Mark Hahnenberg]

Created attachment 209901 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 209901 [details]
Patch

This patch is wrong. It breaks Facebook.

[Mark Hahnenberg]

Created attachment 209997 [details]
Patch

[Mark Hahnenberg]

(In reply to comment #10)
> Created an attachment (id=209997) [details]
> Patch

I went through the code and tried to figure out exactly what it was doing. As I went I added new variables that more accurately describe some of the intermediate calculations that are going on, which make the code much clearer. I also added comments to explain why we're doing certain things in certain places.

[Mark Hahnenberg]

Comment on attachment 209997 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSArray.cpp:726
> +        // Since we're consuming part of the vector by moving its beginning to the left,

...beginning to the *right*

[Michael Saboff]

Comment on attachment 209997 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSArray.cpp:717
>              memmove(
> -                storage->m_vector + startIndex,
> -                storage->m_vector + startIndex + count,
> -                sizeof(JSValue) * (usedVectorLength - (startIndex + count)));
> -            for (unsigned i = usedVectorLength - count; i < usedVectorLength; ++i)
> -                storage->m_vector[i].clear();
> -        }
> +                storage->m_vector + count,
> +                storage->m_vector,
> +                sizeof(JSValue) * startIndex);

What about when the result of the shift will result with a vector length of 0.  Copying in that case is wasteful and may also be wrong.

> Source/JavaScriptCore/runtime/JSArray.cpp:735
> +        memmove(
> +            storage->m_vector + startIndex,
> +            storage->m_vector + firstIndexAfterShiftRegion,
> +            sizeof(JSValue) * numElementsAfterShiftRegion);

Ditto.

[Mark Hahnenberg]

(In reply to comment #13)
> (From update of attachment 209997 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209997&action=review
> 
> > Source/JavaScriptCore/runtime/JSArray.cpp:717
> >              memmove(
> > -                storage->m_vector + startIndex,
> > -                storage->m_vector + startIndex + count,
> > -                sizeof(JSValue) * (usedVectorLength - (startIndex + count)));
> > -            for (unsigned i = usedVectorLength - count; i < usedVectorLength; ++i)
> > -                storage->m_vector[i].clear();
> > -        }
> > +                storage->m_vector + count,
> > +                storage->m_vector,
> > +                sizeof(JSValue) * startIndex);
> 
> What about when the result of the shift will result with a vector length of 0.  Copying in that case is wasteful and may also be wrong.
Why create a special case for that? I ran benchmarks with this patch and it had no effect. 

> 
> > Source/JavaScriptCore/runtime/JSArray.cpp:735
> > +        memmove(
> > +            storage->m_vector + startIndex,
> > +            storage->m_vector + firstIndexAfterShiftRegion,
> > +            sizeof(JSValue) * numElementsAfterShiftRegion);
> 
> Ditto.

[Michael Saboff]

Comment on attachment 209997 [details]
Patch

r=me per our discussion.  Please add RELEASE_ASSERTs to make sure that neither memmove will write past the end (first memmove) or beginning (second memmove).

[Mark Hahnenberg]

Committed r155395: <http://trac.webkit.org/changeset/155395>

[Csaba Osztrogonác]

Comment on attachment 209997 [details]
Patch

Cleared review? from attachment 209997 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).