Review Request 127237: Fix crash in kmore tools on Windows
Gregor Mi
codestruct at posteo.org
Tue Mar 8 19:39:58 UTC 2016
> On March 7, 2016, 9:29 p.m., Gregor Mi wrote:
> > src/kmoretools/kmoretoolsmenufactory.cpp, lines 128-129
> > <https://git.reviewboard.kde.org/r/127237/diff/2/?file=448481#file448481line128>
> >
> > Thanks for the comment. However, as long as we don't know the root cause for the null pointers, I would feel better if the comment states clearly that we don't know what is happening and that it is happening on Windows only.
>
> Kåre Särs wrote:
> This one is not needed at the moment (I did not get null pointers here), but I left it there just in case. I feel it is better to loose functionality than getting a crash ;)
Then it's ok. General question: is there a common log file where one could write a message in case there is nullptr which was catched?
> On March 7, 2016, 9:29 p.m., Gregor Mi wrote:
> > src/kmoretools/kmoretoolsmenufactory.cpp, lines 237-238
> > <https://git.reviewboard.kde.org/r/127237/diff/2/?file=448481#file448481line237>
> >
> > see my comment above
>
> Kåre Särs wrote:
> same as above ;)
:)
> On March 7, 2016, 9:29 p.m., Gregor Mi wrote:
> > src/kmoretools/kmoretoolspresets.cpp, line 97
> > <https://git.reviewboard.kde.org/r/127237/diff/2/?file=448482#file448482line97>
> >
> > Again, this should not happen here, so please add a comment that this is a fix for Windows.
>
> Kåre Särs wrote:
> This is the first place where we get the null pointers. KMoreTools::registerServiceByDesktopEntryName() has at least two code paths that return nullptr.
> kmoretools.cpp
> line 129: qCritical() << ... the kmt-desktopfile " << desktopEntryName << " is provided but no Exec line is specified ...
> line 130: return nullptr;
> and
> line 146: qCritical() << ... a kmt-desktopfile must be provided. Please fix. Return nullptr. ...
> line 147: return nullptr;
>
> These might be errors, but I would rather see that it does not crash even if a runtime file is missing. I suspect that it is the later that generates the nullptr.
> This is why I check the returned pointer before using it.
>
> QStandardPaths::GenericDataLocation does not return ../share/ on Windows and thus does not find the file
Please add these findings to your code comments. Then Ship it.
> On March 7, 2016, 9:29 p.m., Gregor Mi wrote:
> > src/kmoretools/kmoretoolspresets.cpp, lines 165-167
> > <https://git.reviewboard.kde.org/r/127237/diff/2/?file=448482#file448482line165>
> >
> > As above: as long as we don't know the reason for the nullpointers I would be happier if there was a comment.
> >
> > That said I don't know what the KDE policy is when dealing with this kind of problem. Just adding null checks seems to make the code more complicated to analyse later. You are a long-term committer, so your words have a high weight. On the other hand I would be interested in a second opinion.
>
> Kåre Särs wrote:
> KMoreToolsPresets::registerServiceByDesktopEntryName() has one code path that returns a direct nullptr and another that calls KMoreTools::registerServiceByDesktopEntryName() that also can return a nullptr. So I cannot be sure that the pointer isn't null. So check it and don't add nullptr ;)
>
> The problem is, with 99% certainty, the missing .desktop file, but IMO we should not crash even if a runtime file is missing ;)
I agree that a missing desktop file should not be reason for a runtime crash. I wonder if there is a good way to log those errors instead of silently do nothing.
- Gregor
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127237/#review93270
-----------------------------------------------------------
On March 7, 2016, 8:38 p.m., Kåre Särs wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127237/
> -----------------------------------------------------------
>
> (Updated March 7, 2016, 8:38 p.m.)
>
>
> Review request for KDE Frameworks and Gregor Mi.
>
>
> Repository: knewstuff
>
>
> Description
> -------
>
> On Windows we sometimes get null-pointers in stead of pointers to KMoreToolsService. This patch adds checks for null-pointers in those places that made Kate crash in the project plugin and adds a nullptr check before adding them to the list.
>
>
> Diffs
> -----
>
> src/kmoretools/kmoretoolsmenufactory.cpp 30f4d02
> src/kmoretools/kmoretoolspresets.cpp 2405321
>
> Diff: https://git.reviewboard.kde.org/r/127237/diff/
>
>
> Testing
> -------
>
> Compiled and run on windows. No crashes in Kate when right-clicking files in the project plugin.
>
>
> Thanks,
>
> Kåre Särs
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160308/8b535d61/attachment-0001.html>
More information about the Kde-frameworks-devel
mailing list