Bug in QWebEngineProfile::setNotificationPresenter

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Bug in QWebEngineProfile::setNotificationPresenter

Florian Bruhin
Hi,

When running the attached example and clicking "authorize", then "show" 2-3
times, either a segfault or an exception like this will happen:

  TypeError: 'QWebEngineNotification' object is not callable

As a workaround, show_notification_2 can be used instead, which re-registers
the callback and makes things work fine.

Looking at the implementation of setNotificationPresenter in
qwebengineprofile.sip, it does "Py_DECREF(a0);", inside its wrapper, with a0
being the passed function. That seems to be wrong, as the function gets called
multiple times with multiple notifications.

Compare that to e.g. setCookieFilter in qwebenginecookiestore.sip where the
callback isn't DECREF'd.

Florian

--
[hidden email] (Mail/XMPP) | https://www.qutebrowser.org 
       https://bruhin.software/ | https://github.com/sponsors/The-Compiler/
       GPG: 916E B0C8 FD55 A072 | https://the-compiler.org/pubkey.asc
             I love long mails! | https://email.is-not-s.ms/

notifications.py (983 bytes) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Bug in QWebEngineProfile::setNotificationPresenter

Phil Thompson-5
On 21/05/2020 10:42, Florian Bruhin wrote:

> Hi,
>
> When running the attached example and clicking "authorize", then "show"
> 2-3
> times, either a segfault or an exception like this will happen:
>
>   TypeError: 'QWebEngineNotification' object is not callable
>
> As a workaround, show_notification_2 can be used instead, which
> re-registers
> the callback and makes things work fine.
>
> Looking at the implementation of setNotificationPresenter in
> qwebengineprofile.sip, it does "Py_DECREF(a0);", inside its wrapper,
> with a0
> being the passed function. That seems to be wrong, as the function gets
> called
> multiple times with multiple notifications.
>
> Compare that to e.g. setCookieFilter in qwebenginecookiestore.sip where
> the
> callback isn't DECREF'd.

Should be fixed in tonight's snapshot.

Thanks,
Phil
Reply | Threaded
Open this post in threaded view
|

Re: Bug in QWebEngineProfile::setNotificationPresenter

Florian Bruhin
On Fri, May 22, 2020 at 09:35:55AM +0100, Phil Thompson wrote:

> On 21/05/2020 10:42, Florian Bruhin wrote:
> > Hi,
> >
> > When running the attached example and clicking "authorize", then "show"
> > 2-3
> > times, either a segfault or an exception like this will happen:
> >
> >   TypeError: 'QWebEngineNotification' object is not callable
> >
> > As a workaround, show_notification_2 can be used instead, which
> > re-registers
> > the callback and makes things work fine.
> >
> > Looking at the implementation of setNotificationPresenter in
> > qwebengineprofile.sip, it does "Py_DECREF(a0);", inside its wrapper,
> > with a0
> > being the passed function. That seems to be wrong, as the function gets
> > called
> > multiple times with multiple notifications.
> >
> > Compare that to e.g. setCookieFilter in qwebenginecookiestore.sip where
> > the
> > callback isn't DECREF'd.
>
> Should be fixed in tonight's snapshot.
Thanks! There seems to be a similiar issue surrounding the lifetime of the
QWebEngineNotifcation object, though.

Based on Qt's example, it should be possible to store the notification in the
presenter and later e.g. call .click() or .close() on it:

https://github.com/qt/qtwebengine/blob/v5.14.2/examples/webenginewidgets/notifications/notificationpopup.h#L132

However, when doing the same in PyQt (like simulated in the attached example),
it looks like PyQt already deleted the underlying QWebEngineNotification object
(probably because it uses an unique_ptr in its callback?):

RuntimeError: wrapped C/C++ object of type QWebEngineNotification has been deleted

Florian

--
[hidden email] (Mail/XMPP) | https://www.qutebrowser.org 
       https://bruhin.software/ | https://github.com/sponsors/The-Compiler/
       GPG: 916E B0C8 FD55 A072 | https://the-compiler.org/pubkey.asc
             I love long mails! | https://email.is-not-s.ms/

notifications2.py (1K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Bug in QWebEngineProfile::setNotificationPresenter

Phil Thompson-5
On 22/05/2020 11:03, Florian Bruhin wrote:

> On Fri, May 22, 2020 at 09:35:55AM +0100, Phil Thompson wrote:
>> On 21/05/2020 10:42, Florian Bruhin wrote:
>> > Hi,
>> >
>> > When running the attached example and clicking "authorize", then "show"
>> > 2-3
>> > times, either a segfault or an exception like this will happen:
>> >
>> >   TypeError: 'QWebEngineNotification' object is not callable
>> >
>> > As a workaround, show_notification_2 can be used instead, which
>> > re-registers
>> > the callback and makes things work fine.
>> >
>> > Looking at the implementation of setNotificationPresenter in
>> > qwebengineprofile.sip, it does "Py_DECREF(a0);", inside its wrapper,
>> > with a0
>> > being the passed function. That seems to be wrong, as the function gets
>> > called
>> > multiple times with multiple notifications.
>> >
>> > Compare that to e.g. setCookieFilter in qwebenginecookiestore.sip where
>> > the
>> > callback isn't DECREF'd.
>>
>> Should be fixed in tonight's snapshot.
>
> Thanks! There seems to be a similiar issue surrounding the lifetime of
> the
> QWebEngineNotifcation object, though.
>
> Based on Qt's example, it should be possible to store the notification
> in the
> presenter and later e.g. call .click() or .close() on it:
>
> https://github.com/qt/qtwebengine/blob/v5.14.2/examples/webenginewidgets/notifications/notificationpopup.h#L132
>
> However, when doing the same in PyQt (like simulated in the attached
> example),
> it looks like PyQt already deleted the underlying
> QWebEngineNotification object
> (probably because it uses an unique_ptr in its callback?):
>
> RuntimeError: wrapped C/C++ object of type QWebEngineNotification has
> been deleted
>
> Florian

I've changed it so that it takes over the ownership of the
QWebEngineNotication object.

Phil