Skip to content

Commit

Permalink
fix(isolated_declarations): fix potential memory leak (#6622)
Browse files Browse the repository at this point in the history
`Scope` contains 2 x `FxHashMap`s, which own data outside of the arena. So this data will not be dropped when the allocator is dropped.

The scope for this becoming a memory leak in practice is limited for 2 reasons:

1. All `Scope`s except the root one are popped from the stack by the end of traversal. That last scope's hashmaps are always empty, unless there are unresolved references (references to globals).

2. `oxc_allocator::Vec` is currently `Drop`.

However, `oxc_allocator::Vec` will cease to be `Drop` in future, at which point this would become a real memory leak.

Additionally, it doesn't make sense to store temporary data in the arena, as the arena is intended to hold data that needs to live as long as the AST, which temporary data doesn't.
  • Loading branch information
overlookmotel committed Oct 16, 2024
1 parent 3af0840 commit 2673397
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 8 deletions.
2 changes: 1 addition & 1 deletion crates/oxc_isolated_declarations/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ impl<'a> IsolatedDeclarations<'a> {
ast: AstBuilder::new(allocator),
strip_internal,
internal_annotations: FxHashSet::default(),
scope: ScopeTree::new(allocator),
scope: ScopeTree::new(),
errors: RefCell::new(vec![]),
}
}
Expand Down
12 changes: 5 additions & 7 deletions crates/oxc_isolated_declarations/src/scope.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
use std::cell::Cell;

use bitflags::bitflags;
use oxc_allocator::{Allocator, Vec};
use rustc_hash::FxHashMap;

#[allow(clippy::wildcard_imports)]
use oxc_ast::ast::*;
use oxc_ast::AstBuilder;
#[allow(clippy::wildcard_imports)]
use oxc_ast::{visit::walk::*, Visit};
use oxc_span::Atom;
use oxc_syntax::scope::{ScopeFlags, ScopeId};
use rustc_hash::FxHashMap;

bitflags! {
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
Expand Down Expand Up @@ -37,13 +36,12 @@ impl<'a> Scope<'a> {
/// Linear tree of declaration scopes.
#[derive(Debug)]
pub struct ScopeTree<'a> {
levels: Vec<'a, Scope<'a>>,
levels: Vec<Scope<'a>>,
}

impl<'a> ScopeTree<'a> {
pub fn new(allocator: &'a Allocator) -> Self {
let ast = AstBuilder::new(allocator);
let levels = ast.vec1(Scope::new(ScopeFlags::Top));
pub fn new() -> Self {
let levels = vec![Scope::new(ScopeFlags::Top)];
Self { levels }
}

Expand Down

0 comments on commit 2673397

Please sign in to comment.