Closed Bug 1403065 Opened 3 years ago Closed 3 years ago

Object actor's enumProperties assume incorrect things about length property

Categories

(DevTools :: Console, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 58

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

(Whiteboard: [reserve-console-html])

Attachments

(1 file)

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)
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [console-html]
Blocks: 1402848
Flags: qe-verify?
Whiteboard: [console-html] → [reserve-console-html]
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 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 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";`
Attachment #8913694 - Flags: review?(bgrinstead)
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}`?
Attachment #8913694 - Flags: review?(oriol-bugzilla)
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
Attachment #8913694 - Flags: review?(bgrinstead) → review+
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]);
```
Attachment #8913694 - Flags: review?(oriol-bugzilla) → review+
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 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`.
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/30e7b63ebdf4
Fix enumObjectProperties for exotic objects; r=bgrins,Oriol.
https://hg.mozilla.org/mozilla-central/rev/30e7b63ebdf4
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.