Review Request 127795: [DataEngine] Fix memory leak and possible crash + detailed test

[Michael Pyne]

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



Note first off that I'm not a Plasma dev, so some of these comments are questions more than they are statements. I do see several issues though:

- It seems to me that there's a lot of changes in here which aren't actually necessary to fix the bug you're talking about.
- The bug description is extremely sparse. There is presumably a memory leak... how did you notice it? How did you isolate it? Did you find a particular bug and then fix that, or did you make changes here and there until you couldn't reproduce the bug anymore? We prefer bug fixes where the bug *and* the solution are well-understood. ;) But we rely on good descriptions of what the bug is and how it's now fixed (instead of 'see title').
- I believe the Plasma developers are on a separate group; you should edit the review request to add the 'plasma' group to ensure this is reviewed by developers with actual Plasma experience.

With all that said I do believe you've found at least one leak and closed it off, but it's hard to tell that from the patch and minimal description that's been submitted.


autotests/pluginloadertest.cpp (line 84)
<https://git.reviewboard.kde.org/r/127795/#comment64507>

    `engine != 0` can simply be `engine` in this bool context. Plus you should really prefer `Q_NULLPTR` to `0` in modern code anyways even if you wanted to check directly.



src/plasma/datacontainer.cpp (line 162)
<https://git.reviewboard.kde.org/r/127795/#comment64505>

    Although I've made a lot of questions regarding usage of Qt::UniqueConnection elsewhere, this one may be necessary since I don't see any other checks for whether this signal-slot connection already exists.



src/plasma/datacontainer.cpp (line 170)
<https://git.reviewboard.kde.org/r/127795/#comment64506>

    This and the next 3 Qt::UniqueConnection flags appear to be unneeded: by this point in the code path there have already been checks for these signal-slot connections and what appear to be correct calls to QObject::disconnect().



src/plasma/dataengine.cpp (line 269)
<https://git.reviewboard.kde.org/r/127795/#comment64504>

    Although for this instance of Qt::UniqueConnection, I can't prove it impossible that duplicates would occur, this code path does have a guard to avoid duplicate sources being created for this data engine. Are we sure these Qt::UniqueConnection flags are necessary, or is this experimentation?
    
    On that note, it might be a good idea to combine this code path with common portions DataEnginePrivate::source(QString,bool) in order to ensure there's one (and only one) correct way to add a source to a data engine. That would help for later code audits when trying to verify where (and when) QObject connections are created.



src/plasma/dataengine.cpp (line 416)
<https://git.reviewboard.kde.org/r/127795/#comment64500>

    This should be 1 instead of 0 given the changes you've made elsewhere (unless the existing value is a bug...).
    
    But, the entire set of changes to reference count handling appear to be unnecessary. Why the change?



src/plasma/dataengine.cpp (line 521)
<https://git.reviewboard.kde.org/r/127795/#comment64501>

    The DataContainer `s` has only just been created by this point in the code, so `Qt::UniqueConnection` should be meaningless. Likewise for the change 2 lines down.



src/plasma/dataengine.cpp (line 606)
<https://git.reviewboard.kde.org/r/127795/#comment64503>

    By definition the source here has been created for the first time in this code path. So Qt::UniqueConnection should be useless.



src/plasma/private/dataenginemanager.cpp 
<https://git.reviewboard.kde.org/r/127795/#comment64502>

    Is it safe to remove this, given that even the change  to DataEngineManager::loadEngine to ->ref all returned engines still doesn't ->ref the null engine?


- Michael Pyne


On April 30, 2016, 3:18 p.m., Anthony Fieroni wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127795/
> -----------------------------------------------------------
> 
> (Updated April 30, 2016, 3:18 p.m.)
> 
> 
> Review request for KDE Frameworks and Marco Martin.
> 
> 
> Repository: plasma-framework
> 
> 
> Description
> -------
> 
> ^^
> 
> 
> Diffs
> -----
> 
>   autotests/pluginloadertest.h 61fc963 
>   autotests/pluginloadertest.cpp 4f96780 
>   src/plasma/datacontainer.cpp ee067db 
>   src/plasma/dataengine.cpp f942926 
>   src/plasma/private/dataenginemanager.cpp 08e42fb 
> 
> Diff: https://git.reviewboard.kde.org/r/127795/diff/
> 
> 
> Testing
> -------
> 
> Test pass.
> 
> 
> Thanks,
> 
> Anthony Fieroni
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160501/83d8408a/attachment-0001.html>