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

Evaluate function declarations in catch statements properly #54

Merged
merged 1 commit into from
Jan 21, 2019

Conversation

robertsipka
Copy link
Contributor

@robertsipka robertsipka commented Jan 13, 2019

It is not a proper solution, however it is working. The createBinding function is not called for the GlobalEnvironment record in this test case (issue 26). I will investigate it why.

Fixes #26.

@robertsipka robertsipka changed the title Evaluate function declarations in catch statements properly (WIP) Evaluate function declarations in catch statements properly Jan 13, 2019
@robertsipka
Copy link
Contributor Author

robertsipka commented Jan 17, 2019

The following code caused the assert:

try { 
    throw ReferenceError('');
} catch ( x ) { 
    eval ( "function arguments (){};" );
}

The arguments is an array-like object which is accessible inside functions that contains the values of the arguments passed to that function.
When the ScriptParser computed the variables for the interpreted code block it erased the arguments identifier because it assumed that the eval code is part of a function.

This was not a problem if we ran the code inside a function:

function foo() {
    try { 
        throw ReferenceError('');
    } catch ( x ) { 
        eval ( "function arguments (){};" );
    }
}

However the arguments was also erased here, the engine created an arguments object for the function, so it appears again within the identifiers and we can overwrite it inside the eval code.

@robertsipka robertsipka changed the title (WIP) Evaluate function declarations in catch statements properly Evaluate function declarations in catch statements properly Jan 17, 2019
@akosthekiss
Copy link
Contributor

It'd be great if this PR could wait for #41 so that the failure-inducing test case could be added to the regression test suite.

Copy link
Contributor

@ksh8281 ksh8281 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@yichoi yichoi left a comment

Choose a reason for hiding this comment

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

LGTM

@yichoi yichoi merged commit 93c2de5 into Samsung:master Jan 21, 2019
@akosthekiss
Copy link
Contributor

Well, @robertsipka , could you please open a separate PR to add the test case to the regression suite that hasn't get landed here?

@robertsipka robertsipka deleted the fix_26 branch January 21, 2019 08:18
clover2123 referenced this pull request in clover2123/escargot Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants