Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Jan 22, 2019

Conversation

@jonasschnelli
Copy link
Member

@jonasschnelli jonasschnelli commented Jan 19, 2019

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
Copy link
Member

@hebasto hebasto commented Jan 19, 2019

utACK 8441c90af88fb0f08762cf56e508803ed4b47d4c modulo typo

doc/release-notes.md Outdated
@@ -262,6 +262,11 @@ Graphical User Interface (GUI)
balance shown if the wallet was created using the `createwallet` RPC
and the `disable_private_keys` parameter was set to true.

- The launch-on-startup option in no longer available on macOS if

This comment has been minimized.

@hebasto

hebasto Jan 19, 2019
Member

in --> is ?

@laanwj
Copy link
Member

@laanwj laanwj commented Jan 19, 2019

utACK

@fanquake
Copy link
Member

@fanquake fanquake commented Jan 20, 2019

Compiling 8441c90 on macOS 10.14.2:

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

The option is disabled.
pr - no 10 11

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
Copy link
Member Author

@jonasschnelli jonasschnelli commented Jan 20, 2019

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

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2019/01/macos_autostart branch Jan 21, 2019
@jonasschnelli 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
@jonasschnelli
Copy link
Member Author

@jonasschnelli jonasschnelli commented Jan 21, 2019

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.

Copy link
Member

@hebasto hebasto left a comment

Tested 541d287b805ecaad1d9810b5a776d2e0d8b87233 on macOS 10.13.6.

doc/release-notes.md Outdated
@@ -262,6 +262,11 @@ Graphical User Interface (GUI)
balance shown if the wallet was created using the `createwallet` RPC
and the `disable_private_keys` parameter was set to true.

- The launch-on-startup option is no longer available on macOS if
compiled with macosx min version greater then 10.11 (use
CXXFLAGS="-mmacosx-version-min=10.11" for setting the deployment

This comment has been minimized.

@hebasto

hebasto Jan 21, 2019
Member

CFLAGS="-mmacosx-version-min=10.11" could be added as well.
Without CFLAGS there are linker warnings:
screenshot from 2019-01-21 11-38-04

src/qt/guiutil.cpp Outdated
// based on: https://github.com/Mozketo/LaunchAtLoginController/blob/master/LaunchAtLoginController.m

LSSharedFileListItemRef findStartupItemInList(LSSharedFileListRef list, CFURLRef findUrl);
LSSharedFileListItemRef findStartupItemInList(LSSharedFileListRef list, CFURLRef findUrl)
LSSharedFileListItemRef findStartupItemInList(CFArrayRef listSnapshot, LSSharedFileListRef list, CFURLRef findUrl);

This comment has been minimized.

@hebasto

hebasto Jan 21, 2019
Member

This function declaration seems unnecessary. Could it be removed?

src/qt/guiutil.cpp Outdated
return !!foundItem; // return boolified object
CFRelease(loginItems);
CFRelease(listSnapshot);
return res; // return boolified object

This comment has been minimized.

@hebasto

hebasto Jan 21, 2019
Member

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

@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2019/01/macos_autostart branch Jan 21, 2019
@jonasschnelli
Copy link
Member Author

@jonasschnelli jonasschnelli commented Jan 21, 2019

Fixed @hebasto findings

@hebasto
Copy link
Member

@hebasto hebasto commented Jan 21, 2019

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 Outdated Show resolved Hide resolved
@jonasschnelli jonasschnelli force-pushed the jonasschnelli:2019/01/macos_autostart branch to da60118 Jan 22, 2019
@laanwj
Copy link
Member

@laanwj laanwj commented Jan 22, 2019

utACK da60118

@laanwj laanwj merged commit da60118 into bitcoin:master Jan 22, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Jan 22, 2019
… 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
fanquake added a commit to fanquake/bitcoin that referenced this pull request Nov 22, 2019
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.
fanquake added a commit that referenced this pull request Nov 26, 2019
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
sidhujag added a commit to syscoin/syscoin that referenced this pull request Nov 27, 2019
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
HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Apr 17, 2020
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.
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 5, 2020
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
sidhujag added a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
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
backpacker69 referenced this pull request in peercoin/peercoin Mar 28, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants