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

Fix panic when calling function that mutates itself #667

Merged
merged 1 commit into from
Sep 2, 2020
Merged

Fix panic when calling function that mutates itself #667

merged 1 commit into from
Sep 2, 2020

Conversation

dvtkrlbs
Copy link

@dvtkrlbs dvtkrlbs commented Aug 30, 2020

Fixes #663

This commit fixes the panic when a function mutates itself. The issue happens since we borrow the object and when we call the statementList of the object we borrow_mut the object again to mutate it.
It changes the following:

  • This adds a RcStatementList to cheaply clone StatementList
  • It calls the run on the body of the function after dropping the first borrow by returning only the body after setting up the function local env

Thank you @HalidOdat for their help 😃

@codecov
Copy link

codecov bot commented Aug 30, 2020

Codecov Report

Merging #667 into master will increase coverage by 0.03%.
The diff coverage is 92.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #667      +/-   ##
==========================================
+ Coverage   73.14%   73.17%   +0.03%     
==========================================
  Files         192      193       +1     
  Lines       14005    14029      +24     
==========================================
+ Hits        10244    10266      +22     
- Misses       3761     3763       +2     
Impacted Files Coverage Δ
boa/src/builtins/function/mod.rs 79.77% <ø> (ø)
boa/src/syntax/ast/node/mod.rs 20.51% <ø> (ø)
boa/src/syntax/ast/node/statement_list.rs 35.00% <75.00%> (+10.00%) ⬆️
boa/src/builtins/object/gcobject.rs 73.91% <91.66%> (+0.03%) ⬆️
boa/src/builtins/function/tests.rs 100.00% <100.00%> (ø)
boa/src/exec/mod.rs 67.83% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be20b65...7951a9b. Read the comment docs.

@Lan2u Lan2u requested review from HalidOdat, Razican, Lan2u and jasonwilliams and removed request for HalidOdat August 30, 2020 09:33
boa/src/builtins/object/gcobject.rs Outdated Show resolved Hide resolved
boa/src/builtins/object/gcobject.rs Outdated Show resolved Hide resolved
boa/src/syntax/ast/node/statement_list.rs Show resolved Hide resolved
boa/src/syntax/ast/node/statement_list.rs Show resolved Hide resolved
boa/src/builtins/object/gcobject.rs Show resolved Hide resolved
boa/src/builtins/object/gcobject.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat added bug Something isn't working execution Issues or PRs related to code execution labels Aug 30, 2020
@HalidOdat HalidOdat added this to the v0.10.0 milestone Aug 30, 2020
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Also some tests showing that it does not panic any more and give the expected behaviour

@dvtkrlbs
Copy link
Author

I added the test case from the issue

@dvtkrlbs
Copy link
Author

dvtkrlbs commented Aug 30, 2020

Oh wait it should not return the edited value from the call statement. I have no idea why that happens. When we call a self mutating function somehow that mutated value leaks and gets returned from the function. It should not happen this way.

boa/src/builtins/object/gcobject.rs Outdated Show resolved Hide resolved
boa/src/builtins/function/tests.rs Outdated Show resolved Hide resolved
@HalidOdat
Copy link
Member

Oh wait it should not return the edited value from the call statement. I have no idea why that happens. When we call a self mutating function somehow that mutated value leaks and gets returned from the function. It should not happen this way.

That is strange. does it happen on master too. if so then report an issue. this bug does not seem related to this PR

@dvtkrlbs
Copy link
Author

dvtkrlbs commented Sep 1, 2020

>> function x() {}
undefined
>> function y() { x.y = 3 }
undefined
>> x.y
undefined
>> y()
3

happens on master too. so i will apply your suggestions and then this should be ready to merge since this issue is irrelevant to my pr

@dvtkrlbs
Copy link
Author

dvtkrlbs commented Sep 1, 2020

Opened issue #672

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

Looks good to me :) Thanks for the fix!

@Razican Razican merged commit 744ee9f into boa-dev:master Sep 2, 2020
@dvtkrlbs dvtkrlbs deleted the self-mutate-func branch September 2, 2020 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working execution Issues or PRs related to code execution
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calling a function that mutates itself causes borrow panic
3 participants