1282465 - Reps: fix or remove recursive handling in ArrayRep and Obj rep

Status: RESOLVED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Lin Clark [:linclark]]

Currently there is no way to get to the recursive handling in grip-array. It should either be fixed or removed.

Comment 1

[Evan Tseng [:evanxd]]

Hi Lin,

Could you help to elaborate more details about the bug?

Cannot get it, recursive handling for what?

Thanks.

Comment 2

[Lin Clark [:linclark]]

This line here can't be reached: https://dxr.mozilla.org/mozilla-central/source/devtools/client/shared/components/reps/grip-array.js#69

Comment 3

[Dalimil Hajek [:dalimil]]

Attachment: screenshot-when-recursion-ignored.png

I spent some time trying to fix this but I found no way to directly access the actual array elements for equality comparison -> The array in grip-array.js contains itemGrip elements (not the actual primitive array values) and these only have their 'preview' property which for 'arrays inside this array' doesn't give me access to the actual values.

At the same time, I don't think that removing this code is a good solution as I find the current display format very confusing - e.g. arr = [3, 4, [5, 6, 7] ] is displayed as [3, 4, [ 3 ]] - it is hard to tell if the inner array contains 3 elements or only a single element - that is number 3.  (see the attached screenshot)

Comment 4

[Lin Clark [:linclark]]

Honza, what do you think the best course of action here is?

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Dalimil Hajek [:dalimil] from comment #3)
> Created attachment 8773282 [details]
> screenshot-when-recursion-ignored.png
> 
> I spent some time trying to fix this but I found no way to directly access
> the actual array elements for equality comparison -> The array in
> grip-array.js contains itemGrip elements (not the actual primitive array
> values) and these only have their 'preview' property which for 'arrays
> inside this array' doesn't give me access to the actual values.
Correct.

This bug is only related to ArrayRep (array.js) and Obj rep (object.js). These reps are directly dealing with live JS objects. Note that this bug is more about analysis making sure the recursion problem can't accidentally happen the code is solid. We might also want to put some comments into the code to make sure this isn't hidden somewhere in-between the lines.

The problem is displaying props that refer back to the object.

E.g.

let obj = {};
obj.self = obj;

Preview of such object would be:

Object { self: Object }

This is ok, since the `self` property uses 'tiny' mode and renders only `Object` label. The problem could appear if the prop uses e.g. 'short' mode:

Object { self: Object{self: Object{self: ...} }

Now there is a recursion since the object is trying to show its first prop, which points to the same object trying to show its first prop, which ...

We should make sure this can't happen. I think that we want to always use 'tiny' mode for Object props and Array items and so, it should be safe to hardcode it into: Obj.getProps() and ArrayRep.arrayIterator(). This + nice comment can make the code safe. Consequently we can also remove the Reference() component that was designed to be displayed instead of cycle refs.

> At the same time, I don't think that removing this code is a good solution
> as I find the current display format very confusing - e.g. arr = [3, 4, [5,
> 6, 7] ] is displayed as [3, 4, [ 3 ]] - it is hard to tell if the inner
> array contains 3 elements or only a single element - that is number 3.  (see
> the attached screenshot)
The display should be as follows:

Array [ 3, 4, Array[3] ]

We agreed to use titles for all Array like objects somewhere (I can't find the bug tho)

Honza

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

There are some related bugs in-progress, we should wait till they are landed  to avoid conflicts.

Honza

Comment 7

[Evan Tseng [:evanxd]]

Attachment: bug-1282465.patch

Hi Honza,

Could you help to review the patch?

Thanks.

The try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4edd1847851

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8781039 [details] [diff] [review]
bug-1282465.patch

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

Looks good, thanks for cleaning this up!

Btw. there is yet Reference() component in grip-array.js, that one should also be removed.
Do you want to look at it too? (can be new bug report).

Honza

Comment 9

[Evan Tseng [:evanxd]]

Thanks for the review, Honza.

Sure, let remove the Reference component in Bug 1295491.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/061b968f41ad
Hardcode tiny mode for array and object reps. r=Honza

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/061b968f41ad