1403065 - Object actor's enumProperties assume incorrect things about length property

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

In the `enumProperties` function (http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/server/actors/object.js#893,902-903,919-924), we can pass some options to only retrieve indexed or non indexed properties.
The problem when doing this is that it relies on the value of a `length` property to get what's before (indexed) or after (non-indexed). 
But if you have a plain object with a length property like `{length: 789}`, and try to retrieve non-indexed properties, it will return an empty array.

We should fix this function to not rely on the length property but check the properties like we do when we don't have a length attribute (http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/devtools/server/actors/object.js#904-912)

Comment 1

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1403065 - Fix enumObjectProperties for exotic objects; .

In the `enumProperties` function , we can pass some options to only retrieve
indexed or non indexed properties. The problem when doing this is that we were relying
on the value of a `length` property to get what's before (indexed) or after (non-indexed).
But if you have a plain object with a length property like `{length: 789}`,
and try to retrieve non-indexed properties, the function would return an empty ownProperties
object.

In this patch we make extra checks in the function to make sure we do return the expected
data to the client. A unit-test is added to ensure we don't regress.

Review commit: https://reviewboard.mozilla.org/r/185096/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/185096/

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

I did not find any way to fix Bug 1387823 at the same time, so pushing it to review as is since it fixes a bad bug.

Comment 3

[Oriol Brufau [:Oriol]]

Comment on attachment 8913694 [details]
Bug 1403065 - Fix enumObjectProperties for exotic objects; .

https://reviewboard.mozilla.org/r/185096/#review190148

::: devtools/server/actors/object.js:915
(Diff revision 1)
>  
>    if (options.ignoreNonIndexedProperties || options.ignoreIndexedProperties) {
>      let length = DevToolsUtils.getProperty(objectActor.obj, "length");
> -    if (typeof length !== "number") {
> -      // Pseudo arrays are flagged as ArrayLike if they have
> -      // subsequent indexed properties without having any length attribute.
> +    let sliceIndex;
> +    // let's check if the `length - 1` property is indeed a number.
> +    if (typeof length !== "number" || isNaN(names[length - 1])) {

`typeof length === "number"` is not enough. It could be unsafe, decimal or negative (including `-0`), and then it would be an invalid length. The proper way is

```js
function isIntegerLength(num) {
  return Number.isSafeInteger(num) && 1/num > 0;
}
```

`!isNaN` is not a proper way to recognize integer indices. The proper way is

```js
function isIntegerIndex(str) {
  let num = +str;
  return isIntegerLength(num) && num + "" === str;
}
```

Additionally, you should only check `names[length - 1]` if `length` is not `0`.

So the condition should be

```js
if (!isIntegerLength(length) || length && !isIntegerIndex(names[length - 1])) {
```

However, I think all the code could be simplified to

```js
let sliceIndex;
if (isIntegerLength(length)) {
  sliceIndex = length;
} else if (!isIntegerIndex(names[0])) {
  sliceIndex = 0;
} else {
  sliceIndex = names.length;
}
while (sliceIndex > 0) {
  if (isIntegerIndex(names[sliceIndex - 1])) {
    break;
  }
  --sliceIndex;
}
```

::: devtools/server/actors/object.js:945
(Diff revision 1)
>      if (options.ignoreIndexedProperties) {
> -      // Keep items after `length` index
> -      names = names.slice(length);
> +      // Keep items after `sliceIndex` index
> +      names = names.slice(sliceIndex);
>      } else if (options.ignoreNonIndexedProperties) {
> -      // Remove `length` first items
> -      names.splice(length);
> +      // Remove `sliceIndex` first items
> +      names.splice(sliceIndex);

`names.splice(sliceIndex)` returns an useless array with the removed properties. `names.length = sliceIndex` is faster.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8913694 [details]
Bug 1403065 - Fix enumObjectProperties for exotic objects; .

https://reviewboard.mozilla.org/r/185096/#review190204

This looks pretty good to me - clearing the flag so you have a chance to address or respond to Comment 3. Can you please ask both me and Oriol for review on the next version?

::: devtools/server/actors/object.js:824
(Diff revision 1)
>        this.iterator = enumWeakSetEntries(objectActor);
>      } else {
>        throw new Error("Unsupported class to enumerate entries from: " + cls);
>      }
> -  } else if (options.ignoreNonIndexedProperties && !options.query) {
> +  } else if (
> +    TYPED_ARRAY_CLASSES.concat("Array").includes(cls)

Nit: I'd prefer to introduce a new const at the top of the file like ALL_ARRAY_CLASSES instead of doing it inline. Or maybe better: migrate existing TYPED_ARRAY_CLASSES.includes/indexOf calls to a new isTypedArray function and then add a second isArray function that returns `isTypedArray || obj.class == "Array";`

Comment 5

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8913694 [details]
Bug 1403065 - Fix enumObjectProperties for exotic objects; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185096/diff/1-2/

Comment 6

[Oriol Brufau [:Oriol]]

Comment on attachment 8913694 [details]
Bug 1403065 - Fix enumObjectProperties for exotic objects; .

https://reviewboard.mozilla.org/r/185096/#review190594

::: devtools/server/actors/object.js:912
(Diff revision 2)
>      let length = DevToolsUtils.getProperty(objectActor.obj, "length");
> -    if (typeof length !== "number") {
> -      // Pseudo arrays are flagged as ArrayLike if they have
> -      // subsequent indexed properties without having any length attribute.
> -      length = 0;
> -      for (let key of names) {
> +    let sliceIndex;
> +
> +    if (
> +      !isSafePositiveInteger(length)
> +      || !isSafeIndex(names[length - 1])

I still think you should not check `names[length - 1]` if `length` is `0`. You can use one iteration of the `while` loop to check this when necessary.

::: devtools/server/actors/object.js:923
(Diff revision 2)
> +        // If the first item is not a number, this means there is no indexed properties
> +        // in this object.
> +        sliceIndex = 0;
> +      } else {
> +        sliceIndex = names.length;
> +        while (sliceIndex >= 0) {

It should be `while (sliceIndex > 0)`

::: devtools/server/tests/unit/test_objectgrips-20.js:31
(Diff revision 2)
> +
> +  const dbgClient = new DebuggerClient(server.connectPipe());
> +  await dbgClient.connect();
> +  const [,, threadClient] = await attachTestTabAndResume(dbgClient, "test-grips");
> +
> +  [{

Can you add tests with a `length` valid but not greater than some index, e.g. `{0:0, length:0}` and `{0:0, 1:1, length:1}`?

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8913694 [details]
Bug 1403065 - Fix enumObjectProperties for exotic objects; .

https://reviewboard.mozilla.org/r/185096/#review190622

Nothing to add past Oriol's comments. Thanks for doing the utility function cleanups

Comment 8

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8913694 [details]
Bug 1403065 - Fix enumObjectProperties for exotic objects; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185096/diff/2-3/

Comment 9

[Oriol Brufau [:Oriol]]

Comment on attachment 8913694 [details]
Bug 1403065 - Fix enumObjectProperties for exotic objects; .

https://reviewboard.mozilla.org/r/185096/#review191006

Looks good to me after fixing the following.

::: devtools/server/actors/object.js:914
(Diff revisions 2 - 3)
>  
> -    if (
> -      !isSafePositiveInteger(length)
> -      || !isSafeIndex(names[length - 1])
> -    ) {
> -      // The length property does not reflect what the object looks like, let's find
> +    const isLengthTrustworthy =
> +      isSafePositiveInteger(length)
> +      && length === 0
> +      && isSafeIndex(names[length - 1])
> +      && !isSafeIndex(names[length]);

Always imposing `length === 0` is too strict :)

It should be something like

```
const isLengthTrustworthy =
  isSafePositiveInteger(length)
  && (!length || isSafeIndex(names[length - 1]))
  && !isSafeIndex(names[length]);
```

Comment 10

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8913694 [details]
Bug 1403065 - Fix enumObjectProperties for exotic objects; .

https://reviewboard.mozilla.org/r/185096/#review191006

> Always imposing `length === 0` is too strict :)
> 
> It should be something like
> 
> ```
> const isLengthTrustworthy =
>   isSafePositiveInteger(length)
>   && (!length || isSafeIndex(names[length - 1]))
>   && !isSafeIndex(names[length]);
> ```

yeah it strikes me now, thanks !

Comment 11

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8913694 [details]
Bug 1403065 - Fix enumObjectProperties for exotic objects; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185096/diff/3-4/

Comment 12

[Oriol Brufau [:Oriol]]

Comment on attachment 8913694 [details]
Bug 1403065 - Fix enumObjectProperties for exotic objects; .

https://reviewboard.mozilla.org/r/185096/#review191046

::: devtools/server/actors/object.js:912
(Diff revisions 3 - 4)
>      let length = DevToolsUtils.getProperty(objectActor.obj, "length");
>      let sliceIndex;
>  
>      const isLengthTrustworthy =
>        isSafePositiveInteger(length)
> -      && length === 0
> +      && length > 0

The thing is that a length of 0 may be trustworthy. You should not impose `length > 0`. It's just that `isSafeIndex(names[length - 1])` should only be checked if `length > 0`.

Comment 13

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8913694 [details]
Bug 1403065 - Fix enumObjectProperties for exotic objects; .

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/185096/diff/4-5/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30e7b63ebdf4
Fix enumObjectProperties for exotic objects; r=bgrins,Oriol.

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/30e7b63ebdf4