891537 - WebIDL conversion to unsigned long long, long long has boundary-condition errors

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jeff Walden [:Waldo]]

The current code:

http://hg.mozilla.org/mozilla-central/annotate/04d8c309fe72/dom/bindings/PrimitiveConversions.h#l188

appears to reject 2**53 and -2**53 as valid numbers, when converting to int64_t/uint64_t.  Apparently the spec said to do this at one point, but now these values may acceptably convert to/from JS double numbers, so the - 1 in these needs to go away, and tests need to be written for them.

http://dev.w3.org/2006/webapi/WebIDL/#es-long-long

Comment 1

[Wesley Chalmers]

Please assign to me.

Comment 2

[Jeff Walden [:Waldo]]

Done!

Comment 3

[Wesley Chalmers]

Attachment: 891537.patch: Changes the limits in the file linked in bug description. Should allow compliance with the WebIDL standard

This is my first patch, and it assumes that the only change that was needed was the limits in the file linked in the bug description. Have not written tests yet.

Comment 4

[Wesley Chalmers]

How should tests be written for this patch? My guess is JavaScript.

Comment 5

[Wesley Chalmers]

Can you tell I'm new?

Comment 6

[Wesley Chalmers]

Attachment: 891537.patch: Fixed for unsigned long long as well.

Comment 7

[Wesley Chalmers]

I've tried writing an HTML page with a script that declares a variable of size 2**53. Patched and unpatched versions seem to accept i without throwing an exception. I'm not sure how to test this.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 804926 [details] [diff] [review]
891537.patch: Fixed for unsigned long long as well.

Looks fine.

In terms of tests, you want to find APIs that take these types with [EnforceRange] or [Clamp], pass 1<<53 and see what happens.  

For "unsigned long long", you could modify the members of the ProgressEventInit dictionary (e.g. "loaded") to be [EnforceRange] and then try:

  new ProgressEvent("x", { loaded: Math.pow(2, 53) }).loaded

For "long long", maybe see dateTime in CameraManager.webidl?

Checked-in tests are hard: no in-tree API exercises this code.

Comment 9

[Jeff Walden [:Waldo]]

Note that (1 << 53) won't do what you want in JS, because << produces only signed 32-bit integers.  You want Math.pow(2, 53) and such instead.

Comment 10

[Wesley Chalmers]

(In reply to Boris Zbarsky [:bz] from comment #8)
> Comment on attachment 804926 [details] [diff] [review]
> 891537.patch: Fixed for unsigned long long as well.
> 
> Looks fine.
> 
> In terms of tests, you want to find APIs that take these types with
> [EnforceRange] or [Clamp], pass 1<<53 and see what happens.  
> 
> For "unsigned long long", you could modify the members of the
> ProgressEventInit dictionary (e.g. "loaded") to be [EnforceRange] and then
> try:
> 
>   new ProgressEvent("x", { loaded: Math.pow(2, 53) }).loaded
> 
> For "long long", maybe see dateTime in CameraManager.webidl?
> 
> Checked-in tests are hard: no in-tree API exercises this code.

Does "Looks fine" mean "We're checking it in"?!

Comment 11

[Boris Zbarsky [:bzbarsky]]

Well, assuming you've done the manual testing to make sure it actually fixes the bug.

Comment 12

[Wesley Chalmers]

(In reply to Boris Zbarsky [:bz] from comment #11)
> Well, assuming you've done the manual testing to make sure it actually fixes
> the bug.

Can the change to the ProgressEvent API be made without breaking anything?
I guess I need to run all tests to find out.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Can the change to the ProgressEvent API be made without breaking anything?

That change can't land, which is why the testing needs to be manual.  Write the test, change ProgressEvent locally, test with it changed locally.

Comment 14

[Wesley Chalmers]

Attachment: ProgressEvent.webidl - With Altered Fields

These are the changes I made to the ProgressEvent class to include [EnforceRange] attributes on the existing field, loaded, and a new (long long) attribute, longElement.

Comment 15

[Wesley Chalmers]

Comment on attachment 811763 [details]
ProgressEvent.webidl - With Altered Fields

>/* -*- Mode: IDL; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>/* This Source Code Form is subject to the terms of the Mozilla Public
> * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> * You can obtain one at http://mozilla.org/MPL/2.0/.
> */
>
>[Constructor(DOMString type, optional ProgressEventInit eventInitDict), HeaderFile="GeneratedEventClasses.h"]
>interface ProgressEvent : Event
>{
>  readonly attribute boolean lengthComputable;
>  readonly attribute unsigned long long loaded;
>  readonly attribute unsigned long long total;
>};
>
>dictionary ProgressEventInit : EventInit
>{
>  boolean lengthComputable = false;
>  [EnforceRange]
>  unsigned long long loaded = 0;
>  [EnforceRange]
>  long long longElement = 0;
>  unsigned long long total = 0;
>};

Comment 16

[Wesley Chalmers]

Attachment: test_bug891537_long.html

Test for long long fields. Should not throw exception.

Comment 17

[Wesley Chalmers]

Attachment: test_bug891537_unsigned_long.html

Test for unsigned long long fields. Should not throw exception.

Needs the altered ProgressEvent.webidl also attached to this bug.

Comment 18

[Wesley Chalmers]

The attached tests/changes pass when the patch is applied and fail when the patch is not applied.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Great, thank you for checking that!

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

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Heycam, does this have tests in your WebIDL test suite?

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Cameron McCormack (:heycam)]

(In reply to :Ms2ger (⌚ UTC+1/+2) from comment #20)
> Heycam, does this have tests in your WebIDL test suite?

This did end up getting reverted in bug 964583, FWIW.  But the answer is no, I didn't have any tests for (unsigned) long long.    Testing the values at the boundary for the JS -> IDL conversions should be simple enough, but I'm not sure how easy it would be to test the current behavior of the other direction, where we choose the closest representable JS Number.

(This reminded me of https://github.com/w3c/web-platform-tests/pull/271 and I got sad.)