1145058 - SpeciesConstructor is broken without @@species support (breaking use of slice/subarray on typed arrays with a custom .constructor)

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[beingalink]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:39.0) Gecko/20100101 Firefox/39.0
Build ID: 20150318055750

Steps to reproduce:

Visit https://instant.io and click the "Browse" button under "Start seeding". Then select a file.


Actual results:

Under "Log" the message "Creating .torrent file..." appears but nothing happens.


Expected results:

A hash should get generated for the file and seeding should start. I first filed a bug for the website but it appears to be a firefox bug: https://github.com/feross/instant.io/issues/22 A dev there assumes it to be related to a bug in the crypto.subtle api.

Comment 1

[Loic]

[Tracking Requested - why for this release]: recent regression.

Regression range:
good=2015-03-06
bad=2015-03-07
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0189941a3fd5&tochange=43fb1f92e8d4

Alice, any idea about the suspected bug?

Comment 2

[Loic]

Console log: 
AssertionError: false == true1 <unknown>

Comment 3

[Alice0775 White]

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=781e54a2bff4&tochange=1926f1d553bb

Comment 4

[Feross Aboukhadijeh]

Something changed with the way Uint8Array.prototype.subarray works and it broke the 'buffer' npm module which browserify uses (i.e. this Firefox change will break tons of sites, the Amazon AWS SDK, more...).

https://github.com/feross/buffer

I tried narrowing down the exact reason the tests started failing, but I'm having difficulty. If others want to take a look, here's how to run the test suite:

git clone [e-mail]:feross/buffer.git
cd buffer
npm install
npm run test-browser-local

Copy the URL into Firefox Nightly and look at the console logs.

Comment 5

[Jeff Walden [:Waldo]]

The first question is *how* it broke.  The implementation is approximately as literal a translation from spec steps as possible.  (Our previous implementation was not exactly the spec implementation, tho I'm not sure I could enumerate the exact differences.)  If there's a problem, it may well be a spec problem.

I'll see what I can do, investigating now, but I may not be able to get to this until Tuesday.  Given this landed a couple weeks ago, it shouldn't be the end of the world if it stays in another couple days.

Comment 6

[Jeff Walden [:Waldo]]

I'm a bit pressed for time, but a skim of buffer source suggests the problem.  The latest ES6 specs have this (I think unproven) conceit that the various typed array methods are subclass-ready.  What this means is that the methods that take a typed array as this and produce a new typed array, do so approximately using |this.constructor|.

    // Steps 18-20.
    var defaultConstructor = _ConstructorForTypedArray(obj);
    var constructor = SpeciesConstructor(obj, defaultConstructor);

It appears (I could be mistaken) you're calling subarray on a Buffer instance.  A Buffer instance is a typed array, so that's fine.  You'll get Uint8Array for the defaultConstructor line above.

    function SpeciesConstructor(obj, defaultConstructor) {
        var C = obj.constructor;
        if (C === undefined) {
            return defaultConstructor;
        }
        if (!IsConstructor(C)) {
            ThrowError(JSMSG_NOT_CONSTRUCTOR, DecompileArg(1, C));
        }
        return C;
    }

But then for the constructor, the above algorithm will give you back -- because Buffer values have .constructor === Buffer -- the Buffer function.  And Buffer doesn't have the same signature as a typed array constructor has:

    function Buffer (subject, encoding) {

Which suggests (although again, I'm not 100% certain, just going by inspection, quickly) we're invoking that, creating something that in no way matches what the caller wants (presumably a typed array and all), and things go south further along.

If the above analysis is correct -- and it might well not be, this is pretty quick -- the current subclassing scheme at least as regards typed arrays is not compatible with current shipping code.  It's not clear to me whether current shipping code could be changed or not for this, if we wanted to stay the course on this.

Anyway.  I'm out now, others can pick this up if they're around, or it can wait til Monday/Tuesday.

Comment 7

[Tom S [:evilpie]]

I think this would probably work again when SpeciesConstructor actually tried to look up @@species. I assume that would fail and we would just execute the defaultConstructor.

Comment 8

[Allen Wirfs-Brock]

(In reply to Tom Schuster [:evilpie] from comment #7)
> I think this would probably work again when SpeciesConstructor actually
> tried to look up @@species. I assume that would fail and we would just
> execute the defaultConstructor.

That's right.  If you are miss an essential step if you are not looking up @@species as shown in http://people.mozilla.org/%7Ejorendorff/es6-draft.html#sec-speciesconstructor.   If you aren't ready to support @@species a more appropriate interim definition for SpeciesConstructor would be for it to always use defaultConstrutor.

Based upon a quick scan of Buffer.js it looks to me that fixing SpeciesConstructor should be enough to get the existing Buffer code to work (at least for subarray, I don't really look at other methods).

I think a version of Buffer that was an ES6 subclass of Uint32Array could be much simpler than the existing code.  Such a version would over-ride @@species in exactly the way it was intended. That allows the subclass constructor to have a different signature from it superclass.

Comment 9

[Feross Aboukhadijeh]

Jeff,

> It appears (I could be mistaken) you're calling subarray on a Buffer instance

Buffer is not a proper subclass of Uint8Array. The Buffer constructor actually just returns a Uint8Array that we've "augmented" with additional function properties so it looks/acts like a Buffer. It's just a Uint8Array.

Comment 10

[Feross Aboukhadijeh]

Also, just want to emphasize that lots of sites will break if we don't figure out a solution to this (either by making Firefox behave like before, or fixing 'buffer' if that's even possible).

I would love to move Buffer to being a proper ES6 subclass of Uint8Array, but if this involves using new class syntax that breaks in older browsers, then we can't do it for a long time...

There are many users of browserify (and thus 'buffer') including Yahoo, Keybase, and more (https://github.com/substack/node-browserify/wiki/browserify-in-the-wild).

Comment 11

[Allen Wirfs-Brock]

(In reply to Feross Aboukhadijeh from comment #10)
> Also, just want to emphasize that lots of sites will break if we don't
> figure out a solution to this (either by making Firefox behave like before,
> or fixing 'buffer' if that's even possible).
> 

I'm pretty sure that a complete implementation of SpeciesConstructor within SpiderMonkey will fix the problem.

In the meantime, here is a possible work around.  The ES6 specification for Uint8Array.prototype.subarray (actually it applies to all kinds of typed arrays) calls the three argument form of the  typed array construcgtors (http://people.mozilla.org/~jorendorff/es6-draft.html#sec-%typedarray%-buffer-byteoffset-length ) on the "species constructor'.  Because of the current SpiderMonkey SpeciesConstructor implementation it is new'ing Buffer instead passing the three arguments that Uin8Array would accept for that overload.

The work around is within the Buffer constructor, check if it was called with exactly three arguments. The first argument will be an ArrayBuffer instance, the other two arguments will be integers. If the argument signature matches that configuration you are probably being called from within subarray, so do
   return new Uint8Array(arguments[0], arguments[1], arguments[2])

additional compatibility note:  The old Kronos Typed Array spec. did not provide algorithms for most methods, so the inner workings of the typed arrays methods could vary from implementation to implementation.  So, legacy implementations  subarray may or may not have accessed the this value's constructor property and may or may not have called its three argument form. 

> 
> There are many users of browserify (and thus 'buffer') including Yahoo,
> Keybase, and more
> (https://github.com/substack/node-browserify/wiki/browserify-in-the-wild).

Comment 12

[Allen Wirfs-Brock]

(In reply to Feross Aboukhadijeh from comment #10)
...
> 
> I would love to move Buffer to being a proper ES6 subclass of Uint8Array,
> but if this involves using new class syntax that breaks in older browsers,
> then we can't do it for a long time...

I'm pretty sure you can "hand wire" a version of Buffer that followed the ES6 subclass conventions without using actual class syntax.

Comment 13

[Jeff Walden [:Waldo]]

Attachment: Fix SpeciesConstructor to the spec algorithm, with a comment highlighting the spec not-following we must do, lacking @@species

Blah, I made the assumption a method without spec step comments was the spec algorithm.  :-(  Sorry about that.  Updated to spec algorithm, commented the steps we can't implement without @@species as being wrong, and changed to use defaultConstructor in its absence.

Feel free to land this if you review it earlier than I can get to it.  My tree is a couple weeks old, still waiting on jorendorff to rebase it, so I will have a little pain if I have to land this myself.

Comment 14

[Eric Faust [:efaust]]

Comment on attachment 8583568 [details] [diff] [review]
Fix SpeciesConstructor to the spec algorithm, with a comment highlighting the spec not-following we must do, lacking @@species

Review of attachment 8583568 [details] [diff] [review]:
-----------------------------------------------------------------

Stealing, as per Waldo's request on IRC. APPROVED. r=me

::: js/src/builtin/Utilities.js
@@ +171,5 @@
> +        ThrowError(JSMSG_NOT_NONNULL_OBJECT, "object's 'constructor' property");
> +
> +    // Steps 6-7.  We don't yet implement @@species and Symbol.species, so we
> +    // don't implement this correctly right now.  Somebody fix this!
> +    var s = /* ctor[Symbol.species] */ undefined;

OK, this lets us type up as much of this as we can. Cool.

::: js/src/tests/ecma_6/TypedArray/slice.js
@@ +68,5 @@
> +    mathConstructor.constructor = Math.sin;
> +    assertDeepEq(mathConstructor.slice(4), new constructor(4));
> +
> +    assertEq("species" in Symbol, false,
> +             "you've implemented @@species -- add real tests here!");

ah, here's the test I was looking for. Good.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: For checkin

Try push:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=571d556e35c5

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

 Looks like this is about to land soon, but tracking anyway for 39 since this is a recent regression.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0e5bd4702b3

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e0e5bd4702b3