Skip to content
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

Bizarre error possibly related to unreleased bindings. #74

Open
codewarrior0 opened this issue Sep 2, 2015 · 9 comments
Open

Bizarre error possibly related to unreleased bindings. #74

codewarrior0 opened this issue Sep 2, 2015 · 9 comments

Comments

@codewarrior0
Copy link

I wrote a bit about this in the other repo, but you can ignore all of those insane ramblings in favor of this condensed version:

(PySide 1.2.2, Python 2.7 for 64-bit Windows)

Rarely, I experience this completely inexplicable error nestled within my logging code, miles away from anything that uses a QGLContext:

 item = QtGui.QStandardItem(clean_msg(record.getMessage()))
>item.setForeground(QtGui.QBrush(fg))

AttributeError: 'PySide.QtOpenGL.QGLContext' object has no attribute 'setData'

What the fuck, right?

After reading through the baseobject and binding generation code for a bit, the best explanation I can come up with is that a binding is not being released from BindingManager. Huh?

BindingManager keeps track of the bindings that go from C++ objects (void *) to Python objects (SbkObject *). This serves several purposes, but only one of them seems to be of interest:

QStandardItem.setData is a virtual function. When I instantiate a QStandardItem on the Python side, the C++ side will create a CppWrapper - a C++ subclass of QStandardItem which will dispatch its virtual functions through the Python wrapper associated with it through BindingManager. This is meant to allow me to subclass QStandardItem in Python. Any C++ code that invokes the virtual function will call the overrides implemented on the Python side, thanks to the CppWrapper.

QStandardItem.setData is called by the implementation of QStandardItem.setForeground.

I think what is happening is that there is a binding in BindingManager, mapping the C++ QGLContext to the Python QGLContext, that is not being released. Because it is not released, a QStandardItem CppWrapper will later be created at the same memory address, and the BindingManager will then contain a mapping that says, roughly, CppWrapper<QStandardItem> -> SbkObject<QGLContext>

Thus, thanks to the CppWrapper, QStandardItem.setForeground will try to call QGLContext.setData, resulting in an AttributeError.

I won't speculate on why it isn't being released. I don't think I understand the problem well enough to say anything.

I haven't been able to create a minimal test case that reproduces the error. What I have been able to do is modify my application code in such a way that instead of getting the error rarely, I can now get it >90% of the times the app is launched.

@codewarrior0
Copy link
Author

I did a quick patch to the shiboken python module: I added a getBinding method that, given a C++ pointer, returns the Python object that is bound to that pointer via BindingManager.

I called shiboken.getCppPointer to get the C++ pointer for the faulty QStandardItem, and then passed that pointer to shiboken.getBinding to find out which Python wrapper it is bound to. This returns a <PySide.QtOpenGL.QGLContext object>, so I know I'm at least on the right track.

@codewarrior0
Copy link
Author

I read the docs again and found a tidbit that might be related:

9.2.3. Objects with virtual methods

A little bit of implementation details: virtual methods are supported by creating a C++ class, the shell, that inherits from the class with virtual methods, the native one, and override those methods to check if any derived class in Python also override it.

If the class has a virtual destructor (and C++ classes with virtual methods should have), this C++ instance invalidates the wrapper only when the overriden destructor is called.

One exception to this rule is when the object is created in C++, like in a factory method. This way the wrapped object is a C++ instance of the native class, not the shell one, and we cannot know when it is destroyed.

This is almost certainly relevant. The QGLContext is created in C++. When we call myGLWidget.context(), a Python wrapper is created, and a binding is registered with BindingManager. We have no way of knowing when the QGLContext is destroyed - especially because it is not a QObject, so we cannot connect its destroyed signal, which would be the easy way out.

I called getCppPointer on the QGLContext I got from getBinding, and I got back the same value I got when I called getCppPointer on the QStandardItem. Calling setData on the QStandardItem does not crash, so I presume that the C++ pointer is actually pointing to a QStandardItem. Thus, I have a QGLContext whose cptr points to a QStandardItem.

This QStandardItem is newly created. The QGLContext is old (created roughly 50,000 QStandardItems ago). The QGLContext's parent is a widget which I know is still alive; but calling .context() on the widget returns a different QGLContext.

I have a feeling that the purported QGLContext I get when querying the binding for the QStandardItem is a dead object. It was refcounted to zero by Python and freed a long time ago, but the memory at that address still looks like a Python wrapper for a QGLContext.

This feeling was only strengthened after this experience: I was entering Python commands into PDB. I had a list containing that QGLContext. I called gc.get_refererrs on some other object, and when I checked the list again, the QGLContext was replaced by a traceback object. So a new object must have been allocated at that address.

As a tentative fix, I tried changing BindingManager::BindingManagerPrivate::assignWrapper to unconditionally assign the binding, instead of assign the binding only if no binding is assigned for that cptr. With this change in place I can no longer reproduce the error.

@codewarrior0
Copy link
Author

Here's something else that's going on.

If I call myGLWidget.context(), and then add it as a child of another widget, and then call myGLWidget.context() again, I get a different Python QGLContext and a different CppPointer each time. After the second context call, a binding is still registered for the CppPointer of the object returned by the first context call.

After the second call, gc.get_referrers lists myGLWidget as a referrer for both QGLContext objects.

@codewarrior0
Copy link
Author

Okay, I think I'm getting somewhere. When I change the parent of the QGLWidget, it creates a new QGLContext for itself. The BindingManager still has the old QGLContext registered with its wrapper.

The QGLWidget's wrapper still has the old QGLContext wrapper in its parentInfo->children member. I verified this using shiboken.dump(myGLWidget). When I call context() a second time, it creates a new QGLContext wrapper and stores it in parentInfo->children. Also verified with shiboken.dump.

At some point later, the old QGLContext wrapper gets dealloc'd, which calls a series of functions that eventually calls _destroyParentInfo and removeParent to finally remove the context's wrapper from the widget's wrapper's parentInfo->children.

I haven't figured out exactly when the old QGLContext wrapper gets deallocated, but I know that in the time between the QGLWidget creating a new context for itself and the old context wrapper being deallocated, there is an invalid binding registered in BindingManager. Because of the invalid binding, when a C++ QStandardItem is created at the address formerly occupied by a QGLContext, stupid errors like the one at the beginning of the issue will occur.

My tentative fix to assignWrapper stops this behavior at the point where the new QStandardItem wrapper is created:

If a binding is already registered for a C++ pointer when assignWrapper is called, this means the C++ object that previously held that address was already deleted, but its Python wrapper is still alive. The Python wrapper is responsible for freeing the binding; if a new C++ object is allocated at the same address as the deleted object, then its binding will not be registered, so the old binding will not be released until the Python wrapper is freed.

By changing assignWrapper to override an existing binding, the new C++ object will immediately have a valid binding, and the old Python wrapper will not release any bindings when freed as its binding no longer exists.

@codewarrior0
Copy link
Author

At last, a minimal test case that reproduces the issue. (At least, it does for PySide 1.2.2, Python 2.7.10 64-bit on Windows 7...)

from PySide import QtCore, QtGui, QtOpenGL

def main():
    app = QtGui.QApplication([])
    glWidget = QtOpenGL.QGLWidget()
    context = glWidget.context()
    frame = QtGui.QFrame()

    # glWidget.setParent causes the widget to create a new GL context.
    glWidget.setParent(frame)

    # context now wraps a dead C++ object
    item = QtGui.QStandardItem()

    # item is now allocated at the address vacated by the old GL context.
    item.setForeground(QtGui.QColor(1, 1, 1))

    # item assumes the old binding created by context
    # Thus, setForeground calls setData, which is forwarded to context,
    # raising a nonsensical AttributeError.


if __name__ == '__main__':
    main()

@cbecker
Copy link

cbecker commented Sep 3, 2015

I am also getting a similar error, though on a different method:

AttributeError: 'PySide.QtOpenGL.QGLContext' object has no attribute 'moveToThread'

(this is on linux)

is there any quick fix that would enable us to call the desired methods?

@codewarrior0
Copy link
Author

The "quick fix" - if you can even call it that - is to ensure that you never call QGLWidget.context() before reparenting the widget.

The bug persists even for other objects, so the "quick fix" does not fix anything. The bug depends on this sequence of events:

  • Python gets a wrapper for a Qt object (the child) via a method on another Qt object (the parent).
  • The parent object deallocates the child object, but the Python wrapper for the child object is still alive.
  • Another Qt object is allocated at the memory address vacated by the child object.

@codewarrior0
Copy link
Author

I'd like to send a PR with a fix for this, but I'm honestly not sure which commit to base it on. master is way behind where it should be, the git submodule ref from pyside-setup is behind where it should be, the tag 1.2.2 (which would be checked out by setup.py) is not on any branch, and for some reason I have a clone of the old gitorious repo so my own master is way ahead of yours. I pushed a branch that looks like this and the number of commits that are in there is kind of alarming.

Or if you want, I'll just send a PR on the shiboken2 repo.

@empyrical
Copy link
Member

Sending the PR to shiboken2 would be best as that's where development is happening now

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

No branches or pull requests

3 participants