Skip to content
This repository has been archived by the owner on Feb 14, 2020. It is now read-only.

Fix constant endowments #3

Closed
wants to merge 2 commits into from
Closed

Fix constant endowments #3

wants to merge 2 commits into from

Conversation

erights
Copy link
Member

@erights erights commented Oct 27, 2019

Fixes #2

As reported by @XmiliaH at #2 code such as

const Marker = "Marker";
new Evaluator().evaluate("bad", Object.create(null, {bad: {value: Marker}}))

fails to look up the value of "bad" on the endowment. This is due to conflicting assumptions in the code. Historically, we assumed that we were only optimizing optimizable own properties of the safeGlobal. Now that we're building optimizing code per evaluation, it makes sense to extend the optimization to include optimizable own properties of the endowments as well. The refactoring to extend this optimization missed a crucial step: passing the scopeProxy in as the object from which to extract the constant values, rather than passing in the safeGlobal.

This way of repairing the refactoring does cause a proxy trap per optimizable global per evaluation. If this proves expensive, we could pass both the safeGlobal and the endowments directly into the generated optimizer code, as long as we pass them in without introducing more names into scope. arguments[1] and arguments[2] would work.

Note that either way of completing the refactoring introduces a hazard. Neither the scopeProxy (in the first technique) nor the endowments object itself (in the second technique) may leak into user code. They both must be completely encapsulated by the shim. Making them available in the enclosing scope of the eval-ed code makes their encapsulation hard to verify. Such verification would need to reason about all the odd corners of with and sloppy-mode declarations.

@erights erights requested a review from jfparadis October 27, 2019 01:39
@jfparadis jfparadis closed this Feb 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants