> 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é > >
