Bug 82468 - Web Inspector: deleting an array property causes it to appear as `undefined × 1` in the console
Summary: Web Inspector: deleting an array property causes it to appear as `undefined ×...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: sam
URL: data:text/html,<script>var%20array%20...
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2012-03-28 08:13 PDT by Mathias Bynens
Modified: 2015-07-19 14:53 PDT (History)
16 users (show)

See Also:


Attachments
Patch (1.48 KB, patch)
2012-04-25 04:11 PDT, sam
no flags Details | Formatted Diff | Diff
Patch (112.35 KB, patch)
2012-04-25 08:26 PDT, sam
no flags Details | Formatted Diff | Diff
Patch (4.55 KB, patch)
2012-04-25 08:44 PDT, sam
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cq-03 (6.17 MB, application/zip)
2012-04-25 10:36 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-01 (6.26 MB, application/zip)
2012-04-25 14:02 PDT, WebKit Review Bot
no flags Details
Patch (5.40 KB, patch)
2012-07-11 01:12 PDT, sam
pfeldman: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (349.07 KB, application/zip)
2012-07-11 02:16 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Mathias Bynens 2012-03-28 08:13:00 PDT
Open the console and enter:

    var array = [0, 1, 2, 3]; delete array[0]; array;

This will log the following to the console:

    [undefined × 1, 1, 2, 3]

However, this:

    [undefined, 1, 2, 3];

…will log:

    [undefined, 1, 2, 3]

What’s with the `undefined × 1` thing? Why the different output for these two scenarios? Similarly, the following:

    var array = [0, 1, 2, 3]; delete array[0]; delete array[1]; array;

…logs:

    [undefined × 2, 2, 3]

Which makes more sense, as this saves space compared to logging:

    [undefined, undefined, 2, 3]

Is this documented anywhere?
Comment 1 Mathias Bynens 2012-03-28 10:27:34 PDT
Another example: `Array(2)` logs `[undefined × 2]`, but `[undefined, undefined]` logs `[undefined, undefined]`.
Comment 2 sam 2012-04-25 04:11:37 PDT
Created attachment 138780 [details]
Patch
Comment 3 Pavel Feldman 2012-04-25 06:46:43 PDT
Comment on attachment 138780 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138780&action=review

> Source/WebCore/inspector/front-end/ConsoleMessage.js:341
> +            if (!element || element.textContent === "undefined")

This change does not address the bug. You should instead modify appendUndefined to not render "x 1" in case it is actually "1". The code was not handling the undefined as the first value. This also needs a test (see LayoutTests/inspector/console/console-format.html).
Comment 4 sam 2012-04-25 07:20:44 PDT
(In reply to comment #3)
> (From update of attachment 138780 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138780&action=review
> 
> > Source/WebCore/inspector/front-end/ConsoleMessage.js:341
> > +            if (!element || element.textContent === "undefined")
> 
> This change does not address the bug. You should instead modify appendUndefined to not render "x 1" in case it is actually "1". The code was not handling the undefined as the first value. This also needs a test (see LayoutTests/inspector/console/console-format.html).

Thanks for review Pavel.

I have though of the same thing first. 
But then though of other cases as well while cross checking.

Following are some of the cases and their o/p with checking for render "x 1" in case it is actually "1" in appendUndefined.

1).var array = [0, 1, 2, 3]; delete array[0]; array;
[undefined, 1, 2, 3]

2).var array = [undefined, undefined]; array;
[undefined, undefined]
--- I guess this should be handled as undefined*2. 
It would happen as the undefined values gets missed in while going through array elements and its corresponding properties in the loop just below appendUndefined().
Since there is no delete here, all properties value are in order and there will be no cumulation for undefined.

3).var array = [undefined, undefined, 1, 2, 3]; delete array[2]; delete array[3] ;array;
[undefined, undefined, undefined × 2, 3]
--- Same here.


But I guess check for "1" should be in place as "undefined" is better than "undefined x 1".

Kindly suggest.
Comment 5 Pavel Feldman 2012-04-25 07:35:50 PDT
Here is why delete and simple undefined work differently:

> var a = [undefined, undefined, 3]; Object.keys(a);
["0", "1", "2"]

> var a = [1, 2, 3]; delete a[0]; delete a[1]; Object.keys(a);
["2"]


Anyways, in addition to the fix in appendUndefined, you should then change the loop in the _printArray to ignore values that have type undefined:

for (var i = 0; i < properties.length; ++i) {
    var property = properties[i];
    var name = property.name;
    if (!isNaN(name) >>>>>>>>>>&& property.value.type !== "undefined"<<<<<<<<<<<)
        elements[name] = this._formatAsArrayEntry(property.value);
}
Comment 6 sam 2012-04-25 08:26:04 PDT
Created attachment 138815 [details]
Patch
Comment 7 sam 2012-04-25 08:27:55 PDT
(In reply to comment #5)
> Here is why delete and simple undefined work differently:
> 
> > var a = [undefined, undefined, 3]; Object.keys(a);
> ["0", "1", "2"]
> 
> > var a = [1, 2, 3]; delete a[0]; delete a[1]; Object.keys(a);
> ["2"]
> 
> 
> Anyways, in addition to the fix in appendUndefined, you should then change the loop in the _printArray to ignore values that have type undefined:
> 
> for (var i = 0; i < properties.length; ++i) {
>     var property = properties[i];
>     var name = property.name;
>     if (!isNaN(name) >>>>>>>>>>&& property.value.type !== "undefined"<<<<<<<<<<<)
>         elements[name] = this._formatAsArrayEntry(property.value);
> }

ohh, thanks Pavel!, have uploaded the patch.
Comment 8 Pavel Feldman 2012-04-25 08:31:14 PDT
Comment on attachment 138815 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138815&action=review

One small nit and it is good to go. You don't need to change localizedStrings here. (resetting r?).

> Source/WebCore/inspector/front-end/ConsoleMessage.js:337
> +                span.textContent = WebInspector.UIString("undefined");

You don't need to localize "undefined" - it is a JavaScript term. The reason it was localized previously was "x %d" fragment.
Comment 9 sam 2012-04-25 08:44:36 PDT
Created attachment 138819 [details]
Patch
Comment 10 sam 2012-04-25 09:48:51 PDT
(In reply to comment #8)
> (From update of attachment 138815 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138815&action=review
> 
> One small nit and it is good to go. You don't need to change localizedStrings here. (resetting r?).
> 
> > Source/WebCore/inspector/front-end/ConsoleMessage.js:337
> > +                span.textContent = WebInspector.UIString("undefined");
> 
> You don't need to localize "undefined" - it is a JavaScript term. The reason it was localized previously was "x %d" fragment.

okk, have removed it from localizedStrings. 

Is it ok ?
Comment 11 WebKit Review Bot 2012-04-25 10:36:33 PDT
Comment on attachment 138819 [details]
Patch

Rejecting attachment 138819 [details] from commit-queue.

New failing tests:
inspector/console/console-format.html
Full output: http://queues.webkit.org/results/12520608
Comment 12 WebKit Review Bot 2012-04-25 10:36:40 PDT
Created attachment 138839 [details]
Archive of layout-test-results from ec2-cq-03

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 13 WebKit Review Bot 2012-04-25 14:01:08 PDT
Comment on attachment 138819 [details]
Patch

Attachment 138819 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12528690

New failing tests:
inspector/console/console-format.html
Comment 14 WebKit Review Bot 2012-04-25 14:02:09 PDT
Created attachment 138872 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 15 sam 2012-07-11 01:12:17 PDT
Created attachment 151639 [details]
Patch
Comment 16 sam 2012-07-11 01:14:37 PDT
(In reply to comment #15)
> Created an attachment (id=151639) [details]
> Patch

Hi Pavel,

I have uploaded the patch again (content remains the same) as I guess it could not be committed earlier due bot error.
Comment 17 WebKit Review Bot 2012-07-11 02:16:20 PDT
Comment on attachment 151639 [details]
Patch

Attachment 151639 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13180381

New failing tests:
inspector/console/console-format.html
Comment 18 WebKit Review Bot 2012-07-11 02:16:29 PDT
Created attachment 151654 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment 19 Pavel Feldman 2012-07-11 04:39:08 PDT
Comment on attachment 151639 [details]
Patch

Please make sure you rebaseline Layouttests/platform/chromium/inspector/console-fromat-expexted.txt prior to landing.
Comment 20 Nick Krempel 2013-02-14 07:42:15 PST
This doesn't seem like a bug to me. I see the '[undefined × 1]' output as a feature, which lets you distinguish between sparse arrays and arrays filled with the value 'undefined'. If you're going to remove the '× 1' part, then perhaps you should also:

(a) Replace 'undefined' with 'missing', so that:

   Array(1)                 =>   [missing]
   Array(2)                 =>   [missing × 2]

(ideally with 'missing' in italics, but not essential.)

(b) Also group genuine 'undefined' values, so that:

   [undefined]              =>   [undefined]
   [undefined, undefined]   =>   [undefined × 2]

I see (a) as more important than (b), and also an easy change.
Comment 21 Radar WebKit Bug Importer 2014-12-17 11:21:33 PST
<rdar://problem/19281480>
Comment 22 Timothy Hatcher 2015-01-23 13:30:28 PST
Should we distinguish missing and undefined? Close or keep?
Comment 23 Brian Burg 2015-07-19 14:53:20 PDT
Joe's rewrite of array previews has addressed the ambiguity between undefined elements and holes. Closing.