Page MenuHomePhabricator

Fix memory leaks in qt/guiutil.cpp
AbandonedPublic

Authored by nakihito on Apr 30 2019, 22:56.

Details

Reviewers
jasonbcox
Fabien
deadalnix
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Summary

on macOS:
listSnapshot was leaking in findStartupItemInList()
bitcoinAppUrl was leaking in [Get|Set]StartOnSystemStartup()

Backport of Core PR 11156
https://github.com/bitcoin/bitcoin/pull/11156/

Completes T622

Test Plan

make check
test_runner.py

Diff Detail

Repository
rABC Bitcoin ABC
Branch
PR11156
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 5664
Build 9390: Bitcoin ABC Buildbot
Build 9389: arc lint + arc unit

Event Timeline

nakihito created this revision.Apr 30 2019, 22:56
Owners added a reviewer: Restricted Owners Package.Apr 30 2019, 22:56
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 30 2019, 22:56
jasonbcox requested changes to this revision.Apr 30 2019, 23:14

Please investigate why the build is failing and why so much of the backport appears to already be in our codebase.

This revision now requires changes to proceed.Apr 30 2019, 23:14
nakihito added a comment.May 1 2019, 00:00

Please investigate why the build is failing and why so much of the backport appears to already be in our codebase.

https://reviews.bitcoinabc.org/rABCbabd6da6ec0313292cb4e5a0708125ead11a516d
Looks like someone already got to some of the changes. However, those changes diverge just enough from Core than a straight backport breaks things. Starting to read up about Core Foundation, now.

nakihito updated this revision to Diff 8364.May 3 2019, 18:16

Restored CFRetain() call.

deadalnix requested changes to this revision.May 4 2019, 17:42

Nos pace in PR12345 so this can be grepped or. Please edit description to reflect this.

This revision now requires changes to proceed.May 4 2019, 17:42

It does looks like @CCulianu 's patch and the patch you backport where intended to fix the same issue. The mix of both that you are proposing here makes zero sense.

src/qt/guiutil.cpp
830

Here this is retained.

855

To be almost immediately freed in what appears to be the only callsite.

nakihito abandoned this revision.May 6 2019, 15:40

It does looks like @CCulianu 's patch and the patch you backport where intended to fix the same issue. The mix of both that you are proposing here makes zero sense.

I believe you are correct. I did some more reading up on Core Foundation some more over the weekend and it looks like I misunderstood how it worked. I'll defer to @CCulianu's patch since I am unfamiliar to CF development.