Don't invoke qstring localized stuff in critical section
ClosedPublic

Authored by jpalecek on May 17 2020, 5:52 PM.

Details

Summary

Move the DrKonqi socket name generation out of the signal handler, since it can freeze while locking QTextCodec mutexes

Signed-off-by: Jiří Paleček <jpalecek@web.de>

Diff Detail

Repository
R285 KCrash
Branch
for-upstream
Lint
Lint OK
Unit
No Unit Test Coverage
jpalecek created this revision.May 17 2020, 5:52 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 17 2020, 5:52 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
jpalecek requested review of this revision.May 17 2020, 5:52 PM
jpalecek retitled this revision from t/dont-invoke-qstring-localized-stuff-in-critical-section to Don't invoke QString localized stuff in critical section.May 17 2020, 7:37 PM
jpalecek edited the summary of this revision. (Show Details)
jpalecek updated this revision to Diff 83136.May 24 2020, 1:24 AM
jpalecek retitled this revision from Don't invoke QString localized stuff in critical section to Don't invoke qstring localized stuff in critical section.
jpalecek edited the summary of this revision. (Show Details)

(edited the commit msg)

Makes sense; just two minor things.

src/kcrash.cpp
95

prepend static, it's only used in this file.

662

It would be more consistent with the other global vars to name it s_socketpath
(and outside the KCrash namespace)

jpalecek updated this revision to Diff 83213.Jun 4 2020, 8:00 PM

Renamed the socketpath variable to s_socketpath, and make it static as requested

dfaure added a comment.Jun 6 2020, 1:12 PM

The changes you mention don't appear in phabricator.

I suggest amending your commit locally (so there's only one) and then

arc diff HEAD~
jpalecek updated this revision to Diff 83231.Jun 6 2020, 1:42 PM

Really commit the last changes

dfaure accepted this revision.Jun 6 2020, 2:11 PM
This revision is now accepted and ready to land.Jun 6 2020, 2:11 PM

The changes you mention don't appear in phabricator.

I suggest amending your commit locally (so there's only one) and then

arc diff HEAD~

Just to clarify. I of course have it as one commit locally, but I have them more on a branch and develop and test them together. So when I was amending the individual commits I made an error in distributing the changes.

Then I don't use arc because it doesn't work, totally. It just reports some error about missing key and exits. From the network communication I found out this is caused by requests to the phabricator server missing the query parameters when reusing HTTP connection fails, however, I don't want to dig deeper into this as it is something between php and curl. Oddly enough it doesn't work on this repository only, other kde repositories work.

And please, if you accepted it, could you also commit it, with the other 2 revisions? I have no commit access.

This revision was automatically updated to reflect the committed changes.