Review Request: Using KWallet to store Cookies

[Michael Leupold]

> On 2010-11-16 11:21:29, David Faure wrote:
> > /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp, line 499
> > <http://svn.reviewboard.kde.org/r/4927/diff/4/?file=40257#file40257line499>
> >
> >     Same here. Why use Asynchronous if you make it sync anyway?
> >     
> >     I see you handle re-entrancy in this particular method, but I'm still worried about re-entrancy at a higher level.
> >     
> >     Please consider either
> >     1) reworking this to be fully async, or
> >     2) using the Sync API from KWallet, even though this will time out if the user takes too much time to enter his wallet password.
> >     
> >     To make it all async, you can solve the problem of synchronous DBus calls by using dbus transactions -- although one tends to hit the stupidly-low DBus timeout quite often this way, again if the user takes too much time to react, so this doesn't gain much.
> >     
> >     The "real" solution is probably to add an async API to kcookieserver, although even javascript requires synchronous access to cookies IIRC... so this might not work.
> >     
> >     You're hitting one of the most complex issues with Qt (and DBus) - once something low-level is async, everything on top of it needs to be async too, which can be pretty difficult.
> >     
> >     Anyway. I'd much rather see dbus calls time out (due to using the kwallet sync API), then the user can just hit reload after entering his wallet password, than seeing complex-to-debug crashes due to unexpected re-entrancy due to the use of QEventLoop.

Just a short hint regarding the synchronous use of KWallet:
While kwalletd still has synchronous D-Bus calls those aren't exposed in the public KWallet::Wallet API. To work around the timeouts (which in fact will be around 5-20s on most distributions) KWallet::Wallet itself uses the asynchronous D-Bus calls and a nested event-loop.

@David: I think the problem with synchronous KWallet calls timing out is that if a call to KWallet hits a timeout, the call to KCookieJar will have timed out already as it's kioslave->KCookieJar->KWallet. The only solution to this would be setting a lower timeout on the KCookieJar->KWallet call than there is on the kioslave->KCookieJar call - which seems like a really ugly hack.


- Michael


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/4927/#review8753
-----------------------------------------------------------


On 2010-10-26 01:24:01, José Millán Soto wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/4927/
> -----------------------------------------------------------
> 
> (Updated 2010-10-26 01:24:01)
> 
> 
> Review request for kdelibs.
> 
> 
> Summary
> -------
> 
> Currently cookies are stored in a plain text file. This patch allows KCookieJar to store the cookies securely using KWallet.
> 
> The main problem I had writing this patch was that when a web page is requested, KIO ask for the cookies to kded using dbus. In the first implementations that I wrote, if the user took too long to open the wallet, KIO received a dbus timeout.
> 
> To prevent this, if it takes more than 10 seconds to open the wallet, the web page will be requested without sending the cookies (or sending the available cookies if there's still the plain text cookie file). If the wallet is opened after that, the cookies stored in the wallet will be available since then.
> 
> Because of this, the feature is disabled by default.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/apps/konqueror/settings/kio/kcookiespolicies.cpp 1189829 
>   /trunk/KDE/kdebase/apps/konqueror/settings/kio/kcookiespoliciesdlg.ui 1189829 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookiejar.h 1189787 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookiejar.cpp 1189787 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.h 1189787 
>   /trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp 1189787 
> 
> Diff: http://svn.reviewboard.kde.org/r/4927/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> José
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20101116/4bcee7a4/attachment.htm>