A problem with Event objects

komoto48g · January 12, 2022, 12:47pm

Hi there

This post is related to the open issue of Phoenix MouseWheelEvent is broken if MotionEvent is saved, where OP pointed out that there are similar bugs with matplotlib/WXAgg. I have already opened the issue in [Matplotlib issue tracker] and shown options for resolving it, but a few things are not clear to me.

This issue is caused by keeping a reference to the event object.
Here’s an example with a bit of modification of the original code from the [Phoenix issue tracker] that reproduces the problem.

Code Example (click to expand)

import sys
import wx

print('sys.version:', sys.version)
print('wx.__version__:', wx.__version__)

class MyFrame(wx.Frame):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        
        self.Bind(wx.EVT_MOTION, self.OnMouseMove)
        self.Bind(wx.EVT_MOUSEWHEEL, self.OnMouseWheel)
        self.last_event = None

    def OnMouseMove(self, evt):
        if evt.leftIsDown:
            print("Saving MouseMove event")
            self.last_event = evt

    def OnMouseWheel(self, evt):
        print("Wheel rotation: " + str(evt.GetWheelRotation()))
        test(self.last_event)

def test(obj):
    d = {}
    for key in dir(obj):
        try:
            print("key =", key)
            d[key] = getattr(obj, key) # Windows fatal exception: access violation
        except Exception as e:
            print(e)
            pass
    return d

app = wx.App(0)
frame = MyFrame(None)
frame.Show()
app.MainLoop()

To reproduce the bug and messages

Launch this script with debug options
$ python -Xdev -Xtracemalloc ‘script.py’
Press LBtn and move the mouse.
Then, the last wx.MouseEvent object is saved.
Scroll the mouse wheel to show outputs.
Then, a fault occurs when accessing evt.ClassInfo, and it crashes with Windows fatal exception: access violation.

I confirmed this for Windows 10 64 bit &
PY38 - wx 4.0.7 and wx 4.1.1,
PY39 - wx 4.1.1.

In my understanding, the reference of the event object should not be kept outside the event chain.
Therefore, the statement such as self.last_event = evt should be avoided. Otherwise, it will refer to the dead c++ object, and the program will crash easily. If you need event information, you need to save them as primitve types such as int, float, etc.

To avoid this, you can change it as follows.

-           self.last_event = evt
+           self.last_event = evt.__class__(evt)

This calls the implicit copy constructor, but it’s tricky and undocumented. I’m not sure it will work in future versions.

So, my questions are,

Is this issue likely to be improved in a future version?
Or should it be stated in wx.Event doc?
Can the above ‘tricky’ method be used in future versions?

Kind Regards

komoto48g · January 31, 2022, 5:46pm

I found that for matplotlib 3.4 and 3.5 with WXAgg backend, this issue can be ~~fixed~~ avoided with the following code; Place this in your program somewhere.

## patch for matplotlib 3.4/WXAgg
if 1:
    from matplotlib.backend_bases import Event
    
    def __init__(self, name, canvas, guiEvent=None):
        self.name = name
        self.canvas = canvas
        self.guiEvent = None
    
    Event.__init__ = __init__
    del __init__

Cheers

bsoher · March 4, 2022, 3:33pm

Just FYI, this issue also showed up in https://github.com/wxWidgets/Phoenix/issues/1707

It was pointed out by @swt2c that this problem seems to have been fixed in the latest wxPython snapshots. I installed wxPython-4.1.2a1.dev5259+d3bdb143 in a miniconda python 3.9 env on Win 10 and my test code seemed to behave properly.

komoto48g · March 5, 2022, 1:35pm

Thank you for the information, bsoher!

Confirmed that the mouse wheel problem of matplotlib/WXAgg has been fixed with
wxPython 4.1.2a1.dev5308+2258f215 msw (phoenix) wxWidgets 3.1.5

This issue also seems to be fixed.

wx 4.1.2a1 → OK
wx 4.1.1 → NG
wx 4.0.7 → OK

Unfortunately, the problem with my [Code Example] attached to this first post is still alive…

da-dada · March 8, 2022, 10:47pm

dir should not be used in coding, just for hacking!!! from the docs:

Because dir() is supplied primarily as a convenience for use at an interactive prompt, it tries to supply an interesting set of names more than it tries to supply a rigorously or consistently defined set of names, and its detailed behavior may change across releases. For example, metaclass attributes are not in the result list when the argument is a class.

in your Code Example there are even more strange keys

da-dada · March 8, 2022, 11:38pm

oh, I forgot, for what you want to achieve I guess you may use self.last_event = evt.Clone()

komoto48g · March 9, 2022, 3:40am

Thank you very much da-dada, I didn’t know wx.Event.Clone method!!

From the document wx.Event — wxPython Phoenix 4.1.2a1 documentation, I understand that evt.Clone() is equivalent to evt.__ class __ (evt) and I never have to write such tricky code. However, there is no mention of keeping a reference to the event object in the doc. What I want to achieve is to clarify the problem (bug or specification?).

komoto48g · March 9, 2022, 2:31pm

Now I’m ready to change the monkey-patch to the right PR to fix the matplotlib/WXAgg issue https://github.com/matplotlib/matplotlib/issues/22211.

if 1:
    from matplotlib.backend_bases import Event
    
    def __init__(self, name, canvas, guiEvent=None):
        self.name = name
        self.canvas = canvas
        self.guiEvent = guiEvent.Clone() if guiEvent else None
    
    Event.__init__ = __init__
    del __init__

Yet, the “official” guidance from the wx project would be nice.

da-dada · March 9, 2022, 11:51pm

well, I’m not an expert, but it looks as though the event given to the handler is always the same instance (comparing the python id, occasionally I do similarly for performance reasons & the amount of instances, although it can easily be a trap); so if one wants to save the last event a copy is unavoidable…

DietmarSchwertberger · March 10, 2022, 9:59pm

It seems that with the failing versions the python id is always the same for all MouseEvents.
For the other wx versions, the id is different for the EVT_MOTION and EVT_MOUSEWHEEL events.

da-dada · March 10, 2022, 10:55pm

well, I tried quite a few events across different frames and couldn’t find any pattern depending on type/object/category or any other property of an event (except a wx.lib.newevent always seems to use a new instance)
I suppose it’s some memory management of C++ or self-made wx which is highly needed for such an amount of instances (it reminds me of the __slot__ discussion in Python)

komoto48g · March 11, 2022, 3:55pm

Hi da-dada, DietmarSchwertberger,
Thank you for investigating the event problem. I didn’t notice that the MouseEvents’ IDs are the same.

As you said, the ID is sometimes the same for MouseEvents, but interestingly not always. I also noticed that each time scrolling the mouse wheel, different ID comes but it repeats as follows:

id=0x22974828ca0
id=0x22974828a60
id=0x229748289d0
:

In addition, the same ID is displayed not only for MouseEvent but also for other different type such as FocusEvent! Event objects seem to be recycled frequently, and as a result, data such as EventType will be corrupted. So, we should use its Clone to keep the forzen reference.

Attach here the modified wx.lib.eventwatcher ver.2 to monitor the event ID.

eventwatcher2_.py (14.8 KB)

Clipboard11

DietmarSchwertberger · March 11, 2022, 7:34pm

I can provoke similar behaviour in a shell: calls to wx._core.MouseEvent(10038) will result in a set of different ids.

When I add a call to gc.collect(), I always get the same id:

wx._core.MouseEvent(10038)
<wx._core.MouseEvent object at 0x1143FF80>
wx._core.MouseEvent(10038)
<wx._core.MouseEvent object at 0x1039F620>
wx._core.MouseEvent(10038)
<wx._core.MouseEvent object at 0x1143FF80>
wx._core.MouseEvent(10038)
<wx._core.MouseEvent object at 0x1039F620>
wx._core.MouseEvent(10038)
<wx._core.MouseEvent object at 0x1143FF80>
wx._core.MouseEvent(10038);gc.collect()
<wx._core.MouseEvent object at 0x1039F620>
0
wx._core.MouseEvent(10038);gc.collect()
<wx._core.MouseEvent object at 0x0CE25EE0>
0
wx._core.MouseEvent(10038);gc.collect()
<wx._core.MouseEvent object at 0x0CE25EE0>
0
wx._core.MouseEvent(10038);gc.collect()
<wx._core.MouseEvent object at 0x0CE25EE0>
0

In the matplotlib issue tracker I have suggested to explicitely delete the reference to the event. This should fix the issue and is certainly the right way to go.

DietmarSchwertberger · March 12, 2022, 1:06pm

wxWidgets is definitely destroying the event instances once they’re handled.

See e.g. lines 5997 ff: of https://github.com/wxWidgets/wxWidgets/blob/204db7e76a8359ad76366de273e146d8566ac3e2/src/msw/window.cpp#L5997

        wxMouseEvent event(wxEVT_ENTER_WINDOW);
        InitMouseEvent(event, x, y, flags);

        (void)HandleWindowEvent(event);
    }

When the variable event is going out of scope at the }, it’s destructor is being called.

So, no reference to an event should be kept.

da-dada · March 12, 2022, 10:36pm

what I wonder all the time is what drives one to keep a reference to an event outside the handler… (although the magic of SIP around the event is highly interesting)

komoto48g · March 14, 2022, 12:44pm

Thank you for your comments, Dietmar.
I am rethinking the PR for the matplotlib/WXAgg.

First, your suggestion: del event.guiEvent may be useful, but it doesn’t work once the user holds the event reference.

Next, my suggestion at previous post:

I don’t think this is too much overhead, but GC seems to be optimized on the one hand, but memory allocations such as motion events occur frequently on the other hand. So, I don’t think this idea is very good…

Finally, as you suggested, self.guiEvent = None, I think the best so far, but no documentation or evidence. It is also pointed out that "guiEvent is public API and would need a deprecation period."

For the first step (toward making guiEvent API deprecated), I’d like to propose adding the following statement to wx.Event (cf. wx.PaintDC):

This should normally be constructed as a temporary stack object;
Don't store a wx.Event object. Use Clone() to keep the frozen reference.

Btw where is the source of the HTML document?

komoto48g · March 14, 2022, 12:51pm

In my use case, I am using a copy of event <matplotlib.backend_bases.MouseEvent> (not wx.Event!) to refer to the original click-point (xdata, ydata) or the pixel point (x, y).

DietmarSchwertberger · March 14, 2022, 10:13pm

Thats bad design in many ways and resulting in code thats difficult to read and maintain.
Just store whats actually needed and give it a useful name.

The original problem in matplotlib probably was caused by not thinking about lifetime of referenced objects in Python. Objects on the stack will not immediately be destroyed when the reference goes out of scope.
Even with stepping through in the debugger I could not find how to ensure that this does not cause problems.

komoto48g · March 15, 2022, 2:00pm

Thank you very much for your advice!
That’s the code I made a long ago. I will look at it.

da-dada · March 15, 2022, 8:41pm

it looks as though Qt is also deleting the event like wx but throws an exception (wx is more what is called robust in the sense to get on with it)
tk looks more like your train of thought

(the snippet I’ve borrowed from matplotlib)

from matplotlib import pyplot as plt, use

# BACK_END = 'wxagg'
BACK_END = 'qtagg'
# BACK_END = 'tkagg'
use(BACK_END)

class LineBuilder:
    def __init__(self, line):
        self.line = line
        self.xs = list(line.get_xdata())
        self.ys = list(line.get_ydata())
        self.cid = line.figure.canvas.mpl_connect('button_press_event', self)
        self.last_event = None

    def __call__(self, event):
        # print(type(event.guiEvent))
        print('click', event)
        if self.last_event:
            if BACK_END == 'qtagg':
                print(f'{self.last_event.position()=}')
            else:
                print(f'{self.last_event.x=}')
        self.last_event = event.guiEvent
        if event.inaxes!=self.line.axes: return
        self.xs.append(event.xdata)
        self.ys.append(event.ydata)
        self.line.set_data(self.xs, self.ys)
        self.line.figure.canvas.draw()

fig, ax = plt.subplots()
ax.set_title('click to build line segments')
line, = ax.plot([0], [0])  # empty line
linebuilder = LineBuilder(line)

plt.show()