997219 - Object Inspector doesn't show the value of properties with whitespace name

Status: VERIFIED FIXED

(DevTools :: Object Inspector, defect)

Description

[obrufau]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140415030203

Steps to reproduce:

1. Open Web Console
2. Run `({' ': 123})`. Any combination of spaces, tabs and newlines can be used.
3. Click the returned object


Actual results:

Object Inspector shows the whitespace property (collapsed), but doesn't show its value.


Expected results:

It should show its value.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

I haven't looked at this bug yet, but I'm pretty sure bug 881480 will fix it if we end up doing that first. There must be a simpler fix though.

Comment 2

[Oriol Brufau [:Oriol]]

I also noticed this issue.

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b1fb34b07c17&tochange=ee5ca214e87c

Probably caused by bug 808370.

But before that, the debugger was already affected.

Comment 3

[Oriol Brufau [:Oriol]]

Attachment: object-inspector-whitespace-property.patch

The problem is that VariablesView trims the property names.
https://dxr.mozilla.org/mozilla-central/rev/720b5d2c84d5b253d4dfde4897e13384dc97a46a/devtools/client/shared/widgets/VariablesView.jsm#1260

And at the end, if the string is the empty one, it returns before displaying the value.
https://dxr.mozilla.org/mozilla-central/rev/720b5d2c84d5b253d4dfde4897e13384dc97a46a/devtools/client/shared/widgets/VariablesView.jsm#2451

So the solution seems trimming property name when displaying it, but leaving it untrimmed in _nameString.

Not sure who should review, so choosing :past because he commented.

Comment 4

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8779568 [details] [diff] [review]
object-inspector-whitespace-property.patch

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

Redirecting to Brian.

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8779568 [details] [diff] [review]
object-inspector-whitespace-property.patch

I'm unavailable for a couple of days - forwarding to Nick

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8779568 [details] [diff] [review]
object-inspector-whitespace-property.patch

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

LGTM, but we should have a tiny test so that this doesn't regress in the future.

Comment 7

[Oriol Brufau [:Oriol]]

Attachment: object-inspector-whitespace-property-v2.patch

Adding test

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8779844 [details] [diff] [review]
object-inspector-whitespace-property-v2.patch

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

Great test! Thanks a lot!

r=me with the comments below fixed up.

::: devtools/client/debugger/test/mochitest/browser_dbg_variables-view-08.js
@@ +29,5 @@
> +
> +  let obj;
> +  [...scope].forEach(function([name, value]) {
> +    if(name === "obj") obj = value;
> +  });

Nit: for/of loop is more idiomatic than a forEach here. Also, devtools JS code always uses {} even if the condition is just a single statement.

  let obj;
  for (let [name, value] of scope) {
    if (name === "obj") {
      obj = value;
    }
  }

@@ +42,5 @@
> +  let count = values.length;
> +
> +  for (let [property, value] of obj) {
> +    let index = values.indexOf(property);
> +    if(index >= 0) {

Nit: space between "if" and "(" here and throughout

@@ +54,5 @@
> +    }
> +  }
> +  is(count, 0, "There are " + count + " missing properties");
> +
> +  debugger;

Was this left here from debugging? This shouldn't be here.

Comment 9

[Oriol Brufau [:Oriol]]

Attachment: This is a mistake. Ignore it.

(In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #8)
> Was this left here from debugging? This shouldn't be here.

This was left here when I copied from browser_dbg_variables-view-07.js, which was left there when I copied from browser_dbg_variables-view-06.js. It seemed strange, but I thought it was necessary for the tests.

I removed it from these too while I was at it.

Comment 10

[Oriol Brufau [:Oriol]]

Attachment: object-inspector-whitespace-property-v3.patch

Sorry, I attached the wrong patch.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Oriol from comment #9)
> Created attachment 8779917 [details] [diff] [review]
> revoked-proxy-crash-v3.patch
> 
> (In reply to Nick Fitzgerald [:fitzgen] [⏰PDT; UTC-7] from comment #8)
> > Was this left here from debugging? This shouldn't be here.
> 
> This was left here when I copied from browser_dbg_variables-view-07.js,
> which was left there when I copied from browser_dbg_variables-view-06.js. It
> seemed strange, but I thought it was necessary for the tests.

The `debugger` statement in the test case is certainly necessary; the `debugger` statement(s?) in test orchestration files shouldn't be there.

> I removed it from these too while I was at it.

Awesome, thank you very much.

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8779918 [details] [diff] [review]
object-inspector-whitespace-property-v3.patch

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

Looks great!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/eb1e3117f53c
When inspecting an object, do not trim property names except when displaying them. r=fitzgen

Comment 14

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

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

Comment 15

[Saddam Hossain]

I have reproduced this Bug on Nightly 31.0a1 (2014-04-16)  on Windows 10, 64 Bit!

The bug's fix is now verified on latest  Nightly 51.0a1

Nightly  51.0a1:
Build ID	   : 20160817030202
User Agent 	   : Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:51.0) Gecko/20100101 Firefox/51.0

 [bugday-20160817]