Ticket #1037 (closed: fixed)
Crash if accessing python variables assigned to qtiplot windows that have been closed
Reported by: | Russell Taylor | Owned by: | Russell Taylor |
---|---|---|---|
Priority: | major | Milestone: | Release 2.0 |
Component: | MantidPlot | Keywords: | |
Cc: | Blocked By: | ||
Blocking: | Tester: | Janik Zikovsky |
Description (last modified by Russell Taylor) (diff)
In the immediate window, if you do:
instrument_view = getInstrumentView("workspace-name")
show the window:
instrument_view.showWindow()
close it manually and then try to use the instrument_view variable again then you get a crash.
The same thing happens for a variable obtained from a graph window that's been closed (e.g. graph_spec = plotSpectrum("RawData", 0) ).
Change History
comment:2 Changed 10 years ago by Martyn Gigg
- Milestone changed from Iteration 24 to Iteration 25
This is due to python variable pointing to a stale pointer in the same way that the workspace variables used to when a workspace was deleted. That was solved by using a proxy object and returning that when a workspace was requested.
The problem with trying the same thing in MantidPlot is that this would have to be done for absolutely every wrapped MantidPlot function and would be unmaintainable with the current simple exposure using sip.
More investigation into sip and it's return types is needed.
comment:3 Changed 10 years ago by Nick Draper
- Milestone changed from Iteration 26 to Iteration 27
Bulk move of tickets to iteration 27, if your ticket is essential for Iteration 26 then move it back.
comment:4 Changed 10 years ago by Nick Draper
- Milestone changed from Iteration 27 to Iteration 28
Bulk move of tickets at the end of iteration 27
comment:5 Changed 9 years ago by Nick Draper
- Milestone changed from Iteration 28 to Iteration 29
Bulk move of tickets at the end of iteration 28
comment:6 Changed 9 years ago by Nick Draper
- Milestone changed from Iteration 29 to Iteration 30
"New" tickets moved at the code freeze of iteration 29
comment:7 Changed 9 years ago by Nick Draper
- Milestone changed from Iteration 30 to Iteration 31
Bulk move of tickets to iteration 31 at the iteration 30 code freeze
comment:8 Changed 9 years ago by Russell Taylor
- Owner changed from Martyn Gigg to Russell Taylor
- Status changed from new to accepted
- Component set to MantidPlot
I'm going to sort this out by using a proxy object much like the (original Framework python API) WorkspaceProxy. The proxy can listen for the 'destroyed' signal that all QObjects emit when they are deleted, and then set the wrapped reference to None. It's a pity that sip doesn't just do this - it seems like it could - but it doesn't. PySide isn't any better in this respect.
We will still have to intercept some functions: those that return a pointer to a QObject, and those that take one as an argument. Still this is a relatively small subset (perhaps 3 dozen or so) of the total number of methods.
comment:9 Changed 9 years ago by Russell Taylor
Split up mantidplot.py and move it into a directory. Re #1037.
mantidplot.py is starting to get big, and once all the proxy business for qti objects is added is will really swell. So the proxy objects will go in a separate file and everything is in a mantidplot directory that is imported. It shouldn't matter if an old mantidplot.py is lying around in the old place - python will prefer to import the directory. Fingers crossed that I've got the installation settings right.
Changeset: 4564706275d7d74ae5272cb89356fd4965979b46
comment:10 Changed 9 years ago by Russell Taylor
Try to copy directory successfully on Windows. Re #1037.
Changeset: a0eb4109530b5651df9bccbe56aa5e9bc1423cf1
comment:11 Changed 9 years ago by Stuart Campbell
refs #1037. Trying to fix tests.
Changeset: 2221796f38d808f005e2b0eb5ceefb10a5a0014f
comment:12 Changed 9 years ago by Russell Taylor
Try to fix Windows installer. Re #1037.
It's a real pain that it doesn't use CPack like the others.
Changeset: 147436fbcd143432c0b22913e15bae87a4b00893
comment:13 Changed 9 years ago by Russell Taylor
Last attempt for now.... Re #1037.
Changeset: 78c6444cf22b3062016643790d42ee2f22a32722
comment:14 Changed 9 years ago by Russell Taylor
Remove qtiplotrc.py startup file. Re #4335.
And qtiUtil.py as well. We hardly used anything in them. The only thing we did was the import of functions from the qti module into the global namespace. This will be replaced by proxy methods to prevent crashes if objects are deleted (see #1037).
Changeset: 87a32aceead0a4ee6f732fe45768253731926d19
comment:15 Changed 9 years ago by Russell Taylor
Use proxy objects when accessing MantidPlot objects from Python.
Protects against crashes if the windows are closed. Re #1037. Some methods, objects are not done yet. These are ones that are probably rarely, if ever, used and will be done tomorrow.
Changeset: 6e32d1dc935d126c2d64d4f6cc10b4b754662a5f
comment:16 Changed 9 years ago by Russell Taylor
Disconnect from the the 'destroyed' signal on deletion. Re #1037.
Prevents a segmentation fault on exiting MantidPlot. Also intercept a few more methods.
Changeset: d5b9a012f55c221ea0ee210112c522fd7ca175d5
comment:17 Changed 9 years ago by Russell Taylor
Change return type of Layer.addImage to void. Re #1037.
An ImageWidget is not a QObject, so our proxy method won't work. Ensure that pointers to ImageWidgets created in C++ cannot 'get out'. The addImage method can still be called with an ImageWidget constructed in python, or a filename.
Changeset: 77d19bb2810e39b06d2ec8d894a1023eaaf2c99c
comment:18 Changed 9 years ago by Russell Taylor
Complete wrapping of MantidPlot python objects with proxies. Re #1037.
In general, users should only ever get references to proxy objects, which are automatically set to None when the referenced window is closed.
Changeset: a722a49be1207255a97a23cdf94b80a1396f8e22
comment:19 Changed 9 years ago by Russell Taylor
Change import of mantidplot to mantiplotpy in applicable places.
Didn't realise it was used so much..... Re #1037.
Changeset: 432e2fdac4a93c0203110982b768eb4a50af2f2a
comment:20 Changed 9 years ago by Russell Taylor
- Status changed from accepted to verify
- Resolution set to fixed
This is a pretty big one to test!
Everything documented under:
http://www.mantidproject.org/MantidPlot:_Help#Python_Scripting_in_MantidPlot, http://www.mantidproject.org/MantidPlot:_Help#Plotting_and_Displaying_Workspaces and http://www.mantidproject.org/MantidPlot:_Help#Working_with_Instruments
should still work, and it should not be possible to crash MantidPlot by entering a python command that operates on a variable that relates to a window after that window has been closed manually.
comment:21 Changed 9 years ago by Janik Zikovsky
- Status changed from verify to verifying
- Tester set to Janik Zikovsky
comment:22 Changed 9 years ago by Janik Zikovsky
Refs #1037: Unit test for mantid plot python proxies
Exposed the close() method to python so that the windows close properly from python. Refs #4282: make sure the FloatingWindow is deleted when closed. Testing several proxy methods. Russell: Layer.legend() and Layer.grid() do not work for me!
Changeset: 2c795e49bff68c98d2f0df3b26d5f6e5f44bcbb2
comment:23 Changed 9 years ago by Russell Taylor
Make Grid and Legend work with proxy objects. Re #1037.
Changeset: b026dc53ed158b51a35dbbac1f9f918eccb0ac79
comment:24 Changed 9 years ago by Janik Zikovsky
Refs #1037 long set of MantidPlot proxies unit tests
... for closing objects that were open before Russell: Layer.spectrogram() does not seem to work even with the same fix as grid() etc.
Changeset: 10e23cf5f7aa8971bf01b00193dfe7bcc8bc1e27
comment:25 Changed 9 years ago by Janik Zikovsky
Refs #1037: folder deletion asks for confirmation, hanging up test
so commented out those tests for now
Changeset: bd5084d72a190fd8e9e4efe703f05795bf79aadc
comment:26 Changed 9 years ago by Russell Taylor
sip needs to be told that Spectrogram is a QObject. Re #1037.
Changeset: da23c140e1b559d4d0e64f4fbcf34fc4db420685
comment:27 Changed 9 years ago by Janik Zikovsky
- Status changed from verifying to closed
OK, I'm satisfied with the suite of unit tests that all return values that were exposed to python are safe for deletion.
comment:28 Changed 9 years ago by Russell Taylor
Revert "Change import of mantidplot to mantiplotpy in applicable places."
This reverts commit 432e2fdac4a93c0203110982b768eb4a50af2f2a (Re #1037) Back to mantidplot again. Re #4538.
Changeset: 7c9601a1b51e265adb26a07aa6b1cf64ffff8eab
comment:29 Changed 9 years ago by Russell Taylor
Revert "Change import of mantidplot to mantiplotpy in applicable places."
This reverts commit 432e2fdac4a93c0203110982b768eb4a50af2f2a (Re #1037) Back to mantidplot again. Re #4538.
Changeset: 7c9601a1b51e265adb26a07aa6b1cf64ffff8eab
comment:30 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 1885