semlanik / qtprotobuf

Protobuf generator and bindings for Qt framework
https://semlanik.github.io/qtprotobuf
MIT License
169 stars 38 forks source link

Accessing Messages Qproperties from Qml cause GUI app to crash #272

Open Rightbputnow1 opened 2 years ago

Rightbputnow1 commented 2 years ago

A fix would be to set the object’s ownership to cpp in the QProperty accessor or in the type registration.

gmabey commented 2 years ago

@Rightbputnow1 Another way to completely sidestep those issues is to transform the message classes into reference counted objects, much like QString and QByteArray #180

semlanik commented 2 years ago

@Rightbputnow1 could you please provide the minimum example when this happens. In common cases you don't need to set an explicit value ownership when accessing the property. But of course there are exceptions, e.g. when the value is returned from the function. Also it's not clear to me what corrupts the shared pointer ref counter.

RBQt commented 2 years ago

Say you have the following sample message:

syntax="proto3";

package Crash;

message Sample { repeated Agregate agregate = 1; }

message Agregate { uint64 sample = 1; } Now agregate is generated as QList<QSharedPointer>

Accessing any QSharedPointer's data in the previous list from Qml trough a Q_INVOKABLE would blindside the QSharedPointer atomic ref count as the QML JS engine would take ownership of the data and destroy it as soon as the Q_INVOKABLE call return since its note referenced by any javascript named id. The QSharedPointer is now left with a floating data although indicating otherwise and would cause any further dereferencing of the QSharedPointer data to randomly crash the app (garbage collector behavior is not predictible, may instantly crash giving insight on the source of the crash, may crash further in the app's lifetime leaving the user clueless which happened to my colleagues :').

Best fix would be to implement a QML friendly cpp ownership enforcing QSharedPointer that does not hang its data to dry once accessed from the QML. Quickest fix has been proposed as a pull request, basically adding the cpp ownership setter in the message generation portion of code of the message generator.

gmabey commented 2 years ago

https://github.com/semlanik/qtprotobuf/issues/180

On Mon, Sep 26, 2022 at 8:49 AM caashableRamOoOo @.***> wrote:

Any composed Message or list of composed message are good example of a QPproperty that is generatec as a shared or a list of shared pointers causing this issue.

Basically, accessing a qsharedpointer data trought any Q_INVOKABLE from the qml gives ownership of the data to the qml js engine which destroys the data as soon as the Q_INVOKABLE call returns (nasty garbage collector ;-(). Since the garbage collector bypasses the qsharedpointer destructorr (default or custom one), the qsharedpointer refcount get blindsided and indicates an invalide copy count now..

I'ill propose a fix by the end of buisiness day :).

— Reply to this email directly, view it on GitHub https://github.com/semlanik/qtprotobuf/issues/272#issuecomment-1258156850, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVTOUBTDCPQGS2RYJYD7ELWAGZZDANCNFSM5UR7BWCQ . You are receiving this because you commented.Message ID: @.***>