这是indexloc提供的服务,不要输入任何密码
Skip to content

Fix leaking Ref in weakMapMemoize #729

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

Merged
merged 1 commit into from
Aug 11, 2024

Conversation

tomavos
Copy link
Contributor

@tomavos tomavos commented Jul 22, 2024

This PR contains the following changes:

  • Update the dereference logic in weakMapMemoize.ts to check if the lastResult is a Ref before dereferencing it.

Context:

The previous code could leak a WeakRef if the dereferenced value was undefined (which I think happens when the original object is forcefully garbage collected):
Let lastResult be a Ref to undefined:

const lastResultValue = lastResult?.deref?.() ?? lastResult
-> const lastResultValue = lastResult.deref() ?? lastResult
-> const lastResultValue = undefined ?? lastResult`
-> const lastResultValue = lastResult (= Ref{undefined})

With this new change, we will return the new value (even when the old garbage collected value would've passed the resultEqualityCheck and would've been memoized).

I chose the instanceof check because Ref is defined above as a WeakRef if it is available, else it is a StrongRef which is a locally defined class.

I tried to add a unit test which reproduced the issue by garbage collecting values in the cache so they would be Refs to undefined, but I didn't have any luck. I followed a similar strategy to this test by using a FinalizationRegistry. I also tried a test which unmounted a React component containing the original state. Please let me know if you have any ideas for this unit test :)

(aside) How we discovered the issue:

  • We had an array in a React component, which was then fed into a chain of selectors (of which the last returned a weakMapMemoized array).
  • This component got unmounted, and the cached weakMapMemoized result got garbage collected.
  • The next caller of this same selector produced an empty array result.
  • This passed our shallowEquals resultEqualityCheck (Object.keys(WeakRef{undefined}) == Object.keys([]) == [])
  • This leaked the WeakRef into the code which expected an array, and caused an exception.

Copy link

netlify bot commented Jul 22, 2024

Deploy Preview for reselect-docs canceled.

Name Link
🔨 Latest commit cea19b9
🔍 Latest deploy log https://app.netlify.com/sites/reselect-docs/deploys/669e7cb1ff5f5700083e6007

Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@tomavos tomavos changed the title Fix weakMapMemoize deref logic Fix leaking Ref in weakMapMemoize Jul 30, 2024
@markerikson markerikson merged commit 554eed9 into reduxjs:master Aug 11, 2024
22 of 23 checks passed
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.

2 participants