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 Rust values getting GC'd while still borrowed and not getting GC'd if created via. constructor #3940

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Commits on Apr 27, 2024

  1. Add a failing test

    Liamolucko committed Apr 27, 2024
    Configuration menu
    Copy the full SHA
    070a4a8 View commit details
    Browse the repository at this point in the history
  2. Force tests to run sequentially

    At first I tried fixing them so that they didn't need to (giving each of them
    their own `dropCount`), but running multiple GCs in parallel seems to be flaky.
    Liamolucko committed Apr 27, 2024
    Configuration menu
    Copy the full SHA
    16bfc43 View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    e6551c1 View commit details
    Browse the repository at this point in the history
  4. Add a failing test

    Liamolucko committed Apr 27, 2024
    Configuration menu
    Copy the full SHA
    cad34fc View commit details
    Browse the repository at this point in the history

Commits on Apr 28, 2024

  1. Fix exported Rust types being GC'd while still borrowed

    I also discovered and fixed an extra bug while working on this, which
    was that `LongRefFromWasmAbi` wasn't getting used for `self`: this bug
    didn't cause any problems before, because the only type that had a
    different `LongRefFromWasmAbi` impl than its `RefFromWasmAbi` impl was
    `JsValue` which can't be the type of `self`.
    
    It became a problem here because I made the new `LongRefFromWasmAbi`
    impl for exported Rust types clone the `Rc`, whereas the
    `RefFromWasmAbi` impl doesn't because garbage collection can't occur
    during the synchronous call that the value has to live for.
    
    I might actually change it so that both of the impls behave like the
    current `LongRefFromWasmAbi` impl, though: cloning an `Rc` isn't
    expensive and so having the second different impl just makes things more
    complicated for no good reason. I just left it in this commit as
    explanation for how I discovered the `LongRefFromWasmAbi` issue.
    Liamolucko committed Apr 28, 2024
    Configuration menu
    Copy the full SHA
    3bd0f3c View commit details
    Browse the repository at this point in the history
  2. Configuration menu
    Copy the full SHA
    7fdbe95 View commit details
    Browse the repository at this point in the history
  3. Configuration menu
    Copy the full SHA
    d919842 View commit details
    Browse the repository at this point in the history
  4. Get rid of outdated borrow_mut

    Now that borrowing a Rust value always clones its `Rc`, `Rc::into_inner`
    is a sufficient check that the value isn't borrowed.
    Liamolucko committed Apr 28, 2024
    Configuration menu
    Copy the full SHA
    45c4444 View commit details
    Browse the repository at this point in the history
  5. Get rid of needless mut

    For some reason I was getting errors before without it, but that seems
    to be fixed now. (It's probably something to do with having removed the
    `borrow_mut`, but that only takes `&self`, so I still don't get it.)
    Liamolucko committed Apr 28, 2024
    Configuration menu
    Copy the full SHA
    c72cbde View commit details
    Browse the repository at this point in the history
  6. Update reference tests

    Liamolucko committed Apr 28, 2024
    Configuration menu
    Copy the full SHA
    db0c540 View commit details
    Browse the repository at this point in the history
  7. Add changelog entry

    Liamolucko committed Apr 28, 2024
    Configuration menu
    Copy the full SHA
    f992a18 View commit details
    Browse the repository at this point in the history
  8. Update schema hash

    Liamolucko committed Apr 28, 2024
    Configuration menu
    Copy the full SHA
    fa78ca6 View commit details
    Browse the repository at this point in the history