Implement %TypedArray%.prototype.{toString, toLocaleString}

RESOLVED FIXED in Firefox 51

Status

()

Core
JavaScript: Standard Library
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: ziyunfei, Assigned: anba)

Tracking

({dev-doc-complete})

Trunk
mozilla51
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(1 attachment, 9 obsolete attachments)

Comment hidden (empty)
Assignee: nobody → winter2718
Created attachment 8700746 [details] [diff] [review]
typedarraytostring.diff

I based the implementation on what I saw in jsarray.cpp (where toString and toLocaleString reuse join logic). One large shortcoming with this patch is that I haven't made ToLocaleString in vm/SelfHosting.cpp inlinable. If this patch looks alright I'll go ahead and submit a follow up with that.
Attachment #8700746 - Flags: review?(jwalden+bmo)
Attachment #8700746 - Flags: review?(jwalden+bmo)
Created attachment 8701549 [details] [diff] [review]
typedarraytostring.diff

Added some tests and more closely followed the spec regarding toString:

> 22.2.3.29%TypedArray%.prototype.toString ( )
>
> The initial value of the %TypedArray%.prototype.toString data property is the > same built-in function object as the Array.prototype.toString method defined in
Attachment #8701549 - Flags: review?(jwalden+bmo)
Attachment #8700746 - Attachment is obsolete: true
Comment on attachment 8701549 [details] [diff] [review]
typedarraytostring.diff

Arg, bugspam, I need to add a test for accessing TypedArray.prototype.toLocaleString across globals.
Attachment #8701549 - Flags: review?(jwalden+bmo)
Created attachment 8701644 [details] [diff] [review]
typedarraytostring.diff
Attachment #8701549 - Attachment is obsolete: true
Attachment #8701644 - Flags: review?(jwalden+bmo)
Comment on attachment 8701644 [details] [diff] [review]
typedarraytostring.diff

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

I probably should take a second look here, once the various nits are dealt with, as they've piled up a little and I can't quite keep it all in field of view easily enough for a full review now.

::: js/src/builtin/TypedArray.js
@@ +411,5 @@
> +    // Step 9.
> +    var element0 = array[0];
> +
> +    // Steps 10-11.
> +    // arraymit the 'if' clause in step 10, since typed arrays can not have undefined or null elements.

...I'm not sure what "arraymit" here is supposed to mean?

Oh!  Lulz.  This was a search-and-replace of |O| for |array| at some earlier time, right?  Fix that.  :-)

@@ +423,5 @@
> +        // Step 13.b.
> +        var element = array[k];
> +
> +        // Steps 13.c-13.d.
> +        // arraymit the 'if' clause in step 13.c, since typed arrays can not have undefined or null elements.

This looks like it exceeds 79ch (our line limit for comments, different from code, for readability).

@@ +1176,5 @@
>  }
> +
> +// ES6 20151221 22.2.3.28 %TypedArray%.prototype.toLocaleString
> +function TypedArrayToLocaleString() {
> +    // This function is not generic.

You can get rid of this comment -- it's obvious from context (and expected, generic functions being fairly exceptional).  And elsewhere in these additions, if there were more places.

@@ +1182,5 @@
> +        return callFunction(CallTypedArrayMethodIfWrapped, this, "TypedArrayToLocaleString");
> +    }
> +
> +    var buffer = TypedArrayBuffer(this);
> +    if (IsDetachedBuffer(buffer))

IfReified

::: js/src/tests/ecma_6/TypedArray/toLocaleString.js
@@ +6,5 @@
> +        return function() { return typeof this; };
> +    }
> +})
> +
> +assertEq(new Int32Array([1]).toLocaleString(), "number");

Probably should test something more than one element long, to exercise the separator handling, too.

Also, add at least one test to js/src/tests/Intl/ that tests that passing in a different locale, and non-standard options, produces different output (once the ECMA-402 changes have been added to this patch).

::: js/src/tests/ecma_6/TypedArray/toString.js
@@ +1,2 @@
> +// Ensure that TypedArray.toString correctly captures TypedArray's implicit
> +// conversions.

The conversions are performed by the ctor, not toString, so I'd remove this comment.  The test is fine, but this rationale for it is a bit odd.

@@ +6,5 @@
> +assertEq(new Uint16Array([-1, 2, 3, 4, NaN]).toString(), "65535,2,3,4,0");
> +assertEq(new Int8Array([-1, 2, -3, 4, NaN]).toString(), "-1,2,-3,4,0");
> +assertEq(new Uint8Array([255, 2, 3, 4, NaN]).toString(), "255,2,3,4,0");
> +assertEq(new Float32Array([-0, 0, NaN]).toString(), "0,0,NaN");
> +assertEq(new Float64Array([-0, 0, NaN]).toString(), "0,0,NaN");

Add in some tests of -Infinity/+Infinity and -0 here, too, since those cases can have weird handling.  (I think -0 is supposed to be "0" for these methods' purposes, which is arguably weird.)

::: js/src/vm/SelfHosting.cpp
@@ +109,5 @@
>      return true;
>  }
>  
>  static bool
> +intrinsic_ToLocaleString(JSContext* cx, unsigned argc, Value* vp)

This method is all very self-hostable, and faster to boot if so.

function ElementToLocaleString(v)
{
    if (v === null || v === undefined)
        return "";

    return v.toLocaleString();
}

That said, this is clearly/obviously failing to pass along a locale to the toLocaleString method being invoked.  Per

""""
NOTE
If the ECMAScript implementation includes the ECMA-402 Internationalization API this function is based upon the algorithm for Array.prototype.toLocaleString that is in the ECMA-402 specification.
"""

we should be doing something ECMA-402-specified for this.  I'm moderately sure that spec hasn't specified anything for this yet, but maybe there's a draft spec somewhere that says what to do.  I would prefer if we got the locale/options optional argument handling correct from the start here, so that users can't accidentally come to depend on stunted semantics of ignoring all arguments.  (Extra-unlikely, but why take the risk?)
Attachment #8701644 - Flags: review?(jwalden+bmo)
Oh, not v.toLocaleString() now, but callContentFunction(v.toLocaleString, v), I think.
Comment on attachment 8701644 [details] [diff] [review]
typedarraytostring.diff

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

::: js/src/vm/TypedArrayObject.cpp
@@ +870,5 @@
>      // Both of these are actually defined to the same object in FinishTypedArrayInit.
>      JS_SELF_HOSTED_FN("values", "TypedArrayValues", 0, JSPROP_DEFINE_LATE),
>      JS_SELF_HOSTED_SYM_FN(iterator, "TypedArrayValues", 0, JSPROP_DEFINE_LATE),
>      JS_SELF_HOSTED_FN("includes", "TypedArrayIncludes", 2, 0),
> +    JS_SELF_HOSTED_FN("toString", "ArrayToString", 0, 0),

Actually -- I don't think this ensures the functions are visibly identical.  This just ensures the function is backed by the same self-hosted code.  To ensure function pointer identity is exact, you need to do some sort of shenanigans in terms of making Array init depend on TypedArray init (or vice versa), then (probably) a manual definition of the one shared function onto both cases, or something.

I would say leave toString out of the initial patch (which is basically more or less okay), then deal with toString in a separate patch.  Maybe even a separate bug.  And of course make sure you have an |assertEq(new Uint8Array().toString, [].toString);| test which you lack right now.
Assignee: winter2718 → dannas
Created attachment 8724579 [details] [diff] [review]
1121938-v2.diff

Attaching an updated version of the mrrrgn's patch; just made sure it applies cleanly against trunk and added a few minor tweaks to comments from Waldo's suggestions.

I've tried to find a reference to a spec for toLocaleString() that passes a locale as argument; I've looked in the github repo for tc-262 and tc-402. Has anyone else seen such spec or proposal?
(Assignee)

Comment 9

a year ago
Created attachment 8770646 [details] [diff] [review]
bug1121938.patch

When I filed bug 1285833 I've noticed .toString isn't yet implemented, so let's give it a try. Tested with (--with-intl-api, --without-intl-api) x (jstests, jit-tests).

I've also added a simple drive-by change for %TypedArray%.prototype.set to update the length property from 2 to 1 (changed in ES6 draft rev31).
Assignee: dannas → andrebargull
Attachment #8701644 - Attachment is obsolete: true
Attachment #8724579 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8770646 - Flags: review?(jwalden+bmo)
Comment on attachment 8770646 [details] [diff] [review]
bug1121938.patch

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

Mostly r+, with comments, except that the toString test and the toString change need to be removed.  Fixing that is a more difficult change, and we should either get it right or not do it -- and not let that hold up the toLocaleString parts of the patch that are perfectly fine.

::: js/src/builtin/TypedArray.js
@@ +1109,5 @@
> +// ES2017 Intl draft rev 78bbe7d1095f5ff3760ac4017ed366026e4cb276
> +//   13.4.1 Array.prototype.toLocaleString ([ locales [ , options ]])
> +function TypedArrayToLocaleString(locales = undefined, options = undefined) {
> +    // Step 1.
> +    var array = this;

It's probably worth having

  // ValidateTypedArray, then step 1.

at top to clarify what happens first.  (Step 1 in the version I'm looking at is "Let array be ToObject(this value)." which isn't what's here.)  Then we can intermingle the two algorithms in the optimally efficient way, without anyone reading the code thinking the algorithm was (perhaps accidentally) not respected.

@@ +1124,5 @@
> +    if (isTypedArray) {
> +        len = TypedArrayLength(array);
> +    } else {
> +        len = callFunction(CallTypedArrayMethodIfWrapped, array, array, "TypedArrayLength");
> +    }

No bracing on either arm of this.

@@ +1128,5 @@
> +    }
> +
> +    // Step 3.
> +    // We don't support (host) locale dependent separator strings.
> +    var separator = ",";

I'd phrase it "We don't (yet?) implement locale-dependent separators."

Given this step's unused til step 9, I would move it down to before the loop in steps 7-8 that actually uses it.  (Unfortunately, because theoretically the step could do observable stuff, it can't be moved in the spec.  :-( )

@@ +1138,5 @@
> +    // Step 5.
> +    var firstElement = array[0];
> +
> +    // Steps 6-7.
> +    // Omit the 'if' clause in step 6, since typed arrays can not have undefined

can't

@@ +1148,5 @@
> +#endif
> +
> +    // Steps 8-9.
> +    for (var k = 1; k < len; k++) {
> +        // Step 9.a.

9a, typically -- and elsewhere.

@@ +1154,5 @@
> +
> +        // Step 9.b.
> +        var nextElement = array[k];
> +
> +        // Steps 9.c-9.d.

9c-d, typically.

@@ +1155,5 @@
> +        // Step 9.b.
> +        var nextElement = array[k];
> +
> +        // Steps 9.c-9.d.
> +        // Omit the 'if' clause in step 9.c, since typed arrays can not have

can't

@@ +1161,5 @@
> +        // side-effect of calling Number.prototype.toLocaleString, because we
> +        // don't yet implement throwing TypeErrors for detached array buffers in
> +        // 9.4.5.8 IntegerIndexedElementGet (step 4). That means, for the time
> +        // being, we throw a TypeError for the wrong reason (and with a wrong
> +        // error message).

Let's have two comments here.  First,

"""
Step 9c *should* be unreachable: typed array elements are numbers.  But bug 1079853 means |nextElement| *could* be |undefined|, if the previous iteration's step 9d detached |array|'s buffer.  Conveniently, if this happens, evaluating |nextElement.toLocaleString| throws the required TypeError, and the only observable difference is in error message.  So despite bug 1079853, we can skip step 9c.
"""

Second, with a blank line in between, the super-simple

// Step 9d.

::: js/src/tests/ecma_6/TypedArray/toLocaleString.js
@@ +82,5 @@
> +Number.prototype.toLocaleString = originalNumberToLocaleString;
> +
> +// Throws a TypeError if detached in Number.prototype.toLocaleString.
> +for (let constructor of constructors) {
> +    let typedArray, detached;

You might as well live dangerously and move the declarations down to first use.  The closure will capture the declarations even if the declarations appear later.  And putting declaration first probably won't optimize, because we're not building a full call reachability graph (and probably couldn't) to determine that the declarations -- even textually before the closure's evaluated -- dominate all uses of them.

@@ +95,5 @@
> +
> +    // No error for single element arrays.
> +    detached = false;
> +    typedArray = new constructor(1);
> +    assertEq(typedArray.toLocaleString(), localeZero);

Erm, are you sure this shouldn't be comparing to "0"?  With toLocaleString overwritten, and returning |this|, it should return |0| and so stringify to "0".  Try running the test like

  LANG=ar_IQ python tests/jstests.py dbg/js/src/js Intl

and see if it passes.  It doesn't appear to me that it should -- if I'm wrong and it does, we need to debug that.

Incidentally, it doesn't appear anything is testing the number of arguments provided to toLocaleString -- please test that, and verify that it's 0 for builds without Intl (you'll need an Intl-agnostic test for this, probably a separate one) and 2 with, and that if you pass an explicit locale, that gets properly passed through to the actual toLocaleString called.

::: js/src/tests/ecma_6/TypedArray/toString.js
@@ +1,1 @@
> +const TypedArrayPrototype = Object.getPrototypeOf(Int8Array.prototype);

I didn't look at this file, because it and the toString change in TypedArrayObject.cpp need to be split out into a separate patch, on account of |assertEq(TypedArrayPrototype.toString, Array.prototype.toString);| not passing with the test as written.

::: js/src/vm/TypedArrayObject.cpp
@@ +1334,5 @@
>      JS_SELF_HOSTED_FN("subarray", "TypedArraySubarray", 2, 0),
>  #if 0 /* disabled until perf-testing is completed */
>      JS_SELF_HOSTED_FN("set", "TypedArraySet", 2, 0),
>  #else
> +    JS_FN("set", TypedArrayObject::set, 1, 0),

Change the 2 in the other half of the #if 0 to a 1, too.  No reason to have an instant-fail whenever I get around to finishing that up.

@@ +1359,5 @@
>      JS_SELF_HOSTED_FN("values", "TypedArrayValues", 0, 0),
>      JS_SELF_HOSTED_SYM_FN(iterator, "TypedArrayValues", 0, 0),
>      JS_SELF_HOSTED_FN("includes", "TypedArrayIncludes", 2, 0),
> +    JS_SELF_HOSTED_FN("toString", "ArrayToString", 0, 0),
> +    JS_SELF_HOSTED_FN("toLocaleString", "TypedArrayToLocaleString", 2, 0),

Shouldn't the length of toLocaleString be zero as well?  It's %TypedArray%.prototype.toLocaleString ([ reserved1 [ , reserved2 ] ]) in the spec, and Array.prototype.toLocaleString ([ locales [ , options ]]) in ECMA-402, and "Optional parameters (which are indicated with brackets: [ ])...are not included in the default argument count."

Also it appears that es6draft disagrees on this.  *writes gag asking whether you disagree with him, hastily deletes on double-checking that you didn't actually write this*
Attachment #8770646 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 11

a year ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #10)
> Comment on attachment 8770646 [details] [diff] [review]
> bug1121938.patch
> 
> Review of attachment 8770646 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly r+, with comments, except that the toString test and the toString
> change need to be removed.  Fixing that is a more difficult change, and we
> should either get it right or not do it -- and not let that hold up the
> toLocaleString parts of the patch that are perfectly fine.

Maybe I'm misunderstanding bug 1246926 and bug 1235656, but I thought it's now possible to share a self-hosted function?


> 
> It's probably worth having
> 
>   // ValidateTypedArray, then step 1.
> 

Should I also change the other methods in that file to mention "ValidateTypedArray"?


> 
> @@ +1124,5 @@
> > +    if (isTypedArray) {
> > +        len = TypedArrayLength(array);
> > +    } else {
> > +        len = callFunction(CallTypedArrayMethodIfWrapped, array, array, "TypedArrayLength");
> > +    }
> 
> No bracing on either arm of this.

These lines were actually copied from one of the other methods in builtins/TypedArray.js. Should I remove the braces from all methods using that pattern?


> 
> @@ +1148,5 @@
> > +#endif
> > +
> > +    // Steps 8-9.
> > +    for (var k = 1; k < len; k++) {
> > +        // Step 9.a.
> 
> 9a, typically -- and elsewhere.

The dominant style in that file seems to be "9.a" instead of "9a", should I apply the change regardless?


> 
> @@ +95,5 @@
> > +
> > +    // No error for single element arrays.
> > +    detached = false;
> > +    typedArray = new constructor(1);
> > +    assertEq(typedArray.toLocaleString(), localeZero);
> 
> Erm, are you sure this shouldn't be comparing to "0"?  With toLocaleString
> overwritten, and returning |this|, it should return |0| and so stringify to
> "0".  Try running the test like
> 
>   LANG=ar_IQ python tests/jstests.py dbg/js/src/js Intl
> 
> and see if it passes.  It doesn't appear to me that it should -- if I'm
> wrong and it does, we need to debug that.

Oh, well spotted. Thanks! I'll fix both places were I got this wrong.


> ::: js/src/vm/TypedArrayObject.cpp
> @@ +1334,5 @@
> >      JS_SELF_HOSTED_FN("subarray", "TypedArraySubarray", 2, 0),
> >  #if 0 /* disabled until perf-testing is completed */
> >      JS_SELF_HOSTED_FN("set", "TypedArraySet", 2, 0),
> >  #else
> > +    JS_FN("set", TypedArrayObject::set, 1, 0),
> 
> Change the 2 in the other half of the #if 0 to a 1, too.  No reason to have
> an instant-fail whenever I get around to finishing that up.

Apparently default parameters need to be included in JS_SELF_HOSTED_FN, because when I tried to change it to 1, I got the following assertion error:

js> new Int8Array(1).set([1])[0]
Assertion failure: sourceFun->nargs() == targetFun->nargs(), at /home/andre/hg/mozilla-inbound/js/src/vm/SelfHosting.cpp:3130


> 
> @@ +1359,5 @@
> >      JS_SELF_HOSTED_FN("values", "TypedArrayValues", 0, 0),
> >      JS_SELF_HOSTED_SYM_FN(iterator, "TypedArrayValues", 0, 0),
> >      JS_SELF_HOSTED_FN("includes", "TypedArrayIncludes", 2, 0),
> > +    JS_SELF_HOSTED_FN("toString", "ArrayToString", 0, 0),
> > +    JS_SELF_HOSTED_FN("toLocaleString", "TypedArrayToLocaleString", 2, 0),
> 

Same issue as above, when I changed 2 to 0, this happened:

js> new Int8Array(0).toLocaleString()
Assertion failure: sourceFun->nargs() == targetFun->nargs(), at /home/andre/hg/mozilla-inbound/js/src/vm/SelfHosting.cpp:3130

tests/ecma_6/TypedArray/toLocaleString.js includes a test to ensure `toLocaleString.length === 0` is true.



Thanks for reviewing!
(In reply to André Bargull from comment #11)
> Maybe I'm misunderstanding bug 1246926 and bug 1235656, but I thought it's
> now possible to share a self-hosted function?

Maybe!  Somehow I read bugmail for one or both of those, but I didn't remember that at all.  Carry on!

> Should I also change the other methods in that file to mention
> "ValidateTypedArray"?

Just fix what you're adding.  I'm not horribly concerned about overall consistency on this exact point, except that the code written here mapped confusingly onto ValidateTypedArray plus that step 1.

> These lines were actually copied from one of the other methods in
> builtins/TypedArray.js. Should I remove the braces from all methods using
> that pattern?

Sounds fine by me, but if you only remove it in what you add, that's adequate.  I care most about not regressing; monotonic improvement is otherwise good enough for me.

> The dominant style in that file seems to be "9.a" instead of "9a", should I
> apply the change regardless?

Hmm, I guess maybe not.  *grmbls*

> Apparently default parameters need to be included in JS_SELF_HOSTED_FN,
> because when I tried to change it to 1, I got the following assertion error:
> 
> js> new Int8Array(1).set([1])[0]
> Assertion failure: sourceFun->nargs() == targetFun->nargs(), at
> /home/andre/hg/mozilla-inbound/js/src/vm/SelfHosting.cpp:3130

Hmm, I had a recollection of that but couldn't find a bug documenting it.  Woo tests!
(Assignee)

Comment 13

a year ago
Created attachment 8771660 [details] [diff] [review]
bug1121938.patch

Updated patch for check-in, carrying over r+ from Waldo. 


@Waldo:
It still needs try-servering, could you handle that part, because I don't have access to the try-servers? Thanks!
Attachment #8770646 - Attachment is obsolete: true
Attachment #8771660 - Flags: review+
I am not Waldo, but I pushed to try for you: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f2726326606

btw: You can totally request try access.
(Assignee)

Comment 15

a year ago
(In reply to Tom Schuster [:evilpie] from comment #14)
> I am not Waldo, but I pushed to try for you:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f2726326606

Thank you!

> btw: You can totally request try access.

I really ought to do that someday. (・_・;)
(Assignee)

Comment 16

a year ago
Created attachment 8772058 [details] [diff] [review]
bug1121938.patch

try-server showed some test failures, because the toString output of typed arrays changed. Re-requesting review from Waldo because I don't know how cross-component test failures are usually handled. And if the changes look reasonable, another try-server run is probably necessary.
Attachment #8771660 - Attachment is obsolete: true
Attachment #8772058 - Flags: review?(jwalden+bmo)
Comment on attachment 8772058 [details] [diff] [review]
bug1121938.patch

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

::: js/src/builtin/TypedArray.js
@@ +1121,5 @@
> +        return "";
> +
> +    // Step 3 (reordered).
> +    // We don't (yet?) implement locale-dependent separators.
> +    var separator = ",";

Put this step before the steps 8-9 loop, as in the [].toLocaleString review.

::: js/src/tests/Intl/TypedArray/toLocaleString.js
@@ +1,1 @@
> +// |reftest| skip-if(!this.hasOwnProperty("Intl"))

if (typeof Intl === "object")

@@ +15,5 @@
> +const localeSep = [,,].toLocaleString();
> +
> +const originalNumberToLocaleString = Number.prototype.toLocaleString;
> +
> +// Missing arguments are passed as |undefined|.

Add a version of this loop that passes only a single argument to the outermost toLocaleString, too.

::: js/src/tests/ecma_6/TypedArray/toLocaleString-nointl.js
@@ +1,1 @@
> +// |reftest| skip-if(this.hasOwnProperty("Intl"))

Mild preference for |if (typeof Intl === "object")| again.
Attachment #8772058 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 18

a year ago
Created attachment 8774848 [details] [diff] [review]
bug1121938.patch

Updated patch, carrying r+ from Waldo. Still needs try-server runs.
Attachment #8772058 - Attachment is obsolete: true
Attachment #8774848 - Flags: review+
I assume you actually need somebody to push this (and your other patches) to try?
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebc77752a240
(Assignee)

Comment 21

a year ago
(In reply to Tom Schuster [:evilpie] from comment #19)
> I assume you actually need somebody to push this (and your other patches) to
> try?

Yes, thank you!
Please ignore the gl/webgl failures, those are mine ..

Are these failures in X real?
toolkit/components/osfile/tests/xpcshell/test_osfile_async_setDates.js | test_nonproto - [test_nonproto : 70] 2000 == 0
(Assignee)

Comment 23

a year ago
(In reply to Tom Schuster [:evilpie] from comment #22)
> Are these failures in X real?
> toolkit/components/osfile/tests/xpcshell/test_osfile_async_setDates.js |
> test_nonproto - [test_nonproto : 70] 2000 == 0

Yes.... *grmbl*


xpcshell/test_osfile_async_setDates.js has tests to catch invalid parameters, one of the invalid parameters is an Uint8Array:

http://hg.mozilla.org/integration/mozilla-inbound/file/tip/toolkit/components/osfile/tests/xpcshell/test_osfile_async_setDates.js#l62
---
    // 4. Check that invalid params will cause an exception/rejection.
    {
      for (let p of ["invalid", new Uint8Array(1), NaN]) {
        try {
          yield OS.File.setDates(path, p, modDate);
          do_throw("Invalid access date should have thrown for: " + p);
        } catch (ex) {
          let stat = yield OS.File.stat(path);
          do_check_eq(accDate, stat.lastAccessDate.getTime());
          do_check_eq(modDate, stat.lastModificationDate.getTime());
        }
        ...
---

OS.File.setDates calls normalizeDate (for Unix/Posix platforms), which in turn calls isNaN:

http://hg.mozilla.org/integration/mozilla-inbound/file/tip/toolkit/components/osfile/modules/osfile_unix_front.jsm#l1152
---
     function normalizeDate(fn, date) {
       ...
       if (isNaN(date)) {
         throw new TypeError("|date| parameter of " + fn + " must be a " +
                             "|Date| instance or number");
       }
       ...
---

When the |date| parameter is an Uint8Array with a single element, |isNaN(date)| used to return true (because |isNaN("[object Uint8Array]")| is true). This is no longer the case with the new implementation, because isNaN(new Uint8Array(1)) => isNaN(ToNumber(new Uint8Array(1))) => isNaN(ToNumber(new Uint8Array(1).toString())) => isNaN(ToNumber("0")) =>  isNaN(0) = false. 

The fix should be simple, I just need to add typeof to avoid the implicit coercion with ToNumber:
---
       if (typeof date !== "number" || isNaN(date)) {
         throw new TypeError("|date| parameter of " + fn + " must be a " +
                             "|Date| instance or number");
       }
---

Btw, there are no test failures on Windows, because that platform uses a different normalization method [1].

[1] http://hg.mozilla.org/integration/mozilla-inbound/file/tip/toolkit/components/osfile/modules/osfile_win_front.jsm#l683
(Assignee)

Comment 24

a year ago
Created attachment 8776034 [details] [diff] [review]
bug1121938.patch

Updated toolkit/components/osfile/modules/osfile_unix_front.jsm per comment #23.

Do I now need a separate r+ from a toolkit reviewer?
Attachment #8774848 - Attachment is obsolete: true
Attachment #8776034 - Flags: review+
I was going to comment to note that isNaN(new Date(NaN)), but the isNaN shouldn't be seeing Date objects, given the processing just before, and the @param documentation saying only numbers/Dates should appear.

That code might also consider Number.isNaN, which returns true *only* for the number NaN, and false for everything else.  At least, if it's certain it's preprocessed everything it should into numbers.  Auditing the callers to prove this probably isn't worth it, now.  It's unfortunate the code doesn't contain assertions that would have proven the documented precondition, and thus let us be more strict here.

So given the context above that minor change, and the @param docs, I would say no need for separate r+.
Oh, um, I guess if a typed array was flowing in there, *definitely* some caller is failing to obey the function's contract.  Pfui.  Stab.  Triple-stab.  And I guess this test is testing that -- albeit not super-well, because it's ass-u-meing that all such values will be isNaN, and so objects and things that flow in won't ToNumber() to NaN.

So I guess this is forcibly changing the function to actually do what its @param docs say it allows.  This would be frowny, but I just took a look for setDate textual occurrences in the tree (two setDate functions appear to be the only things that can call this function at a distance), and it's actually not horribly too many.  I audited all the non-test occurrences of setDate, and they're all taking actual Date objects or are omitted, or they're correct inductively.  (I didn't actually see any taking plain numbers, interestingly.)

So I would be inclined to say this doesn't need further review, even still.  This is a little aggressive, but I'd beg forgiveness rather than ask permission if I were the one considering its readiness to land, if a try-run came back clean.
And I guess since your typeof test excludes everything not a number, I would make the isNaN into Number.isNaN because it's *definitely* not going to change semantics.
Waldo, is this where you keep your blog?
Blocks: 1291857
No longer blocks: 1291857
Comment on attachment 8776034 [details] [diff] [review]
bug1121938.patch

Hey, can you please review the OS.File change in this patch (also see the previous comments)? Thanks!
Attachment #8776034 - Flags: review?(dteller)
Comment on attachment 8776034 [details] [diff] [review]
bug1121938.patch

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

Judging by Yoric's activity on bugzilla, he might be away? Switching reviewer.
Attachment #8776034 - Flags: review?(dteller) → review?(nfroyd)
Comment on attachment 8776034 [details] [diff] [review]
bug1121938.patch

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

"because |isNaN("[object Uint8Array]")| is true"...wat.

Thank you for the detailed explanation!
Attachment #8776034 - Flags: review?(nfroyd) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd77aa94d3f9

Comment 33

a year ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fbb8651a171
Implement TypedArray.prototype.toString and .toLocaleString. r=waldo, r=froydnj
Blocks: 1294212
Backed out for causing bug 1294212, which turned out to not just affect the tier-2 taskcluster based jobs:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d96c3e9b3713
Flags: needinfo?(evilpies)
Not my bug.
Flags: needinfo?(evilpies) → needinfo?(andrebargull)
And just in case it's not obvious, what we mean by "breaking" and "permafailing" is "causing to not fail" and "permapassing" since the  test "failure" from your patch was causing that test which we expected to fail to instead unexpectedly pass. You do still have to look at *why* it started to pass, to see whether you actually fixed it or broke some horrible hack in the mechanics of the test, but at least you can start looking happy that you fixed more than you thought you fixed :)
https://dxr.mozilla.org/mozilla-central/rev/6cf0089510fad8deb866136f5b92bbced9498447/testing/web-platform/tests/encrypted-media/Google/encrypted-media-syntax.html#78 looks like it might be at fault here, in that a handful of lines up there's a comment suggesting the typed array there gets stringified.  But I'd have thought "[object Uint8Array]" and "" as strings would be treated identically there, so I dunno.  Worth looking at first-ish, maybe.
(Assignee)

Comment 38

a year ago
Created attachment 8780150 [details] [diff] [review]
bug1121938.patch

Updated patch to remove the test exclusion for "Navigator.requestMediaKeySystemAccess() exceptions" in testing/web-platform/meta/encrypted-media/Google/encrypted-media-syntax.html.ini.

Locally ran the WPT runner to ensure "Navigator.requestMediaKeySystemAccess() exceptions" is now passing with the %TypedArray%.prototype.toString changes. Before this change |navigator.requestMediaKeySystemAccess(new Uint8Array(0), [{}])| reported a NotSupportedError, now it's reporting InvalidAccessError which seems to be correct per https://chromium.googlesource.com/chromium/src/+/9a661f77cc1f3a9c8af1d584d4c3ab28445ac47e. 

Carrying r+ from waldo and nfroyd
Attachment #8776034 - Attachment is obsolete: true
Flags: needinfo?(andrebargull)
Attachment #8780150 - Flags: review+
(Assignee)

Comment 39

a year ago
(In reply to Phil Ringnalda (:philor) from comment #36)
> [...] but at least you can start looking happy that you fixed more than you thought you fixed :)

Unfortunately I'm not looking happy, because I've spent too much time until I've finally realised my local inbound copy was stale: I didn't had the changes from https://hg.mozilla.org/integration/mozilla-inbound/rev/ec92d299ecc0, which means encrypted-media-syntax.html wasn't marked as failing in my local copy. :-/
So can we land this patch?
(Assignee)

Comment 41

a year ago
(In reply to Tom Schuster [:evilpie] from comment #40)
> So can we land this patch?

Yes, I think it's good to go. Well, maybe another Try server run first, just to be sure everything went fine with the updates.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0627aac32bb
Looks like there is a new test failure.
(Assignee)

Comment 43

a year ago
(In reply to Tom Schuster [:evilpie] from comment #42)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0627aac32bb
> Looks like there is a new test failure.

That was an easy one: The detachArrayBuffer function no longer expects a second argument, so the tests need to be updated.

Comment 44

a year ago
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/633c05b48792
Implement TypedArray.prototype.toString and .toLocaleString. r=waldo
(Assignee)

Comment 45

a year ago
(In reply to Pulsebot from comment #44)
> Pushed by evilpies@gmail.com:

Thank you! :-D

Comment 46

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/633c05b48792
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
https://developer.mozilla.org/en-US/Firefox/Releases/51#JavaScript
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/toString
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/toLocaleString
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1314172
Depends on: 1314175
No longer depends on: 1314172
You need to log in before you can comment on or make changes to this bug.