Qt: remove macOS launch-at-startup when compiled with > macOS 10.11, fix memory missmanagement #15208

jonasschnelli · 2019-01-19T08:04:40Z

The launch-at-startup API Bitcoin Core uses on macOS where removed in macOS 10.11 leading to a segmentation-fault due to the weak-linking when not actively compiled against SDK 10.11 (-mmacosx-version-min=10.11)

This PR removes the launch-at-startup feature on macOS when compiled with macOS min version > 10.11 (the default is always the macOS version you compile on).

The depends built binaries (Gitian) are not affected since we are building with min macOS 10.10.

Users self compiling on macOS > 10.11 can re-enable the feature by compiling with min version <= 10.11 (CXXFLAGS="-mmacosx-version-min=10.11" CFLAGS="-mmacosx-version-min=10.11" ./configure)

Isn't there a new API from Apple?
Yes, there is.
It will require to create a helper application which needs to be embedded in the .app folder (needs code signing as well). Developers willing to go down that rabbit hole are welcome.

Fixes #15142

hebasto · 2019-01-19T08:39:56Z

utACK 8441c90af88fb0f08762cf56e508803ed4b47d4c modulo typo

hebasto · 2019-01-19T08:41:55Z

doc/release-notes.md

in --> is ?

laanwj · 2019-01-19T12:27:32Z

utACK

fanquake · 2019-01-20T03:30:15Z

Compiling 8441c90 on macOS 10.14.2:

./autogen.sh && ./configure && make check -j6

The option is disabled.

However, when passing -mmacosx-version-min=10.11 to ./configure:

./autogen.sh
CXXFLAGS="-mmacosx-version-min=10.11" CFLAGS="-mmacosx-version-min=10.11" ./configure
make check -j6
lldb src/qt/bitcoin-qt

I'm still seeing the same segfault behaviour as #15142:

Current executable set to 'src/qt/bitcoin-qt' (x86_64).
(lldb) run
Process 20678 launched: '/Users/michael/github/bitcoin/src/qt/bitcoin-qt' (x86_64)
2019-01-20 11:09:45.261118+0800 bitcoin-qt[20678:2180338] MessageTracer: Falling back to default whitelist
Process 20678 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x20)
    frame #0: 0x00007fff72cb58bf libobjc.A.dylib`objc_retain + 31
libobjc.A.dylib`objc_retain:
->  0x7fff72cb58bf <+31>: testb  $0x4, 0x20(%rcx)
    0x7fff72cb58c3 <+35>: je     0x7fff72cb58f6            ; <+86>
    0x7fff72cb58c5 <+37>: testb  $0x1, %al
    0x7fff72cb58c7 <+39>: je     0x7fff72cb58e9            ; <+73>
Target 0: (bitcoin-qt) stopped.
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x20)
  * frame #0: 0x00007fff72cb58bf libobjc.A.dylib`objc_retain + 31
    frame #1: 0x00007fff484b8f18 SharedFileList`-[SFLLoginItemList removeItem:error:] + 35
    frame #2: 0x00007fff484bf3c7 SharedFileList`LSSharedFileListItemRemove + 127
    frame #3: 0x000000010007041f bitcoin-qt`GUIUtil::SetStartOnSystemStartup(bool) + 223
    frame #4: 0x00000001000a25fa bitcoin-qt`OptionsModel::setData(QModelIndex const&, QVariant const&, int) + 202
    frame #5: 0x000000010241a7a7 QtWidgets`QItemDelegate::setModelData(QWidget*, QAbstractItemModel*, QModelIndex const&) const + 295
    frame #6: 0x0000000102423b48 QtWidgets`___lldb_unnamed_symbol4564$$QtWidgets + 248
    frame #7: 0x0000000102424c7b QtWidgets`QDataWidgetMapper::submit() + 59
    frame #8: 0x000000010009573b bitcoin-qt`OptionsDialog::on_okButton_clicked() + 43
    frame #9: 0x00000001001e0f9a bitcoin-qt`OptionsDialog::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) + 202
    frame #10: 0x00000001001e136b bitcoin-qt`OptionsDialog::qt_metacall(QMetaObject::Call, int, void**) + 139
    frame #11: 0x0000000102e79bbd QtCore`QMetaObject::activate(QObject*, int, int, void**) + 1773
    frame #12: 0x000000010227aacf QtWidgets`___lldb_unnamed_symbol1034$$QtWidgets + 111
    frame #13: 0x000000010227a96c QtWidgets`___lldb_unnamed_symbol1032$$QtWidgets + 236
    frame #14: 0x000000010227ba9f QtWidgets`QAbstractButton::mouseReleaseEvent(QMouseEvent*) + 271

jonasschnelli · 2019-01-20T20:58:47Z

@fanquake: your right. There is something wrong with our implementation... I'll fix that soon.

jonasschnelli · 2019-01-21T06:39:08Z

Current macOS launch at login code does release an item which belongs to an array that gets release later (double release) which causes a crash on certain macOS versions.

I added a commit that fixes the memory mismanagement.

hebasto

Tested 541d287b805ecaad1d9810b5a776d2e0d8b87233 on macOS 10.13.6.

hebasto · 2019-01-21T09:39:59Z

doc/release-notes.md

CFLAGS="-mmacosx-version-min=10.11" could be added as well.
Without CFLAGS there are linker warnings:

hebasto · 2019-01-21T09:41:35Z

src/qt/guiutil.cpp

This function declaration seems unnecessary. Could it be removed?

hebasto · 2019-01-21T09:45:21Z

src/qt/guiutil.cpp

res is a bool already. Could the comment be removed?

jonasschnelli · 2019-01-21T18:28:46Z

Fixed @hebasto findings

hebasto · 2019-01-21T20:43:38Z

tACK 35f5cb791de6bd47b62e031a74d885f00efd2585 (macOS 10.13.6 + Qt 5.11.2)

The depends built binaries (Gitian) are not affected since we are building with min macOS 10.10.

Note: official v0.17.1 is affected.
Could backport?

doc/release-notes.md

…10.11

laanwj · 2019-01-22T21:52:09Z

utACK da60118

… macOS 10.11, fix memory missmanagement da60118 Fix macOS launch-at-startup memory issue (Jonas Schnelli) 516437a Qt: remove macOS launch-at-startup option when compiled with > macOS 10.11 (Jonas Schnelli) Pull request description: The launch-at-startup API Bitcoin Core uses on macOS where removed in macOS 10.11 leading to a segmentation-fault due to the weak-linking when not actively compiled against SDK 10.11 (`-mmacosx-version-min=10.11`) This PR removes the launch-at-startup feature on macOS when compiled with macOS min version > 10.11 (the default is always the macOS version you compile on). **The depends built binaries (Gitian) are not affected since we are building with min macOS 10.10.** Users self compiling on macOS > 10.11 can re-enable the feature by compiling with min version <= 10.11 (`CXXFLAGS="-mmacosx-version-min=10.11" CFLAGS="-mmacosx-version-min=10.11" ./configure`) **Isn't there a new API from Apple?** Yes, [there is](https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/CreatingLoginItems.html). It will require to create a helper application which needs to be embedded in the .app folder (needs code signing as well). Developers willing to go down that rabbit hole are welcome. Fixes #15142 Tree-SHA512: fa9cc4e39d5a2d2559919b7e22b7766f5e0269a361719294d4a4a2df2fd9d955e5b23b5907e68023fdeee297f652f844f3c447904bf18f9c1145348ad101c432

The macOS startup item code was disabled for builds targeting macOS > 10.11 in bitcoin#15208. Now that we require macOS 10.12 as a minimum, bitcoin#17550, we can remove the startup item code entirely, as the API we were using was removed in macOS 10.12.

27d82b6 gui: remove macOS start on login code (fanquake) Pull request description: The macOS startup item code was disabled for builds targeting macOS > `10.11` in #15208. Now that we require macOS `10.12` as a minimum (#17550), we can remove the startup item code entirely. The API we were using, `LSSharedFileListItemCopyResolvedURL`, `LSSharedFileListCopySnapshot` etc, was removed in macOS `10.12` SDK. ACKs for top commit: jonasschnelli: utACK 27d82b6 jonasschnelli: Tested ACK 27d82b6 - successfully compiled on 10.15.1 Tree-SHA512: 7420757b91c7820e6a63280887155394547134a9cebcf3721af0284da23292627f94cd431241e033075b3fd86d79ace3ebf1b25d17763acbf71e07a742395409

27d82b6 gui: remove macOS start on login code (fanquake) Pull request description: The macOS startup item code was disabled for builds targeting macOS > `10.11` in bitcoin#15208. Now that we require macOS `10.12` as a minimum (bitcoin#17550), we can remove the startup item code entirely. The API we were using, `LSSharedFileListItemCopyResolvedURL`, `LSSharedFileListCopySnapshot` etc, was removed in macOS `10.12` SDK. ACKs for top commit: jonasschnelli: utACK 27d82b6 jonasschnelli: Tested ACK 27d82b6 - successfully compiled on 10.15.1 Tree-SHA512: 7420757b91c7820e6a63280887155394547134a9cebcf3721af0284da23292627f94cd431241e033075b3fd86d79ace3ebf1b25d17763acbf71e07a742395409

The macOS startup item code was disabled for builds targeting macOS > 10.11 in bitcoin#15208. Now that we require macOS 10.12 as a minimum, bitcoin#17550, we can remove the startup item code entirely, as the API we were using was removed in macOS 10.12.

Summary: > The macOS startup item code was disabled for builds targeting macOS > > 10.11 in #15208. Now that we require macOS 10.12 as a minimum, #17550, > we can remove the startup item code entirely, as the API we were using > was removed in macOS 10.12. We have `OSX_MIN_VERSION=10.12` set in `depends/hosts/darwin.mk` This is a backport of [[bitcoin/bitcoin#15208 | PR15208]] and [[bitcoin/bitcoin#17567 | PR17567]] All code modified in `src/qt/guituil.cpp` in [[bitcoin/bitcoin#15208 | PR15208]] is removed in [[bitcoin/bitcoin#17567 | PR17567]], including everything related to *fix memory missmanagement*. Test Plan: I do not have a Mac to test this, so I rely on CI cross compilation tests. `ninja all check-all` Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8280

27d82b6 gui: remove macOS start on login code (fanquake) Pull request description: The macOS startup item code was disabled for builds targeting macOS > `10.11` in bitcoin#15208. Now that we require macOS `10.12` as a minimum (bitcoin#17550), we can remove the startup item code entirely. The API we were using, `LSSharedFileListItemCopyResolvedURL`, `LSSharedFileListCopySnapshot` etc, was removed in macOS `10.12` SDK. ACKs for top commit: jonasschnelli: utACK 27d82b6 jonasschnelli: Tested ACK 27d82b6 - successfully compiled on 10.15.1 Tree-SHA512: 7420757b91c7820e6a63280887155394547134a9cebcf3721af0284da23292627f94cd431241e033075b3fd86d79ace3ebf1b25d17763acbf71e07a742395409

The macOS startup item code was disabled for builds targeting macOS > 10.11 in #15208. Now that we require macOS 10.12 as a minimum, #17550, we can remove the startup item code entirely, as the API we were using was removed in macOS 10.12.

2896a48 GUI: remove macOS start on login (furszy) 300d752 scripted-diff: prefer MAC_OSX over __APPLE__ (furszy) 7467c9f gui: remove macOS ProgressBar workaround (fanquake) c7575ac gui: remove SubstituteFonts (fanquake) 15ed388 Use window() instead of obsolete topLevelWidget() (Hennadii Stepanov) d6e1383 Fix macOS launch-at-startup memory issue (Jonas Schnelli) 233b156 Qt: remove macOS launch-at-startup option when compiled with > macOS 10.11 (Jonas Schnelli) d787b25 Cleanup: Remove unused TableViewLastColumnResizingFixer and DHMSTableWidgetItem from guiutil.h/cpp (furszy) Pull request description: Straightforward update for the `guiutil` files. First commit removes unused `TableViewLastColumnResizingFixer` and `DHMSTableWidgetItem` from `guiutil.h/cpp`. Second and third commit comes from bitcoin#15208. Fourth commit comes from bitcoin#14801. ACKs for top commit: random-zebra: re-utACK 2896a48 Fuzzbawls: utACK 2896a48 Tree-SHA512: b66920006007017a7fc9de06405627ab968d0d5fb2aab97d7131af6b044b34bbf370ebc47b87e3d088a83a1596d056b02e41d929e5180902ed6da4529e179f6d

…h > macOS 10.11

jonasschnelli added the macOS label Jan 19, 2019

hebasto reviewed Jan 19, 2019

View reviewed changes

doc/release-notes.md Outdated

Copy link

Member

hebasto Jan 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in --> is ?

donaloconnor reacted with thumbs up emoji

jonasschnelli force-pushed the 2019/01/macos_autostart branch from 33ebd96 to 541d287 Compare January 21, 2019 06:17

jonasschnelli changed the title ~~Qt: remove macOS launch-at-startup when compiled with > macOS 10.11~~ Qt: remove macOS launch-at-startup when compiled with > macOS 10.11, fix memory missmanagement Jan 21, 2019

hebasto requested changes Jan 21, 2019

View reviewed changes

jonasschnelli force-pushed the 2019/01/macos_autostart branch from 541d287 to 35f5cb7 Compare January 21, 2019 18:28

practicalswift reviewed Jan 22, 2019

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

jonasschnelli added 2 commits January 21, 2019 22:42

Qt: remove macOS launch-at-startup option when compiled with > macOS …

516437a

…10.11

Fix macOS launch-at-startup memory issue

da60118

jonasschnelli force-pushed the 2019/01/macos_autostart branch from 35f5cb7 to da60118 Compare January 22, 2019 08:43

laanwj merged commit da60118 into bitcoin:master Jan 22, 2019

fanquake mentioned this pull request Nov 23, 2019

gui: remove macOS start on login code #17567

Merged

furszy mentioned this pull request May 11, 2021

[Cleanup] guiutils cleanup + fix memory mismanagement PIVX-Project/PIVX#2380

Merged

kwvg added a commit to kwvg/dash that referenced this pull request Aug 26, 2021

merge bitcoin#15208: remove macOS launch-at-startup when compiled wit…

38bf71e

…h > macOS 10.11

kwvg added a commit to kwvg/dash that referenced this pull request Aug 30, 2021

merge bitcoin#15208: remove macOS launch-at-startup when compiled wit…

839568f

…h > macOS 10.11

kwvg added a commit to kwvg/dash that referenced this pull request Aug 31, 2021

merge bitcoin#15208: remove macOS launch-at-startup when compiled wit…

def36ab

…h > macOS 10.11

lyricidal mentioned this pull request Nov 3, 2021

[Upstream] [Cleanup] guiutils cleanup + fix memory mismanagement PRCYCoin/PRCYCoin#145

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Qt: remove macOS launch-at-startup when compiled with > macOS 10.11, fix memory missmanagement #15208

Qt: remove macOS launch-at-startup when compiled with > macOS 10.11, fix memory missmanagement #15208

Uh oh!

Conversation

jonasschnelli commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jan 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto Jan 19, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jan 19, 2019

Uh oh!

fanquake commented Jan 20, 2019

Uh oh!

jonasschnelli commented Jan 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Jan 21, 2019

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Jan 21, 2019

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Jan 21, 2019

Uh oh!

hebasto commented Jan 21, 2019

Uh oh!

Uh oh!

laanwj commented Jan 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jonasschnelli commented Jan 19, 2019 •

edited

Loading

hebasto commented Jan 19, 2019 •

edited

Loading

jonasschnelli commented Jan 20, 2019 •

edited

Loading