Closed Bug 1286757 Opened 8 years ago Closed 8 years ago

CallResolveOp shouldn't be invoked for global scope lets

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED INVALID

People

(Reporter: manishearth, Unassigned)

Details

Attachments

(1 file)

`let x;` in the global scope triggers CallResolveOp on the global. This is unnecessary; `let x` cannot touch or define properties on the global.


The root of the problem seems to be `js::CheckLexicalNameConflict` calling GetOwnPropertyDescriptor. Removing this seems to work.
Attached patch Proposed fixSplinter Review
This patch seems to work. `let x` no longer calls GetOwnPropertyDescriptor.

bz gave an interesting example, `<script>Object.defineProperty(this, "cat", { value: 5 });</script><script>let cat</script>` -- this is supposed to throw an error since let can't shadow. This patch preserves this behavior -- when `cat` has been defined by `defineProperty`, it still shows up in the `varObj->as<NativeObject>().lookup(cx, name)` check, so it still errors.


I'm very unsure about this fix, however. It seems to work but I'm really not familiar with this codebase.
Attachment #8770835 - Flags: feedback?(shu)
Your timing is impeccably unfortunate.  :-)  This code is being modified/rewritten/changed as part of bug 1263355, e.g.:

https://github.com/syg/gecko-dev/pull/11
https://github.com/syg/gecko-dev/pull/18
https://github.com/syg/gecko-dev/commit/48a85bf54ae4df492c2368846f46904cdac0b797

Any changes here will get overwritten -- or require merge-pain -- when that branch lands.

Given there doesn't *seem* to be any bad consequence to this behavior except inefficiency (at least with the current set of global resolve hooks), I'd say we should wait til that branch has landed to do anything here.  I'd guess that branch lands at the start of the next cycle or the one after.  One or two more cycles with extra resolve-hook-calling seems very tolerable.
Works for me.
Comment on attachment 8770835 [details] [diff] [review]
Proposed fix

This is wrong. Calling the resolve op is necessary on the global because of ES 15.1.11 step 5.c-d [1]. |let undefined|, for instance, is supposed to throw a SyntaxError because 'undefined' is a non-configurable property on the global, and this property along with many other builtins properties like NaN and Infinity that are also considered "restricted global properties" are lazily resolved via the global object's resolve hook.

Did you notice this causing performance problems?

[1] https://tc39.github.io/ecma262/#sec-globaldeclarationinstantiation
Attachment #8770835 - Flags: feedback?(shu) → feedback-
No performance problems that I notice, just seemed unnecessary. I hadn't thought of `let undefined` :)
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: