1245495 - Improve DataView out of bounds error messages

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Florian Scholz (Open Web Docs)]

I think the DataView error messages could be improved. Especially "argument 1" seems wrong in situations a and b.

Situation a:
var arrayBuffer = new ArrayBuffer(16);
var dataView = new DataView(arrayBuffer, 17);
> Firefox: RangeError: argument 1 accesses an index that is out of range
> Chrome: Uncaught RangeError: Start offset is outside the bounds of the buffer

Situation b:
var arrayBuffer = new ArrayBuffer(16);
var dataView = new DataView(arrayBuffer, 15, 20);
> Firefox: RangeError: argument 1 accesses an index that is out of range
> Chrome: Uncaught RangeError: Invalid data view length

Situation c:
var arrayBuffer = new ArrayBuffer(16);
var dataView = new DataView(arrayBuffer);
dataView.getUint32(20);
> Firefox: RangeError: argument 1 accesses an index that is out of range
> Chrome: Uncaught RangeError: Offset is outside the bounds of the DataView

Comment 1

[kikuchi1024r]

I'm trying to solve this problem.

Comment 2

[Tooru Fujisawa [:arai]]

Thank you!

Comment 3

[kikuchi1024r]

Attachment: Change error message of case a, b, and c.

Now the error messages are above like the reporter says. 

Now the error message uses the number of arguments, so when the number doesn't match, you might confuse what is wrong.

In this patch, those are changed like below.

if you access an address which is not allocated for the reason that offset is outside of the bounds, error message is below.(reporter's situation a,c)

when the allocation is for buffer, (situation a)
```
RangeError: Start offset is outside the bounds of the buffer
```

when the allocation is for DataView, (situation c)
```
RangeError: Offset is outside the bounds of the DataView
```

if you access an address which is not allocated for the reason that data view length is too long and the end is outside of the bounds, error message is below.(reporter's situation b)
```
RangeError: Invalid data view length
```

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8923689 [details] [diff] [review]
Change error message of case a, b, and c.

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

Thank you for your patch :D

The code change itself looks good.
Please fix some formatting changes and ask review again.

::: js/src/builtin/DataViewObject.cpp
@@ +171,4 @@
>  
>      // Step 7.
>      if (offset > bufferByteLength) {
> +	JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_OFFSET_OUT_OF_BUFFER);

please use SPACEs instead of HTABs.

::: js/src/js.msg
@@ +56,4 @@
>  MSG_DEF(JSMSG_TOPRIMITIVE_RETURNED_OBJECT, 2, JSEXN_TYPEERR, "can't convert {0} to {1}: its [Symbol.toPrimitive] method returned an object")
>  MSG_DEF(JSMSG_NO_PROPERTIES,           1, JSEXN_TYPEERR, "{0} has no properties")
>  MSG_DEF(JSMSG_BAD_REGEXP_FLAG,         1, JSEXN_SYNTAXERR, "invalid regular expression flag {0}")
> +MSG_DEF(JSMSG_INVALID_DATA_VIEW_LENGTH,  0, JSEXN_RANGEERR, "Invalid data view length")

please align "0" to "1" in the previous line.

also, currently we're not using capital (in most case),
so please use lowercase for the initial letter.

@@ +60,1 @@
>  MSG_DEF(JSMSG_ARG_INDEX_OUT_OF_RANGE,  1, JSEXN_RANGEERR, "argument {0} accesses an index that is out of range")

now the remaining consumer of JSMSG_ARG_INDEX_OUT_OF_RANGE is only in js/src/shell/js.cpp .

can you file a followup bug to improve the error there as well?
https://dxr.mozilla.org/mozilla-central/rev/1c618b1a13662de7cec429f606367db3827b6dc7/js/src/shell/js.cpp#1117

@@ +60,2 @@
>  MSG_DEF(JSMSG_ARG_INDEX_OUT_OF_RANGE,  1, JSEXN_RANGEERR, "argument {0} accesses an index that is out of range")
> +MSG_DEF(JSMSG_OFFSET_OUT_OF_BUFFER,  0, JSEXN_RANGEERR, "Start offset is outside the bounds of the buffer")

please align "0" to "0" in the next line

Comment 5

[kikuchi1024r]

Attachment: bug1245495.patch

Thank you for your review.

I changed what you mentioned.

Comment 6

[Tooru Fujisawa [:arai]]

Comment on attachment 8923716 [details] [diff] [review]
bug1245495.patch

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

Thanks again!

Here's try run (automation for build and test), that will take 2-3 hours.
when it runs without failure (or only know failure), feel free to add "checkin-needed" keyword to this bug, so this patch will be landed shortly.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f62f7ebda007a590e94bbd2998fa2f69bb827de1

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e31bc6095e05
Change error message about offset and data view length. r=arai

Comment 8

[Natalia Csoregi [:nataliaCs]]

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