Review Request 127237: Fix crash in kmore tools on Windows

[Kåre Särs]

> 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.

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 ;)


> 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

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.

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


> 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.

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 ;)


- Kåre


-----------------------------------------------------------
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/73b9a665/attachment.html>