Re: Review Request 127237: Fix crash in kmore tools on Windows

[Kåre Särs]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127237/
---

(Updated March 11, 2016, 12:43 p.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks and Gregor Mi.


Changes
---

Submitted with commit def01c5bb668a284d9db48c4c402adc8268b895a by Kåre Särs to 
branch master.


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: 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
> > 
> >
> > 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 ;)
> 
> Gregor Mi wrote:
> 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?

.xsession-errors/stderr probably get the qCritical() debug output and dbgviewer 
on windows is logging it if running.


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127237: Fix crash in kmore tools on Windows

[Gregor Mi]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127237/#review93309
---


Ship it!




- Gregor Mi


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127237: Fix crash in kmore tools on Windows

[Gregor Mi]

> On March 7, 2016, 9:29 p.m., Gregor Mi wrote:
> > src/kmoretools/kmoretoolsmenufactory.cpp, lines 128-129
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> 
>

___
Kde-frameworks-devel mailing list

Re: 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
> > 
> >
> > 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
> > 
> >
> > see my comment above

same as above ;)


> On March 7, 2016, 9:29 p.m., Gregor Mi wrote:
> > src/kmoretools/kmoretoolspresets.cpp, line 97
> > 
> >
> > 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
> > 
> >
> > 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127237: Fix crash in kmore tools on Windows

[Gregor Mi]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/127237/#review93270
---




src/kmoretools/kmoretoolsmenufactory.cpp (lines 128 - 129)


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.



src/kmoretools/kmoretoolsmenufactory.cpp (lines 237 - 238)


see my comment above



src/kmoretools/kmoretoolspresets.cpp (line 97)


Again, this should not happen here, so please add a comment that this is a 
fix for Windows.



src/kmoretools/kmoretoolspresets.cpp (lines 165 - 167)


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.


- Gregor Mi


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127237: Fix crash in kmore tools on Windows

[Kåre Särs]

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


Changes
---

It is not related to DBus (Just Windows ;)

Added comments and a nullptr check before adding a the pointer to the 
kmtServiceList


Summary (updated)
-

Fix crash in kmore tools on Windows


Repository: knewstuff


Description (updated)
---

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 (updated)
-

  src/kmoretools/kmoretoolsmenufactory.cpp 30f4d02 
  src/kmoretools/kmoretoolspresets.cpp 2405321 

Diff: https://git.reviewboard.kde.org/r/127237/diff/


Testing (updated)
---

Compiled and run on windows. No crashes in Kate when right-clicking files in 
the project plugin.


Thanks,

Kåre Särs

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel