Page MenuHomePhabricator
Differential D2894

Fix memory leaks in qt/guiutil.cpp
AbandonedPublic
Actions

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 Passed
Unit
No Test Coverage
Build Status
Buildable 5663
Build 9388: Bitcoin ABC Buildbot (legacy)
Build 9387: 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
teamcity added a comment.Apr 30 2019, 22:57

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

Harbormaster failed remote builds in B5633: Diff 8317!Apr 30 2019, 23:11
teamcity added a comment.Apr 30 2019, 23:11

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing failed.

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
In D2894#68565, @jasonbcox wrote:

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.

teamcity added a comment.May 3 2019, 18:18

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

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

Typo.

teamcity added a comment.May 3 2019, 18:19

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

teamcity added a comment.May 3 2019, 18:31

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5663: Diff 8364.May 3 2019, 18:31
teamcity added a comment.May 3 2019, 18:32

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5664: Diff 8365.May 3 2019, 18:32
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
deadalnix added a subscriber: CCulianu.May 4 2019, 17:48

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 ↗(On Diff #8365)

Here this is retained.

855 ↗(On Diff #8365)

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

nakihito abandoned this revision.May 6 2019, 15:40
In D2894#69127, @deadalnix wrote:

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.

Revision Contents
Changeset List

PathSize
src/
qt/
26 lines

Diff 8364

View Options

src/qt/guiutil.cpp

Loading...