New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Oct 2016
Cc:
Components:
HW: All
NextAction: ----
OS: All
Priority: 3
Type: Bug

Blocking:
issue 4193



Sign in to add a comment

ArrayBuffer constructor spec violations

Project Member Reported by bmeu...@chromium.org, Dec 4 2015

Issue description

According to the ES2015 spec, invoking the following lines should throw range errors:

new ArrayBuffer()
new ArrayBuffer(NaN)
new ArrayBuffer("foo")

But this doesn't match the behavior of FF or Safari. FF basically performs ToInteger on the input instead of ToNumber, which we will also do for web compatibility. It'd be nice to update the spec to match the actual behavior of browsers in the wild.

 
Blocking: v8:4193
Project Member

Comment 2 by bugdroid1@chromium.org, Dec 4 2015

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/8aae841ce26f01f3535e54af3a58fb7c65833eb7

commit 8aae841ce26f01f3535e54af3a58fb7c65833eb7
Author: bmeurer <bmeurer@chromium.org>
Date: Fri Dec 04 06:05:26 2015

[es6] Match ArrayBuffer constructor behavior of Firefox.

We choose to deliberately violate the ES2015 specification and implement
the ArrayBuffer constructor in a way that matches Firefox and Safari
instead.

BUG= v8:4592 
LOG=n

Review URL: https://codereview.chromium.org/1497163002

Cr-Commit-Position: refs/heads/master@{#32597}

[modify] http://crrev.com/8aae841ce26f01f3535e54af3a58fb7c65833eb7/src/builtins.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Dec 4 2015

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/9730118946938262b090f88d5ed8fd461a04f0a9

commit 9730118946938262b090f88d5ed8fd461a04f0a9
Author: machenbach <machenbach@chromium.org>
Date: Fri Dec 04 10:37:12 2015

Revert of [es6] Match ArrayBuffer constructor behavior of Firefox. (patchset #1 id:1 of https://codereview.chromium.org/1497163002/ )

Reason for revert:
Blocks the roll:
https://codereview.chromium.org/1497763004/

Original issue's description:
> [es6] Match ArrayBuffer constructor behavior of Firefox.
>
> We choose to deliberately violate the ES2015 specification and implement
> the ArrayBuffer constructor in a way that matches Firefox and Safari
> instead.
>
> BUG= v8:4592 
> LOG=n
>
> Committed: https://crrev.com/8aae841ce26f01f3535e54af3a58fb7c65833eb7
> Cr-Commit-Position: refs/heads/master@{#32597}

TBR=jarin@chromium.org,bmeurer@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= v8:4592 

Review URL: https://codereview.chromium.org/1496293002

Cr-Commit-Position: refs/heads/master@{#32605}

[modify] http://crrev.com/9730118946938262b090f88d5ed8fd461a04f0a9/src/builtins.cc

Comment 4 by adamk@chromium.org, Dec 16 2015

Did the "match Firefox" change ever get relanded? Or is the idea to hope that we don't break too much stuff?
I want to reland both with a fix for ASAN, just haven't had time. 
There is now a test262 test that checks to ensure that we throw when the length parameter is absent:

built-ins/ArrayBuffer/length-is-absent

Let's keep this bug open until we conform to the spec, one way or another.

adamk, are you planning on starting this spec discussion? Or, alternately, should we evangelize to websites that they should move to the spec? I've gotten pushback on GitHub against spec changes where we didn't prove that we couldn't make the web conform to the standard first.
Brian Terlson confirms that Edge/IE also implements this behavior.
Brian filed this bug at https://github.com/tc39/ecma262/issues/265 .
Did we decide to ship the Firefox-like semantics because we encountered web compatibility issues, or because we wanted to match the preponderance of other browsers? To get a change in the spec, we might need real web incompatibility evidence. I think TC39 knew that they were spec'ing something stricter than the previous Khronos semantics, but felt that it was important anyway. I don't really see why these weaker semantics are useful for users; they just suppress certain exceptions, but maybe the web needs them suppressed.
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/cb21144baf8a11c406df17ffcabf1e4ba978c9f1

commit cb21144baf8a11c406df17ffcabf1e4ba978c9f1
Author: bmeurer <bmeurer@chromium.org>
Date: Fri Jan 01 07:12:48 2016

[es6] Unify ArrayBuffer and SharedArrayBuffer constructors.

Unify the constructors and isView methods for ArrayBuffer and
SharedArrayBuffer, moving them to C++ because there's no point
in having the JavaScript wrappers for them.

We choose to deliberately violate the ES2015 specification and
implement the ArrayBuffer constructor in a way that matches
Firefox and Safari instead.

CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG= chromium:565917 ,  v8:4592 
TBR=hpayer@chromium.org
R=cbruni@chromium.org
LOG=n

Committed: https://crrev.com/3235ccbb7826ceec2188f6ebab98fc851b54f60e
Cr-Commit-Position: refs/heads/master@{#32590}

Review URL: https://codereview.chromium.org/1500543002

Cr-Commit-Position: refs/heads/master@{#33072}

[modify] http://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1/src/bootstrapper.cc
[modify] http://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1/src/builtins.cc
[modify] http://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1/src/builtins.h
[modify] http://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1/src/heap/heap.h
[modify] http://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1/src/js/arraybuffer.js
[modify] http://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1/src/js/harmony-sharedarraybuffer.js
[modify] http://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1/src/runtime/runtime-typedarray.cc
[modify] http://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1/src/runtime/runtime.h
[add] http://crrev.com/cb21144baf8a11c406df17ffcabf1e4ba978c9f1/test/mjsunit/regress/regress-crbug-565917.js

To respond to #9 (since I raised the compat issue to begin with): I did a quick search both on internal google3 and on github and found a good number of examples of folks calling the ArrayBuffer constructor without an argument. Given that that (before bmeurer's change) had compatible behavior across browsers, it seemed reasonable to me to stick with matching other browsers (and I asked bmeurer to file this bug to track our incompatibility, as a reminder to follow up on the spec).

I don't have any evidence about other, non-undefined arguments to ArrayBuffer().
Labels: Test262Failures
Labels: -test262failures Hotlist-test262
Leo Balter has been working on TypedArray, ArrayBuffer and DataView constructor spec changes to make them more web-compatible and consistent with each other. The work in progress is at https://github.com/tc39/ecma262/pull/410/files . Do these address our web compatibility concerns?

Comment 15 by adamk@chromium.org, Mar 21 2016

The changes there look pretty good to me; I like that it brings the spec in line with existing implementations that throw for negative lengths. And though it's more restrictive about not accepting non-int floats, I can't imagine anyone passing such a length (and at least in a few searches I don't see any examples of such usage).
Cc: adamk@chromium.org
Owner: ----
Status: Available (was: Assigned)
At the March 2016 TC39 meeting, Leo Balter made a proposal to change several of the constructors. This seemed to achieve provisional consensus, though more data was requested about what browsers do today. Let's keep things the way they are in V8 until more progress is made on the spec front.
Labels: SpecViolation-OpenQuestion

Comment 19 by bakkot@google.com, Jun 23 2016

Owner: bakkot@google.com
Status: Started (was: Available)
The PR mentioned above is merged. We'll conform to spec after v8:4784 and v8:5124, I believe.
Owner: ----
Status: Available (was: Started)
Status: Fixed (was: Available)
I think bakkot's patches fix up the spec violations here.
Labels: Priority-3

Sign in to add a comment