<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="https://git.reviewboard.kde.org/r/116087/">https://git.reviewboard.kde.org/r/116087/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 1st, 2014, 4:17 p.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hmm, this might be equivalent, but all it means is that the orig code was wrong.
We should not make any memory allocations within the crash handler.
So we should instead store the startup id as a const char* somewhere and use strlcpy.
Unless we can make sure that the call to startupId() will always just return an existing QByteArray - but looking at the code, this doesn't seem to be the case.</pre>
</blockquote>
<p>On March 3rd, 2014, 4:02 p.m. UTC, <b>Alex Merry</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Hrm... I think we're actually querying the wrong place anyway. We should be asking the xcb platform plugin, since that's what actually handles the startup notifications, since the demise of KApplication (perhaps that part of KStartupInfo's functionality should be stripped out?).
Note that the platform plugin does always just return an existing QByteArray, so that should be fine. Which is good, because taking our own copy outside the handler would not work, as we would need to know when it was cleared.</pre>
</blockquote>
<p>On March 3rd, 2014, 4:21 p.m. UTC, <b>Alex Merry</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Actually, you don't get access to the QByteArray. You could do something like
QPlatformNativeInterface *platform = QGuiApplication::platformNativeInterface();
const char * startupId = reinterpret_cast<char *>(platform->nativeResourceForIntegration(QByteArrayLiteral("startupid")));
if (startupId && *startupId) {
argv[i++] = "--startupid";
argv[i++] = startupId;
}
Since we're in the crash handler, is it safe to assume that the rest of the application is stopped, and so that string will never disappear from underneath us?</pre>
</blockquote>
<p>On March 4th, 2014, 4:16 p.m. UTC, <b>Alexander Richardson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'll update the review to use this solution if someone else can confirm that this is safe. Even in multithreaded environments? Does the crash handler stop all threads?</pre>
</blockquote>
<p>On March 12th, 2014, 3:26 p.m. UTC, <b>Kevin Ottens</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think so yes, David could you confirm?</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'm not an expert on crash handling in a multithreaded application, but anyway, threads do not call into the xcb plugin (it's not threadsafe, and it's a GUI thing anyway). So yeah Alex Merry's suggestion sounds fine.</pre>
<br />
<p>- David</p>
<br />
<p>On March 12th, 2014, 3:26 p.m. UTC, Alexander Richardson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks and David Faure.</div>
<div>By Alexander Richardson.</div>
<p style="color: grey;"><i>Updated March 12, 2014, 3:26 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kcrash
</div>
<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;">remove usage of strlcpy
That copy was actually unnecessary, incrementing the refcount on
QByteArray and then calling constData() is enough</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </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;">Compiles</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>src/kcrash.cpp <span style="color: grey">(6c41022206130f186d52ddbb77afd58c429f368f)</span></li>
<li>src/strlcpy-fake.c <span style="color: grey">(9b2dc68c466908d11370ba0c4bbe8d315962da5d)</span></li>
<li>src/CMakeLists.txt <span style="color: grey">(d19b97f98e057d5537cf0eb6a8e1997d2a24cb1e)</span></li>
<li>src/config-strlcpy.h.cmake <span style="color: grey">(5d9163d7a60d89b9792afcdf498af6425a9038a8)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/116087/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>