Bug 125391 – new Int32Array(new ArrayBuffer(100), 1, 1) shouldn't throw an error that says "RangeError: Byte offset and length out of range of buffer"

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

[Filip Pizlo]

It should tell you that the problem is that byteOffset is not a multiple of 4.

[Diego Pino]

Created attachment 234913 [details]
Patch

[Build Bot]

Comment on attachment 234913 [details]
Patch

Attachment 234913 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5657106719965184

New failing tests:
fast/canvas/webgl/data-view-crash.html
fast/canvas/webgl/data-view-test.html

[Build Bot]

Created attachment 234921 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234913 [details]
Patch

Attachment 234913 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4910900712570880

New failing tests:
fast/canvas/webgl/data-view-crash.html
fast/canvas/webgl/data-view-test.html

[Build Bot]

Created attachment 234922 [details]
Archive of layout-test-results from webkit-ews-14 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-14  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234913 [details]
Patch

Attachment 234913 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4860924473114624

New failing tests:
fast/canvas/webgl/data-view-crash.html
fast/canvas/webgl/data-view-test.html

[Build Bot]

Created attachment 234927 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Diego Pino]

Created attachment 234984 [details]
Patch

[Build Bot]

Comment on attachment 234984 [details]
Patch

Attachment 234984 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6515551765528576

New failing tests:
media/W3C/video/networkState/networkState_during_loadstart.html
fast/canvas/webgl/data-view-test.html

[Build Bot]

Created attachment 234991 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234984 [details]
Patch

Attachment 234984 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4573289439559680

New failing tests:
fast/canvas/webgl/data-view-test.html

[Build Bot]

Created attachment 234992 [details]
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-03  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234984 [details]
Patch

Attachment 234984 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5325215269650432

New failing tests:
fast/canvas/webgl/data-view-test.html

[Build Bot]

Created attachment 234993 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Diego Pino]

Created attachment 234994 [details]
Patch

[Filip Pizlo]

Comment on attachment 234994 [details]
Patch

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

> Source/JavaScriptCore/runtime/ArrayBufferView.h:85
> +        return !(sizeof(T) > 1 && byteOffset % sizeof(T));

Why not just: !(byteOffset & (sizeof(T) - 1))

[Diego Pino]

Comment on attachment 234994 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ArrayBufferView.h:85
>> +        return !(sizeof(T) > 1 && byteOffset % sizeof(T));
> 
> Why not just: !(byteOffset & (sizeof(T) - 1))

Nice, I didn't know.

[Diego Pino]

Created attachment 235071 [details]
Patch

[Darin Adler]

Comment on attachment 235071 [details]
Patch

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

Looks good.

But there is no test coverage for alignment problems. The tests only test the string change for out of range problems, not the new code to check for alignment problems.

Also, when there is both an alignment problem and an out of range problem, which error takes precedence? The tests should cover that and the code should be structured to work as expected. I don’t have a preference, but it would be good to have this be well defined.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:82
> +    template <typename T>
> +    static bool verifyByteOffsetAlignment(unsigned byteOffset)

I think it’s overkill that we use a template here just so we can call sizeof(T). Doesn’t seem right. Could just be an inline function where we pass the size.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:84
> +        return !(byteOffset & (sizeof(T) - 1));

This assumes that sizeof(T) is a power of 2 without checking that it is. The old code used modulus which does not make that assumption. Can we change this back or is doing it this way an important optimization?

The old code was optimized/specialized for the size 1, but here that optimization was removed. Was that optimization not important? If it is important, it’s easy to re-create.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:89
>      template <typename T>

I think it’s overkill that we use a template here just so we can call sizeof(T). Doesn’t seem right. Could just be an inline function where we pass the size.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:91
> +    static bool verifySubRangeLength(PassRefPtr<ArrayBuffer> buffer,
> +        unsigned byteOffset, unsigned numElements)

This should all be on one line.

> Source/JavaScriptCore/runtime/ArrayBufferView.h:-92
> -        if (sizeof(T) > 1 && byteOffset % sizeof(T))
> -            return false;

Why remove the optimization for size 1? Was that optimization not important? If it is important, we could probably write it a lot better.

> Source/JavaScriptCore/runtime/GenericTypedArrayViewInlines.h:67
>          return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSDataView.cpp:52
>          throwVMError(
> -            exec, createRangeError(exec, "Byte offset and length out of range of buffer"));
> +            exec, createRangeError(exec, "Length out of range of buffer"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSDataView.cpp:53
> +        return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSDataView.cpp:57
> +        exec->vm().throwException(
> +            exec, createRangeError(exec, "Byte offset is not aligned"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSDataView.cpp:58
>          return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:88
>          exec->vm().throwException(
> -            exec, createRangeError(exec, "Byte offset and length out of range of buffer"));
> +            exec, createRangeError(exec, "Length out of range of buffer"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:89
> +        return 0;

Should be nullptr.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:93
> +        exec->vm().throwException(
> +            exec, createRangeError(exec, "Byte offset is not aligned"));

Please put all on one line instead of breaking into two lines.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:94
>          return 0;

Should be nullptr.

[Filip Pizlo]

(In reply to comment #19)
> (From update of attachment 235071 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235071&action=review
> 
> Looks good.
> 
> But there is no test coverage for alignment problems. The tests only test the string change for out of range problems, not the new code to check for alignment problems.
> 
> Also, when there is both an alignment problem and an out of range problem, which error takes precedence? The tests should cover that and the code should be structured to work as expected. I don’t have a preference, but it would be good to have this be well defined.
> 
> > Source/JavaScriptCore/runtime/ArrayBufferView.h:82
> > +    template <typename T>
> > +    static bool verifyByteOffsetAlignment(unsigned byteOffset)
> 
> I think it’s overkill that we use a template here just so we can call sizeof(T). Doesn’t seem right. Could just be an inline function where we pass the size.
> 
> > Source/JavaScriptCore/runtime/ArrayBufferView.h:84
> > +        return !(byteOffset & (sizeof(T) - 1));
> 
> This assumes that sizeof(T) is a power of 2 without checking that it is. The old code used modulus which does not make that assumption. Can we change this back or is doing it this way an important optimization?

The typed array code, and even the typed array specification, ensure that T will be a power of two. I think it's good to take advantage of this and it's not necessary to add more assertions to that effect. 

> 
> The old code was optimized/specialized for the size 1, but here that optimization was removed. Was that optimization not important? If it is important, it’s easy to re-create.
> 
> > Source/JavaScriptCore/runtime/ArrayBufferView.h:89
> >      template <typename T>
> 
> I think it’s overkill that we use a template here just so we can call sizeof(T). Doesn’t seem right. Could just be an inline function where we pass the size.
> 
> > Source/JavaScriptCore/runtime/ArrayBufferView.h:91
> > +    static bool verifySubRangeLength(PassRefPtr<ArrayBuffer> buffer,
> > +        unsigned byteOffset, unsigned numElements)
> 
> This should all be on one line.
> 
> > Source/JavaScriptCore/runtime/ArrayBufferView.h:-92
> > -        if (sizeof(T) > 1 && byteOffset % sizeof(T))
> > -            return false;
> 
> Why remove the optimization for size 1? Was that optimization not important? If it is important, we could probably write it a lot better.
> 
> > Source/JavaScriptCore/runtime/GenericTypedArrayViewInlines.h:67
> >          return 0;
> 
> Should be nullptr.
> 
> > Source/JavaScriptCore/runtime/JSDataView.cpp:52
> >          throwVMError(
> > -            exec, createRangeError(exec, "Byte offset and length out of range of buffer"));
> > +            exec, createRangeError(exec, "Length out of range of buffer"));
> 
> Please put all on one line instead of breaking into two lines.
> 
> > Source/JavaScriptCore/runtime/JSDataView.cpp:53
> > +        return 0;
> 
> Should be nullptr.
> 
> > Source/JavaScriptCore/runtime/JSDataView.cpp:57
> > +        exec->vm().throwException(
> > +            exec, createRangeError(exec, "Byte offset is not aligned"));
> 
> Please put all on one line instead of breaking into two lines.
> 
> > Source/JavaScriptCore/runtime/JSDataView.cpp:58
> >          return 0;
> 
> Should be nullptr.
> 
> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:88
> >          exec->vm().throwException(
> > -            exec, createRangeError(exec, "Byte offset and length out of range of buffer"));
> > +            exec, createRangeError(exec, "Length out of range of buffer"));
> 
> Please put all on one line instead of breaking into two lines.
> 
> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:89
> > +        return 0;
> 
> Should be nullptr.
> 
> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:93
> > +        exec->vm().throwException(
> > +            exec, createRangeError(exec, "Byte offset is not aligned"));
> 
> Please put all on one line instead of breaking into two lines.
> 
> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:94
> >          return 0;
> 
> Should be nullptr.

[Oliver Hunt]

> > This assumes that sizeof(T) is a power of 2 without checking that it is. The old code used modulus which does not make that assumption. Can we change this back or is doing it this way an important optimization?
> 
> The typed array code, and even the typed array specification, ensure that T will be a power of two. I think it's good to take advantage of this and it's not necessary to add more assertions to that effect. 

Hmmm, however there is a structured type proposal that allows for array-of-struct semantics that probably does not guarantee n^2 semantics, so maybe it's worth considering that as a future proofing thing?

[Filip Pizlo]

(In reply to comment #21)
> > > This assumes that sizeof(T) is a power of 2 without checking that it is. The old code used modulus which does not make that assumption. Can we change this back or is doing it this way an important optimization?
> > 
> > The typed array code, and even the typed array specification, ensure that T will be a power of two. I think it's good to take advantage of this and it's not necessary to add more assertions to that effect. 
> 
> Hmmm, however there is a structured type proposal that allows for array-of-struct semantics that probably does not guarantee n^2 semantics, so maybe it's worth considering that as a future proofing thing?

That's very far off in the future, so I think that this kind of future-proofing is premature.

[Filip Pizlo]

(In reply to comment #22)
> (In reply to comment #21)
> > > > This assumes that sizeof(T) is a power of 2 without checking that it is. The old code used modulus which does not make that assumption. Can we change this back or is doing it this way an important optimization?
> > > 
> > > The typed array code, and even the typed array specification, ensure that T will be a power of two. I think it's good to take advantage of this and it's not necessary to add more assertions to that effect. 
> > 
> > Hmmm, however there is a structured type proposal that allows for array-of-struct semantics that probably does not guarantee n^2 semantics, so maybe it's worth considering that as a future proofing thing?
> 
> That's very far off in the future, so I think that this kind of future-proofing is premature.

(Not to mention, that proposal is unlikely to have a requirement that arrays of struct S have a byteOffset that is a multiple of sizeof(S) - likely the alignment requirement will still be in terms of some primitive type and that types size is going to be a power of two.)

[Diego Pino]

Created attachment 235191 [details]
Patch

[Diego Pino]

* Fixed the nits from the previous patch (concatenate lines, return nullptr).
* Removed templates and pass sizeof(T) as a parameter.
* Reintroduced optimization size > 1.
* I left the assumption that size is a 2^n number.
* Added a test case checking a byteoffset exception is triggered.
* Added a test case checking a length exception takes precedence over a byteoffset exception.

[Darin Adler]

(In reply to comment #20)
> I think it's good to take advantage of this and it's not necessary to add more assertions to that effect.

But how is writing code that assumes this rather than other code that will compile to the same thing a helpful change? That’s my point. Was there a problem with the modulo code that led us to change it?

[Filip Pizlo]

(In reply to comment #26)
> (In reply to comment #20)
> > I think it's good to take advantage of this and it's not necessary to add more assertions to that effect.
> 
> But how is writing code that assumes this rather than other code that will compile to the same thing a helpful change? That’s my point. Was there a problem with the modulo code that led us to change it?

Either way, we're already assuming that sizeof(T) is a power of two.  That assumption is already present in this code.  I would find it sloppy to have some places in the typed array code be needlessly generalized for the case that sizeof(T) could be something other than a power of two, given how many places we have where power of two is a hard requirement.

Also, I find that the bitwise-and-based modulo arithmetic is a good way of documenting the invariant that sizeof(T) is a power of two.  In particular, I would *definitely* say "x % sizeof(T)" if I knew that sizeof(T) might not be a power of two.  That's not true here and no upcoming JS proposal would make it true.  Even arrays-of-structs wouldn't require alignment on struct size, but on the size of the largest primitive inside the struct - and primitives always have power-of-two sizes, since otherwise you wouldn't be able to compute their address in an array efficiently.

Using modulo won't be wrong here, but it would be a regression anyway:

- We use the "& (sizeof(T) - 1)" for such alignment checks in a lot of places - including in machine code, where using modulo wouldn't be an option.  It would be weird to be inconsistent.  I find that in tricky code like this, consistency is one way of making the code manageable.  I like getting my mind trained to read "& (x - 1)" as "alignment check".

- We use other bitwise arithmetic for manipulating offsets, addresses, and sizes.  In many places it's because we don't have a good alternative (after all, "(int)pow(2, x)" is pretty much obviously a horrible thing to write).  It's easier to reason about bitwise math if it's used consistently.  It helps remind us of the invariants we rely upon.  In this case: ~(sizeof(T) - 1) is the mask of allowable bits in a well-aligned pointer to T.

- "& (sizeof(T) - 1" doesn't necessarily have the same codegen as "% sizeof(T)" if signed numbers are involved.  So, you wouldn't be able to use % for alignment checks consistently in our codebase, since we do use intptr_t's in some places.

- The modulo operator has undef corner cases while the and operator does not.  Undef corner cases can lead to traps at either the compiler's or hardware's discretion.  For example ,"% 0" is a trap while "& (0 - 1)" is not.

[Darin Adler]

(In reply to comment #27)
> I would find it sloppy to have some places in the typed array code be needlessly generalized for the case that sizeof(T) could be something other than a power of two, given how many places we have where power of two is a hard requirement.

Wow, really long detailed response!

I’m surprised you feel that way. Switching from % to & seemed to me like a gratuitous change in this code that was unmotivated. But it sounds like you think this is much better.

[Darin Adler]

Comment on attachment 235191 [details]
Patch

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

> Source/JavaScriptCore/runtime/ArrayBufferView.h:83
> +        return !(size > 1 && byteOffset & (size - 1));

I know I was the one who mentioned a special case for a size of 1, but this is not what I had in mind. The code would work fine without the size > 1 expression, so I suggest we omit it. Sorry for leading you astray.

[Diego Pino]

Created attachment 235214 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 235214 [details]
Patch for landing.

Clearing flags on attachment: 235214

Committed r171323: <http://trac.webkit.org/changeset/171323>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.