1264780 - Null destructuring assignment in let declaration in console throws internal error

Status: VERIFIED FIXED

(DevTools :: Console, defect)

Description

[Oriol Brufau [:Oriol]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160414030247

Steps to reproduce:

Enter this in web console:
> let {bug} = null;


Actual results:

The web console does not show any result.

The browser console shows
> error occurred while processing 'evaluateJSAsync: TypeError: Debugger.Object.prototype.forceLexicalInitializationByName: expected string, got undefined
> Stack: WCA_evalWithDebugger@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:1308:15
> WCA_onEvaluateJS@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:873:20
> WCA_onEvaluateJSAsync@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/actors/webconsole.js:843:20
> DSC_onPacket@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/server/main.js:1643:15
> LocalDebuggerTransport.prototype.send/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/transport/transport.js:569:11
> exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
> exports.makeInfallible/<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/shared/ThreadSafeDevToolsUtils.js:101:14
> Line: 1308, column: 15


Expected results:

The web console should show
> TypeError: can't convert null to object

No internal error should be thrown.

Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2bab88e97af5f29f32861135af8481412ee50499&tochange=6104903f75c5f154cc6c5e8d89dbf0a279284e59

Comment 1

[Morgan Phillips [:mrrrgn]]

Reflect.parse is being used to find identifiers which need "forced initialization" - destructuring cases need to be added.

Comment 2

[Kohei Yoshino [:kohei]]

wrong component.

Comment 3

[Morgan Phillips [:mrrrgn]]

Attachment: forcedestruct.diff

This handles array and object destructuring.

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8741563 [details] [diff] [review]
forcedestruct.diff

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

I believe all of these cases would be fixed up by Bug 1257088 / Bug 1249175 (and this code removed), right?  But in the mean time this looks fine to me besides the comment about new variables names and destructuring assignment below.. Thanks for adding the extra test coverage.

::: devtools/server/actors/webconsole.js
@@ +1309,5 @@
> +          let identifiers = [];
> +          for (let decl of line.declarations) {
> +            switch (decl.id.type) {
> +              case "Identifier": {
> +

Nit: no need for extra blank line after these case statements.

Likewise, no need for brackets around the case bodies

@@ +1330,5 @@
> +              case "ObjectPattern": {
> +
> +                // let {bilbo, my} = {bilbo: "baggins", my: "precious"};
> +                for (let prop of decl.id.properties) {
> +                  if (prop.key.type == "Identifier") {

I think we want prop.value.type here instead, right?  So as to handle 'a' and 'b' here: `let { foo: a, bar: b } = asdf()`.

What would be the consequence of calling forceLexicalInitializationByName on the wrong value ('foo' in this case)?  I tested with a global 'foo' on the page and it didn't seem to change anything, but I'd like to know if this could cause any problems.

Comment 5

[Morgan Phillips [:mrrrgn]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Comment on attachment 8741563 [details] [diff] [review]
> forcedestruct.diff
> 
> Review of attachment 8741563 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I believe all of these cases would be fixed up by Bug 1257088 / Bug 1249175
> (and this code removed), right?  But in the mean time this looks fine to me
> besides the comment about new variables names and destructuring assignment
> below.. Thanks for adding the extra test coverage.
> 
> ::: devtools/server/actors/webconsole.js
> @@ +1309,5 @@
> > +          let identifiers = [];
> > +          for (let decl of line.declarations) {
> > +            switch (decl.id.type) {
> > +              case "Identifier": {
> > +
> 
> Nit: no need for extra blank line after these case statements.
> 
> Likewise, no need for brackets around the case bodies
> 
> @@ +1330,5 @@
> > +              case "ObjectPattern": {
> > +
> > +                // let {bilbo, my} = {bilbo: "baggins", my: "precious"};
> > +                for (let prop of decl.id.properties) {
> > +                  if (prop.key.type == "Identifier") {
> 
> I think we want prop.value.type here instead, right?  So as to handle 'a'
> and 'b' here: `let { foo: a, bar: b } = asdf()`.
> 
> What would be the consequence of calling forceLexicalInitializationByName on
> the wrong value ('foo' in this case)?  I tested with a global 'foo' on the
> page and it didn't seem to change anything, but I'd like to know if this
> could cause any problems.

Holy cow, good catch. Insofar as calling forceLexicalInitializationByName on the wrong values it will have no impact. The method will ignore anything which isn't in the 'initializing' state. Bindings which are already initialized or don't exist will have no impact.

Comment 6

[Morgan Phillips [:mrrrgn]]

Attachment: forcedestruct.diff

Attempted to address concerns from the review and added additional test cases to cover situations like: |let {blah: foo} = noexist();| |let {blah: foo=99} = noexist();| and |let {x=1} = null;|

Comment 7

[Brian Grinstead [:bgrins]]

Comment on attachment 8741912 [details] [diff] [review]
forcedestruct.diff

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

::: devtools/server/actors/webconsole.js
@@ +1309,5 @@
> +
> +          let identifiers = [];
> +          for (let decl of line.declarations) {
> +            switch (decl.id.type) {
> +              case "Identifier": {

Nit: no need for brackets around case statements

@@ +1332,5 @@
> +                // let {blah: foo}    = {blah: yabba()}
> +                // let {blah: foo=99} = {blah: yabba()}
> +                for (let prop of decl.id.properties) {
> +                  // key
> +                  if (prop.key.type == "Identifier")

Nit: brackets around this if block

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce90323cb7c

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ce90323cb7c

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Regression in 47, is this worth the risk to uplift to beta?

Comment 11

[Morgan Phillips [:mrrrgn]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #10)
> Regression in 47, is this worth the risk to uplift to beta?

Thinking on it for a bit, I would say yes. It could confuse developers. I'll request uplift.

Comment 12

[Morgan Phillips [:mrrrgn]]

Attachment: forcedestruct.diff

Approval Request Comment
[Feature/regressing bug #]: 1264780
[User impact if declined]: Some destructuring errors will not be displayed in the web console.
[Describe test coverage new/current, TreeHerder]: Tests are included in the patch.
[Risks and why]: None foreseeable, other than possible merge conflicts.
[String/UUID change made/needed]:

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8749497 [details] [diff] [review]
forcedestruct.diff

New regression in 47, let's uplift to Beta.

Comment 14

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

https://hg.mozilla.org/releases/mozilla-beta/rev/f08801dfeb2d

Comment 15

[Nazir Ahmed Sabbir [:NaSb]]

I've managed to reproduce this bug on Nightly 48.0a1 (2016-04-14) ;
(Build ID: 20160414030247) on Linux, 64 Bit by following the comment 0's instruction!

This Bug is now verified as fixed on Latest Firefox Beta 47.0b4 (Build ID: 20160509171155)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0

and also Latest Firefox Developer Edition 48.0a2 (2016-05-11) ; (Build ID: 20160511004106)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
OS: Linux 3.19.0-58-generic x86-64

Comment 16

[Mohammad Maruf Islam]

I have reproduced this bug with Firefox Nightly 48.0a1 (Build ID: 20160414030247) on 
Windows 8.1, 64-bit with the instructions from comment 0.

Verified as fixed with Firefox Beta 47.0b5 (Build ID: 20160512003946)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0

Verified as fixed with Firefox Developer edition 48.0a2 (Build ID: 20160513004028)
Mozilla/5.0 (Windows NT 6.3; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

As this bug is also verified on Linux (comment 15), I am marking this as verified!

[bugday-20160511]