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.

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

Ensure ES2016 engines construct Uint8Array (not Buffer) from Buffer.p… #4702

Closed
wants to merge 1 commit into from

Conversation

littledan
Copy link

…rototype.slice

In the ES2016 draft specification, TypedArray methods like
%TypedArray%.prototype.subarray() call out to a constructor for the result
based on the receiver. Ordinarily, the constructor is instance.constructor,
but subclasses can override this using the Symbol.species property on the
constructor.

Buffer.prototype.slice calls out to %TypedArray%.prototype.subarray, which
calls this calculated constructor with three arguments. The argument pattern
doesn't correspond to a constructor for Buffer, so without setting
Symbol.species appropriately, the wrong kind of result is created.

This patch sets Buffer[Symbol.species] to Uint8Array when appropriate, to
address the issue.

This addresses issue #4701

…rototype.slice

In the ES2016 draft specification, TypedArray methods like
%TypedArray%.prototype.subarray() call out to a constructor for the result
based on the receiver. Ordinarily, the constructor is instance.constructor,
but subclasses can override this using the Symbol.species property on the
constructor.

Buffer.prototype.slice calls out to %TypedArray%.prototype.subarray, which
calls this calculated constructor with three arguments. The argument pattern
doesn't correspond to a constructor for Buffer, so without setting
Symbol.species appropriately, the wrong kind of result is created.

This patch sets Buffer[Symbol.species] to Uint8Array when appropriate, to
address the issue.
@cjihrig
Copy link
Contributor

cjihrig commented Jan 14, 2016

cc: @trevnorris

@littledan
Copy link
Author

Note, this patch has only been tested in the browser context (actually testing feross/buffer#97), and running the basic node tests to ensure there isn't a regression. It could use some more testing when the next version of V8 is pulled in, and it can also definitely wait to be merged until then.

@@ -70,6 +70,11 @@ function Buffer(arg, encoding) {
Object.setPrototypeOf(Buffer.prototype, Uint8Array.prototype);
Object.setPrototypeOf(Buffer, Uint8Array);

if (Symbol.species && Buffer[Symbol.species] === Buffer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In Node we probably want to wait for V8 4.9 to land and then remove the if, replacing it with a comment.

Copy link
Author

Choose a reason for hiding this comment

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

If species ends up being staged, but not shipped, for a Node release, do you think it might make sense to keep the conditional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, that's fair.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expound on why "being staged, but not shipped" requires node to do language feature detection?

Copy link
Contributor

Choose a reason for hiding this comment

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

That way it will work whether or not people do --harmony

Copy link
Contributor

Choose a reason for hiding this comment

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

ah. gotcha.

@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Jan 14, 2016
@jasnell
Copy link
Member

jasnell commented Jan 14, 2016

semver-minor?

@littledan
Copy link
Author

semver-minor?

Yes, I'd say so.

@trevnorris
Copy link
Contributor

Just to confirm, this technically wouldn't be necessary until v4.8 lands, correct?

@domenic
Copy link
Contributor

domenic commented Jan 15, 2016

4.9 I believe.

I think this is semver-patch. It adds no new features.

@littledan
Copy link
Author

This isn't necessary until V8 lands @@species, which will be some time after 4.9.

Fishrock123 referenced this pull request in nodejs/node-chakracore Jan 19, 2016
* Add runtime property "process.jsEngine" to indicate current JS engine
   node runs on (JS engine is determined at build time).
* Update REPL to work with Chakra engine.
* Buffer/SlowBuffer currently does not support @@species constructor.
   Mark them as such to avoid failure as per ES6 TypedArray @@species spec.
* Minor fix in `string_bytes.cc` because of way Chakra handles
   `String::NewExternal`.
@feross
Copy link
Contributor

feross commented Feb 18, 2016

Just want to let folks know that we shipped @littledan's change (feross/buffer#97) in feross/buffer v4.5.0. Works great. I'll share here if there are any user reports of gotchas in real-world usage.

@jasnell
Copy link
Member

jasnell commented Mar 22, 2016

@trevnorris @littledan ... what's the status on this?

@littledan
Copy link
Author

I don't see any reason why it can't merge. However, it might not be needed if given the new Buffer constructor signature.

@@ -70,6 +70,11 @@ function Buffer(arg, encoding) {
Object.setPrototypeOf(Buffer.prototype, Uint8Array.prototype);
Object.setPrototypeOf(Buffer, Uint8Array);

if (Symbol.species && Buffer[Symbol.species] === Buffer) {
Object.defineProperty(Buffer, Symbol.species,
{ value: null, configurable: true });
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: bump over one space.

@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

@littledan ... can you rebase on master and address the style nit?

@littledan
Copy link
Author

Do we still want this if the three-argument constructor pattern is allowed?

@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

I'll leave that up to @trevnorris and @bnoordhuis if they think it is worthwhile

@bnoordhuis
Copy link
Member

No strong opinion, except that I'd be a bit worried about new Buffer(...) performance regressions.

I understand this doesn't need to land until we upgrade to a V8 version that ships @@species? (V8 5.1?)

@trevnorris
Copy link
Contributor

Is this supposed to fix where subarray() returns a Uint8Array and not a Buffer? I'd like to see a test added for this.

@littledan
Copy link
Author

@bnoordhuis Actually even in 51, I put in a workaround for subarray to make it always construct the base class because the web package which uses the same logic was just too prevalent. But it would be really nice to not have this forever, as it violates the ECMAScript 2017 draft spec. The three-argument Buffer constructor should address this. What sort of cause of performance regression are you worried about?

@estliberitas estliberitas force-pushed the master branch 2 times, most recently from 7da4fd4 to c7066fb Compare April 26, 2016 05:22
@benjamingr
Copy link
Member

We're now approaching Node V6 on V8 5 but from what I can see it's still not there.

@littledan
Copy link
Author

This issue should not affect V8 5.0, only V8 5.1 and later. I don't see the downside to the three-argument approach or the reason why it should have lower performance. If someone explains that, then I will rebase and update this patch.

@trevnorris
Copy link
Contributor

@littledan Buffer has already been updated to accept three arguments, to be compatible with the Uint8Array constructor.

@littledan
Copy link
Author

OK, sounds like this patch is no longer needed.

@littledan littledan closed this Apr 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buffer Issues and PRs related to the buffer subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants