search

zxcalc / zxlive

A graphical tool for the ZX calculus
Apache License 2.0
49 stars 19 forks source link

crash due to accessing selection after `graph_scene` is deleted #218

Closed dlyongemallo closed 7 months ago

dlyongemallo commented 9 months ago

Steps to repro:

  1. start a derivation
  2. select any node
  3. close the app

Expected: app closes

Actual: App crashes in update_on_selection in rewrite_action.py, since a selection changed is triggered when the proof panel is closed, and the graph_scene is deleted when the app is closed, and the code in update_on_selection attempts to use the graph_scene.

It's not such a bad crash since the user is closing the app anyway, but it dumps a stack trace to the console.

Traceback (most recent call last):
  File "/home/davinci/workspace/zxlive/zxlive/rewrite_action.py", line 217, in update_on_selection
    selection, edges = self.proof_panel.parse_selection()
  File "/home/davinci/workspace/zxlive/zxlive/proof_panel.py", line 148, in parse_selection
    selection = list(self.graph_scene.selected_vertices)
  File "/home/davinci/workspace/zxlive/zxlive/graphscene.py", line 60, in selected_vertices
    return (it.v for it in self.selectedItems() if isinstance(it, VItem))
RuntimeError: Internal C++ object (GraphScene) already deleted.
valleyofblackpanther commented 8 months ago

Hi, @dlyongemallo @jvdwetering @RazinShaikh to address the crash upon closing the application due to the update_on_selection being called after the graph_scene has been deleted, we can add a check within the update_on_selection method to ensure that the graph_scene is still valid. For example:

def update_on_selection(self, g, selection, edges) -> None if self.proof_panel.graph_scene is None or getattr(self.proof_panel.graph_scene, 'g', None) is None:

Explanation:

In the update_on_selection method, before iterating over the child items or updating the rewrite, we check if self.proofpanel.graph_scene is None. Additionally we check if the g attribute of graph_scene is present and not None If either check fails, the method returns early without performing any updates. This could prevent the application from trying to access a deleted object. Of course, this approach hadn't been tested yet. But if there are any comments I would like to hear.

dlyongemallo commented 8 months ago

The best way to verify whether this works or not is to just try it. Please report back on the outcome.

valleyofblackpanther commented 8 months ago

Hi @RazinShaikh @dlyongemallo I have made the necessary modifications, this is the PR 230 I did not follow the previous method I mentioned, it didn't work out.