<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/4927/">http://svn.reviewboard.kde.org/r/4927/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On November 16th, 2010, 11:21 a.m., <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://svn.reviewboard.kde.org/r/4927/diff/4/?file=40257#file40257line499" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<td colspan="2"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">KWallet::Wallet* KCookieServer::wallet()</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">491</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">wallet</span> <span class="o">=</span> <span class="n">KWallet</span><span class="o">::</span><span class="n">Wallet</span><span class="o">::</span><span class="n">openWallet</span><span class="p">(</span><span class="n">KWallet</span><span class="o">::</span><span class="n">Wallet</span><span class="o">::</span><span class="n">NetworkWallet</span><span class="p">(),</span> <span class="mi">0</span><span class="p">,</span> <span class="n">KWallet</span><span class="o">::</span><span class="n">Wallet</span><span class="o">::</span><span class="n">Asynchronous</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Michael</p>
<br />
<p>On October 26th, 2010, 1:24 a.m., José Millán Soto wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By José Millán Soto.</div>
<p style="color: grey;"><i>Updated 2010-10-26 01:24:01</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/KDE/kdebase/apps/konqueror/settings/kio/kcookiespolicies.cpp <span style="color: grey">(1189829)</span></li>
<li>/trunk/KDE/kdebase/apps/konqueror/settings/kio/kcookiespoliciesdlg.ui <span style="color: grey">(1189829)</span></li>
<li>/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookiejar.h <span style="color: grey">(1189787)</span></li>
<li>/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookiejar.cpp <span style="color: grey">(1189787)</span></li>
<li>/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.h <span style="color: grey">(1189787)</span></li>
<li>/trunk/KDE/kdelibs/kioslave/http/kcookiejar/kcookieserver.cpp <span style="color: grey">(1189787)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/4927/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>