<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/128760/">https://git.reviewboard.kde.org/r/128760/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 26th, 2016, 4:22 a.m. UTC, <b>Anthony Fieroni</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="https://git.reviewboard.kde.org/r/128760/diff/1/?file=475410#file475410line44" style="color: black; font-weight: bold; text-decoration: underline;">kstyle/breezestyleplugin.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">44</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="p"><span class="hl">});</span></span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">44</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k"><span class="hl">if</span></span><span class="hl"> </span><span class="p"><span class="hl">(</span></span><span class="n"><span class="hl">inited</span></span><span class="p"><span class="hl">)</span></span><span class="hl"> </span><span class="p"><span class="hl">{</span></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This must be not (!inited).
However this is not proper fix. Correct and test patch in this way</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QPointer<Qstyle> style = new Style;</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Below unchanged, so when QPointer got delete it hold nullptr by itself and delete will be safe.</p></pre>
</blockquote>
<p>On August 26th, 2016, 9:50 a.m. UTC, <b>Peter Wu</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This does not work since the interface requires a QStyle pointer. If I use this, I guess the caller unwraps it into a raw pointer and the issue is still triggered.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Good point about <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">if (!inited)</code>, forgot to change this while renaming. I'll fix it for the next version. Do you know the root cause of the original issue that required the use of this? It is not documented by Qt.</p></pre>
</blockquote>
<p>On August 26th, 2016, 11:17 a.m. UTC, <b>Anthony Fieroni</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">QPointer track QObject and he knows when it's life or not, so issue must be fixed, at end</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">return style.data();</p></pre>
</blockquote>
<p>On August 26th, 2016, 2:34 p.m. UTC, <b>Peter Wu</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">style.data()</code> should not be needed because the cast operator of QPointer does this implicitly. (I did test it though and it makes no difference.)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Digging further, I am not convinced that this patch (or the previous code) is correct. While the result of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QApplication::style()</code> is ignored, its parent is set to a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QApplication</code> instance. So when a program exits normally, QApplication will be taking care of the QStyle instance. When <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">exit()</code> is invoked, the QApplication destructor does not seem to be called which probably led to the code in the first place.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I am tempted to remove the delete code completely, it seems a hack/workaround at the wrong level. Thoughts?</p></pre>
</blockquote>
<p>On August 26th, 2016, 2:44 p.m. UTC, <b>Anthony Fieroni</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You can test to remove manual deleting with style->setParent(this)</p></pre>
</blockquote>
<p>On August 26th, 2016, 3:30 p.m. UTC, <b>Peter Wu</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Setting <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">style->setParent(0)</code> in the two places (the initial <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QApplication::style()</code> call and the internal <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QProxyStyle</code> code from the testcase) prevents the crash on <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">exit()</code>. What about removing the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">delete style</code> code, what are the risks for that?</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I would vote for removing the "delete code".
I know it was introduce to fix a crash in e.g. kioclient5, but i don't think this is the right patch. It basically results in ambiguous ownership of the Style objects (QApp, and/or QStylePlugin), which is wrong (and creates all sort of problems. not only this one).
The issue of QApplication not being destroyed on exit(), which was the reason why this code was introduced, should be fixed upstream.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't think deleting only the "first" QStyle is a proper fix either. (does not solve the ambiguous ownership, and has no guarantee to fix the original issue either, basically you get the worse of both worlds :))</p></pre>
<br />
<p>- Hugo</p>
<br />
<p>On August 25th, 2016, 9:56 p.m. UTC, Peter Wu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da Costa.</div>
<div>By Peter Wu.</div>
<p style="color: grey;"><i>Updated Aug. 25, 2016, 9:56 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="https://bugs.kde.org/show_bug.cgi?id=356940">356940</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
breeze
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Do not delete all style instances which we create, restrict ourselves to
the first instance. I have no idea if the delete hack is still needed,
but let's keep it until it is certain that it is unneeded.</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Used "Testcase (with ASAN)" from bug https://bugs.kde.org/show_bug.cgi?id=356940. Run directly, no more crashes. Double-checked with a breakpoint on Breeze::StylePlugin::create that the second instance is called through QProxyStyle.</p></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>kstyle/breezestyleplugin.cpp <span style="color: grey">(083100e)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128760/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>