Open Bug 1385198 Opened 8 years ago Updated 2 years ago

"values" defined in web console is interfering with lexical declaration

Categories

(DevTools :: Console, defect, P2)

Product:
DevTools
Component:
Console
Version:
53 Branch
Type:
defect

Tracking

(firefox56 wontfix, firefox57 fix-optional, firefox117 affected)

Status:
REOPENED
Milestone:
117 Branch
Tracking Flags:
Tracking Status
firefox56 --- wontfix
firefox57 --- fix-optional
firefox117 --- affected

People

(Reporter: w.doeringer, Assigned: nchevobbe)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1385198 - [devtools] Don't add webconsole helper bindings if variable exists. r=#devtools-reviewers.
48 bytes, text/x-phabricator-request
Details | Review
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/59.0.3071.115 Safari/537.36 Steps to reproduce: // let values = ['abab', 3, {name: 'pogo'}], iterator = values[Symbol.iterator](); let array = ['abab', 3, {name: 'pogo'}], iterator = array[Symbol.iterator](); // 1st version gives /* Exception: TypeError: values[Symbol.iterator] is not a function @Scratchpad/1:10:54 */ Actual results: see above Expected results: 1st version should be ok also
Where do you run the code? I'm seeing the issue only in web console. and looks like there's "values" function defined, and apparently it's interfering. > values.call([], {a:10, b:20}) Array [10, 20] guessing from the behavior, it's Object.values or similar. for now moving to Developler Tools component, but might be Core-JavaScript issue.
Component: Untriaged → Developer Tools: Console
Summary: Symbol.iterator → "values" defined in web console is interfering with lexical declaration
to be clear, `values` referred in `iterator = values[Symbol.iterator]()` returns the `value` function, instead of `values` declared there. > let values = ['abab', 3, {name: 'pogo'}], iterator = values; iterator.toString() "function () { [native code] }"
<!DOCTYPE html> <html xmlns="http://www.w3.org/1999/xhtml" xml:lang="de" lang="de"> <!-- Revision: 2017/07/29 - - - - - - - - - - - - - - - - - - - - - - - - - --> <!-- Runs ok in Firefox; copy code to Scratchpad and error is thrown on global:keys and global:values --> <!-- not on global:entries; local versions are ok --> <head> <meta http-equiv="content-type" content="text/html;charset=utf-8" /> <meta name="viewport" content="width=device-width,initial-scale=1"> <title>bugzilla</title> <!-- --> <script type="text/javascript"> { console.log('--- local: entries'); let entries = [1,2,3]; let itentries = entries[Symbol.iterator](); console.log(itentries === itentries[Symbol.iterator]()); } { console.log('--- local: keys'); let keys = [1,2,3]; let itkeys = keys[Symbol.iterator](); console.log(itkeys === itkeys[Symbol.iterator]()); } { console.log('--- local: values'); let values = [1,2,3]; let itvalues = values[Symbol.iterator](); console.log(itvalues === itvalues[Symbol.iterator]()); } console.log('--- global: entries'); let entries = [1,2,3]; let itentries = entries[Symbol.iterator](); console.log(itentries === itentries[Symbol.iterator]()); console.log('--- global: keys'); let keys = [1,2,3]; let itkeys = keys[Symbol.iterator](); console.log(itkeys === itkeys[Symbol.iterator]()); console.log('--- global: values'); let values = [1,2,3]; let itvalues = values[Symbol.iterator](); console.log(itvalues === itvalues[Symbol.iterator]()); </script> </head> <!-- --> <body> <p>All output to console!</p> </body> </html>
`keys` and `values` are defined as console input commands here : http://searchfox.org/mozilla-central/source/devtools/server/actors/utils/webconsole-utils.js#410-433. Not sure if this is helpful for people though. At least we should allow people to override it so they don't have unexpected results.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #4) > `keys` and `values` are defined as console input commands here : > http://searchfox.org/mozilla-central/source/devtools/server/actors/utils/ > webconsole-utils.js#410-433. > Not sure if this is helpful for people though. > At least we should allow people to override it so they don't have unexpected > results. Yes, the page variables should override the input helpers
Status: UNCONFIRMED → NEW
status-firefox56: --- → wontfix
Ever confirmed: true
Priority: -- → P2
Product: Firefox → DevTools
Severity: normal → S3
Duplicate of this bug: 1817593
See Also: → 1684884
Duplicate of this bug: 1684884
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Duplicate of this bug: 1841308
Attachment #9342208 - Attachment description: Bug 1385198 - [devtools] Remove keys and values console helpers. r=#devtools-reviewers. → Bug 1385198 - [devtools] Don't add webconsole helper bindings if variable exists. r=#devtools-reviewers.
Duplicate of this bug: 1673543

Comment on attachment 9342208 [details]
Bug 1385198 - [devtools] Don't add webconsole helper bindings if variable exists. r=#devtools-reviewers.

Revision D182717 was moved to bug 1517907. Setting attachment 9342208 [details] to obsolete.

Attachment #9342208 - Attachment is obsolete: true

I added a test in D182773 that demonstrates the current issue.
arai, do you know what could be done for avoiding this ?

Flags: needinfo?(arai.unmht)
Attachment #9342208 - Attachment is obsolete: false
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5f3bdce0e3a1 [devtools] Don't add webconsole helper bindings if variable exists. r=devtools-reviewers,ochameau.

https://hg.mozilla.org/mozilla-central/rev/5f3bdce0e3a1

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox117: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 117 Branch
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

executeInGlobalWithBindings evaluates the given code in the following environment chain:

global <- global lexical <- with (bindings)

So, it's something like the following, but without the block:

var bindings = { x: "x from bindings" };
with (bindings) {
  var x = 1; x;
}

Inside with, var x = 1 declaration puts the value into the bindings object,
and x gets the value from inner-most environment, which is with (bindings),
so it returns 1.

Then, the behavior is different with let and const, due to the corner case around with + lexical declaration.

In JS syntax, a lexical declaration is not allowed as with body, and it requires a block:

with ({}) let a = 10; // Syntax Error
with ({}) { let a = 10; } // OK

But given executeInGlobalWithBindings's behavior is equivalent to the following without the block, it behaves in unexpected way:

var bindings = { y: "y from bindings" };
with (bindings) {
  let y = 2; y;
}

If there's no enclosing block, let and const have effect on the global lexical environment.
So, let y = 2 adds a global lexical variable,
and y still gets the value from inner-most environment, which is with (bindings).
That results in returning "y from bindings".

So, I think we need to define a better/clearer behavior for the top-level let and const inside executeInGlobalWithBindings.

Flags: needinfo?(arai.unmht)
Depends on: 1841964

Thanks for the detailed comment arai 👍

Bug 1842701 has a WIP patch that changes the bindings behavior to always get shadowed by global vars and lexicals.
I want to make sure the assumption below is valid:

(bug 1842701 comment #0)

  • if the conflict happens, users will most likely expect syntactic variables in their code always shadows the extra bindings

The possibly problematic case happens if the variable is accessed before the assignment in the declaration is executed.
Minimal testcase that behaves differently is the following, evaluated in DevTools console:

console.log($); var $ = 10; console.log($);

The behavior difference is the following:

first console.log second console.log
current behavior the binding function 10
bug 1842701 patch undefined 10

In the current behavior, $ is written to and read from the with environemnt for the bindings, and the global variable is untouched.

With bug 1842701 patch, the binding's $ is shadowed by the var $ declaration, and $ is written to and read from the global object, and the bindings is untouched, and the first console.log reads the default undefined value.

Another possibly surprising testcase is the following:

console.log($); if (false) { var $ = 10; } console.log($);
first console.log second console.log
current behavior the binding function the binding function
bug 1842701 patch undefined undefined

Given var is hoisted to the top-level, even if the declaration is unreachable, the global variable exists and it shadows the bindings, and $ is read from the global object in the patches case.

These behavior is specific to var (and function for the 2nd case).
Lexical variables (let, const, class`) are unaffected because they don't allow accessing before initialization, and a block creates a new lexical scope.

:nchevobbe, can I have your input here about the above behavior?
do you think the behavior is acceptable? also, is there any other example that needs attention if bindings gets shadowed?

Flags: needinfo?(nchevobbe)

Thanks for working on this arai.

For both example you provided, your patch has the behavior I would expect.
More globally, as a user, I'd expect the evaluation to happen as if there wasn't any binding added by the console itself

One example that could be confusing with the current behavior is :

console.log($); { let $ = 10 }; console.log($)

both logs display binding function, which you might wonder where they come from.
With a "corrected" behavior, I would expect this to throw with a ReferenceError: $ is not defined
But I can see how this could be difficult, so it's not mandatory that we take care of this one.

Flags: needinfo?(nchevobbe)

Thank you :)

In the WIP patch, I treated the extra bindings like special global variables which is available only during executing the given code.

So, in the let example, given it's block-scoped local variable, it doesn't affect the global variable, and it will keep the current behavior that shows the bindings' value.

I'll look into hiding the binding when there's any conflicting global or local variable and see if it's light-weight enough that doesn't slow down the compilation.

Attachment #9342339 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: