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

RESOLVED FIXED in mozilla27

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
22 days ago

People

(Reporter: Waldo, Assigned: Wesley Chalmers)

Tracking

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bz][lang=c++])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
Whiteboard: [mentor=bz][lang=c++]
(Assignee)

Comment 1

5 years ago
Please assign to me.
(Reporter)

Comment 2

5 years ago
Done!
Assignee: nobody → wwchalme
(Assignee)

Comment 3

5 years ago
Created attachment 804899 [details] [diff] [review]
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.
Attachment #804899 - Flags: feedback+
(Assignee)

Comment 4

5 years ago
How should tests be written for this patch? My guess is JavaScript.
(Assignee)

Updated

5 years ago
Attachment #804899 - Flags: feedback+ → feedback?(jwalden+bmo)
(Assignee)

Updated

5 years ago
Attachment #804899 - Flags: feedback?(jwalden+bmo) → feedback?(bzbarsky)
(Assignee)

Comment 5

5 years ago
Can you tell I'm new?
(Assignee)

Comment 6

5 years ago
Created attachment 804926 [details] [diff] [review]
891537.patch: Fixed for unsigned long long as well.
Attachment #804926 - Flags: feedback?
(Assignee)

Updated

5 years ago
Attachment #804926 - Flags: feedback? → feedback?(bzbarsky)
(Assignee)

Comment 7

5 years ago
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.
Attachment #804899 - Attachment is obsolete: true
Attachment #804899 - Flags: feedback?(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.
Attachment #804926 - Flags: feedback?(bzbarsky) → feedback+
(Reporter)

Comment 9

5 years ago
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.
(Assignee)

Comment 10

5 years ago
(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"?!
Well, assuming you've done the manual testing to make sure it actually fixes the bug.
(Assignee)

Comment 12

5 years ago
(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.
> 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.
(Assignee)

Comment 14

5 years ago
Created attachment 811763 [details]
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.
(Assignee)

Comment 15

5 years ago
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;
>};
Attachment #811763 - Attachment description: ProgressEvent.webidl - Wutg Altered Fields → ProgressEvent.webidl - With Altered Fields
(Assignee)

Comment 16

5 years ago
Created attachment 811764 [details]
test_bug891537_long.html

Test for long long fields. Should not throw exception.
(Assignee)

Comment 17

5 years ago
Created attachment 811765 [details]
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.
(Assignee)

Comment 18

5 years ago
The attached tests/changes pass when the patch is applied and fail when the patch is not applied.
Great, thank you for checking that!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab4c0533cff4
Target Milestone: --- → mozilla27
Heycam, does this have tests in your WebIDL test suite?
Flags: needinfo?(cam)
https://hg.mozilla.org/mozilla-central/rev/ab4c0533cff4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(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.)
Flags: needinfo?(cam)
You need to log in before you can comment on or make changes to this bug.