Review Request 128016: [OS X] Prevent a crash in the IdealDockWidget's context menu

[Sven Brauch]

> On May 26, 2016, 8:56 a.m., Sven Brauch wrote:
> > sublime/idealdockwidget.cpp, line 99
> > <https://git.reviewboard.kde.org/r/128016/diff/1/?file=465464#file465464line99>
> >
> >     Don't do this. It just clutters the code, and you would have to fix it in about every second line of the application anyways, should it ever happen (it doesn't on linux).
> 
> René J.V. Bertin wrote:
>     I don't understand what you're implying here. Not doing this would either mean not using a pointer at all (= abandoning the patch), or not checking if the allocation succeeded. And that's not acceptable either. No matter how likely an application is to crash anyway if an allocation fails.
>     From what I see this is true also when using a QScopedPointer.
> 
> Sven Brauch wrote:
>     Nowhere in KDE code does anybody ever check if an allocation succeeds. IMO it's pedantic and useless. We are not writing life-critical software; if your system is out of memory, your IDE crashes, such is life. On Linux it doesn't do anything _anyways_ in the default kernel configuration, since allocations always succeed.
> 
> René J.V. Bertin wrote:
>     I just don't agree with that. If you really insist on not checking here, I'd propose that you remove the 2 lines in a separate commit, because I don't want to go on permanent record not doing things properly.
>     
>     Writing proper code is indeed pedantic. Is it useless? What's the percentage of security fixes that we would NOT see if everyone always checked against NULL before using a pointer? 
>     
>     > allocations always succeed.
>     
>     I'm not sure if I should be in awe for a kernel that apparently is able to provide more memory than I could ever afford to buy, or lose quite a bit of esteem for inciting lazy and sloppy programming practices...
> 
> Kai Uwe Broulik wrote:
>     If operator new fails it throws std::bad_alloc in which case your program will blow up anyway as we don't catch exceptions anywhere.
> 
> René J.V. Bertin wrote:
>     Tomaz posted a more detailed comment with the same gist on the ML (Tomaz: please consider replying via the website, it's twice now that I scanned in vain through the RR trying to find your contributions ;) )
>     
>     Anyway, that's more constructive, and apparently the Linux kernel has nothing to do with it. I clearly never absorbed the fact that `new` throws in case of failure, I guess I've never been sufficiently annoyed by that yet :)
>     
>     If the check is pointless I'll remove it.
>     
>     That doesn't mean I consider it pointless to check, or handle allocation exceptions. I just hope there are no situations in which a big allocation fails that could have been handled with a nice "sorry, can't do that right now" failure message rather than a downright abort which can evidently lead to data (code) loss.

> I clearly never absorbed the fact that new throws

Because it doesn't, unless you set vm.overcommit_memory to 2 by hand in sysctl. :D


- Sven


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


On May 26, 2016, 3:34 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128016/
> -----------------------------------------------------------
> 
> (Updated May 26, 2016, 3:34 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> OS X can be capricious when instances corresponding to a widget are deleted, if the class in question uses "native" ObjC SDKs behind the scenes. Pending events can in that case be (generated and) delivered to objects that were already deleted.
> According to the documentation, one should prefer to use `QObject::deleteLater()` rather than the regular, direct `delete` whether it be explicit or implicit.
> 
> I've long used a local patch that uses this approach in order to prevent a recurring crash after using the context menu of the "ideal dock widget". Somehow I never put it up for review here, apparently.
> 
> 
> Diffs
> -----
> 
>   sublime/idealdockwidget.cpp dae0ea2 
> 
> Diff: https://git.reviewboard.kde.org/r/128016/diff/
> 
> 
> Testing
> -------
> 
> Builds and permits reliable behaviour on both OS X and Linux.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20160526/be19ca60/attachment.html>