Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #5576 - Chakra should not redeclare "undefined" keyword #5734

Merged
merged 1 commit into from
Oct 2, 2018

Conversation

wyrichte
Copy link
Collaborator

Fixed by adding opcode OP_EnsureNoRootRedeclFunc that enforces the section of #sec-globaldeclarationinstantiation where #CanDeclareGlobalFunction is called. This enforces that when a global function that is not configurable, not writable, and not enumerable is overwritten a TypeError is thrown.

@wyrichte wyrichte force-pushed the build/wyrichte/5576_1 branch from 4ad22f4 to 5558c3d Compare October 1, 2018 18:16
@wyrichte wyrichte requested review from curtisman and aneeshdk October 1, 2018 18:17
@wyrichte wyrichte force-pushed the build/wyrichte/5576_1 branch from 5558c3d to 3a65aa7 Compare October 1, 2018 18:25
LSC_ERROR_MSG( 1015, ERRnoStrEnd , "Unterminated string constant")
LSC_ERROR_MSG( 1016, ERRnoCmtEnd , "Unterminated comment")
LSC_ERROR_MSG( 1017, ERRIdAfterLit , "Unexpected identifier after numeric literal")
LSC_ERROR_MSG(1004, ERRnoSemic, "Expected ';'")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Can you revert the formatting changes that were made to this file?

Copy link
Contributor

@dilijev dilijev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM except maybe tweaking that error message.

@dilijev dilijev added the Bytecode-Update This PR updates bytecode and will cause merge conflicts with other PRs with this label label Oct 1, 2018
@@ -313,6 +313,7 @@ namespace Js {
virtual BOOL InitPropertyInEval(PropertyId propertyId, Var value, PropertyOperationFlags flags = PropertyOperation_None, PropertyValueInfo* info = NULL);
virtual BOOL EnsureProperty(PropertyId propertyId);
virtual BOOL EnsureNoRedeclProperty(PropertyId propertyId);
virtual BOOL EnsureNoRedeclFunc(PropertyId propertyId);
Copy link
Contributor

@sethbrenith sethbrenith Oct 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that you're following the pattern of EnsureNoRedeclProperty, but I don't think the pattern should necessarily apply, because EnsureNoRedeclProperty is used in a much bigger variety of ways.

  • Why does this have a return value? As far as I can tell, the returned value is never used.
  • Why is this virtual and defined on RecyclableObject? The only call site knows that the type will be RootObjectBase; can't we just cast directly to that and call a non-virtual method?

@wyrichte wyrichte force-pushed the build/wyrichte/5576_1 branch 3 times, most recently from bb944f9 to 66e1d2d Compare October 2, 2018 20:52
…ord.

Fixed by adding opcode OP_EnsureNoRootRedeclFunc that enforces the section of #sec-globaldeclarationinstantiation where #CanDeclareGlobalFunction is called. This enforces that when a global function that is not configurable, not writable, and not enumerable is overwritten a TypeError is thrown.
@wyrichte wyrichte force-pushed the build/wyrichte/5576_1 branch from 66e1d2d to 8ecdb9e Compare October 2, 2018 21:39
@chakrabot chakrabot merged commit 8ecdb9e into chakra-core:master Oct 2, 2018
chakrabot pushed a commit that referenced this pull request Oct 2, 2018
…defined" keyword

Merge pull request #5734 from wyrichte:build/wyrichte/5576_1

Fixed by adding opcode OP_EnsureNoRootRedeclFunc that enforces the section of #sec-globaldeclarationinstantiation where #CanDeclareGlobalFunction is called. This enforces that when a global function that is not configurable, not writable, and not enumerable is overwritten a TypeError is thrown.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bytecode-Update This PR updates bytecode and will cause merge conflicts with other PRs with this label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants