Fix webapp copy HTML crash due to destroyed borrowed PyProxy#358
Conversation
…ruction Agent-Logs-Url: https://github.com/scientific-python/repo-review/sessions/d8eb4e52-c4ff-4232-b88e-1dcfd29ed66f Co-authored-by: henryiii <[email protected]>
There was a problem hiding this comment.
Pull request overview
Fixes a Pyodide PyProxy lifetime issue in the React webapp where “Copy HTML” could crash due to a borrowed proxy becoming invalid after collected.destroy() runs.
Changes:
- Convert
(collected as any).familiesfrom a borrowed attribute proxy into an owned proxy by calling.copy()immediately.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| collected = collect_checks(pyodide, pyPackage); | ||
| const results_list = run_process(pyodide, pyPackage, collected) as any; | ||
| const families_dict = (collected as any).families; | ||
| const families_dict = (collected as any).families.copy(); |
There was a problem hiding this comment.
Calling .copy() here makes families_dict an owned PyProxy that won’t be auto-invalidated when collected.destroy() runs, but it also means it won’t be auto-freed. If an exception is thrown after this line (before pyFamilies is stored in state), or when repo/ref changes clear pyFamilies/pyChecks, the proxy can be leaked. Consider tracking families_dict/results_list in locals and explicitly .destroy() them on error paths, and destroying existing stored proxies when clearing them (e.g., in handleRepoChange/handleRefChange or unmount cleanup).
There was a problem hiding this comment.
Done in 16d8b41. Changes made:
- Added a
destroyProxy(proxy)helper that safely calls.destroy()and swallows errors, replacing all ad-hoc try/catch patterns. handleCompute:families_dict/results_listare nowletlocals declared before thetry. In thecatchblock they are destroyed if they were never saved to state. AftersetStatesucceeds, locals are nulled to prevent double-destroy. Old proxies are now destroyed viadestroyProxy(no more nested try/catch).handleRepoChange/handleRefChange: destroy existing stored proxies before setting them tonull.- Added
componentWillUnmountto destroy stored proxies when the component unmounts. finallyblock simplified to usedestroyProxyforcollectedandpyPackage.
…nmount Agent-Logs-Url: https://github.com/scientific-python/repo-review/sessions/caee2400-8054-44ff-82e1-6dc395ce1352 Co-authored-by: henryiii <[email protected]>
Clicking "copy HTML" in the webapp throws a
pyodide.ffi.JsExceptionbecausefamilies_dictis a borrowed attribute proxy ofcollected, andcollected.destroy()is called in thefinallyblock — automatically invalidatingfamilies_dictbefore it can be reused.Changes
.copy()on the borrowed proxy immediately after access, making it an independent ownedPyProxythat survivescollected's destruction.destroyProxy()helper that safely calls.destroy()on aPyProxy | nulland swallows errors, replacing repeated ad-hoc try/catch patterns.families_dictandresults_listasletlocals outside thetryblock so they can be destroyed on error paths if they were never saved to state. After a successfulsetState, locals are nulled to prevent double-destroy.handleRepoChange()andhandleRefChange()now destroy existing stored proxies before nullifying them, preventing leaks when the user changes repo or ref.componentWillUnmount()to destroy stored proxies when the component unmounts.