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

Mark header of rooted ephemerons when tracing #3049

Merged
merged 5 commits into from
Jun 25, 2023
Merged

Conversation

jedel1043
Copy link
Member

A very subtle bug that was discovered by @tunz. Also added additional checks on the test to make sure the ephemerons are correctly cleaned up.

@jedel1043 jedel1043 added bug Something isn't working gc Issue related to garbage collection labels Jun 19, 2023
@jedel1043 jedel1043 added this to the v0.17.0 milestone Jun 19, 2023
@jedel1043 jedel1043 requested a review from a team June 19, 2023 09:33
@github-actions
Copy link

github-actions bot commented Jun 19, 2023

Test262 conformance changes

Test result main count PR count difference
Total 94,842 94,842 0
Passed 74,663 74,663 0
Ignored 18,812 18,812 0
Failed 1,367 1,367 0
Panics 0 0 0
Conformance 78.72% 78.72% 0.00%

@jedel1043 jedel1043 marked this pull request as draft June 19, 2023 09:47
@codecov
Copy link

codecov bot commented Jun 19, 2023

Codecov Report

Merging #3049 (8f207a8) into main (86726f1) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3049      +/-   ##
==========================================
+ Coverage   50.55%   50.59%   +0.03%     
==========================================
  Files         446      446              
  Lines       46071    46073       +2     
==========================================
+ Hits        23292    23309      +17     
+ Misses      22779    22764      -15     
Impacted Files Coverage Δ
boa_gc/src/internals/ephemeron_box.rs 84.50% <100.00%> (ø)
boa_gc/src/lib.rs 99.51% <100.00%> (-0.49%) ⬇️

... and 13 files with indirect coverage changes

@jedel1043 jedel1043 marked this pull request as ready for review June 19, 2023 09:56
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.

Looks good to me! just a small nitpick :)

Could be this be related to the sporadic weakmap panics in test262?

boa_gc/src/lib.rs Outdated Show resolved Hide resolved
@jedel1043
Copy link
Member Author

Could be this be related to the sporadic weakmap panics in test262?

Not really. I tried to run the minimized reproduction from #2732 (comment) but the engine still panics.

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.

Looks good to me! :)

@Razican
Copy link
Member

Razican commented Jun 20, 2023

Could we add a test for the bug?

@jedel1043 jedel1043 requested a review from a team June 20, 2023 19:15
@raskad raskad added this pull request to the merge queue Jun 25, 2023
Merged via the queue into main with commit 530fe97 Jun 25, 2023
@raskad raskad deleted the fix-eph-finalizer branch June 25, 2023 18:57
Razican pushed a commit that referenced this pull request Jun 26, 2023
* Mark header of rooted ephemerons when tracing

* Add additional assertions

* Use assert_eq instead of expect

* Apply review

* Add test for fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working gc Issue related to garbage collection
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants