Accessible List Box Row Segfault/Crash on Exit

Development
1

Hi

Juce 6.1.6.

In a situation where I create a ListBox and ListBoxModel with a custom rowComponent, and when that rowComponent contains itself a component which has accessibility, and when I exit my program with that list box on display, I get a segfault in unwinding the destructors. The reason for this is, it seems, that the ListBox contains a weak pointer to the ListBoxModel - which is fine once the ListBox is destroyed - but the accessibility handlers keep references to the individual components of the listbox.

The crash I get is here

 AccessibleState getCurrentState() const override
        {
            if (auto* m = rowComponent.owner.getModel())
                if (rowComponent.row >= m->getNumRows())
                    return AccessibleState().withIgnored();

the ‘m’ is a dead pointer at this point in the stack. This happens in the destruction of the rows subordinate button, whether I use my custom button class or a juce::TextButton.

The relevant stack is below. This problem is actually pretty easy for me to work around. In the destructor of the component holding the list box, before I free, I set the listboxmodel to nullptr. That is, I do

KeyBindingsOverlay::~KeyBindingsOverlay()
{
    bindingList->setModel(nullptr);
}

But this order issue only occurs with the accessible feature of a subcomponent of the row component activated, so thought you may want to take a look. and figure I would post it here in case other people are making accessible rows with tables.

Here’s the full stack if it helps.

0   Surge XT                      	       0x100fa23a0 juce::ListBox::RowComponent::RowAccessibilityHandler::getCurrentState() const + 76
1   Surge XT                      	       0x100fa2378 juce::ListBox::RowComponent::RowAccessibilityHandler::getCurrentState() const + 36
2   Surge XT                      	       0x100e56a78 juce::AccessibilityHandler::isIgnored() const + 64
3   Surge XT                      	       0x100e56f40 juce::getUnignoredAncestor(juce::AccessibilityHandler*) + 48
4   Surge XT                      	       0x100e56eb4 juce::AccessibilityHandler::getParent() const + 56
5   Surge XT                      	       0x100e575c4 juce::AccessibilityHandler::isParentOf(juce::AccessibilityHandler const*) const + 52
6   Surge XT                      	       0x100e569d4 juce::AccessibilityHandler::hasFocus(bool) const + 124
7   Surge XT                      	       0x100e5670c juce::AccessibilityHandler::giveAwayFocus() const + 36
8   Surge XT                      	       0x100e5664c juce::AccessibilityHandler::~AccessibilityHandler() + 44
9   Surge XT                      	       0x10093ff08 Surge::Widgets::MultiSwitchAHMenuOnly::~MultiSwitchAHMenuOnly() + 56
10  Surge XT                      	       0x10093e034 Surge::Widgets::MultiSwitchAHMenuOnly::~MultiSwitchAHMenuOnly() + 28
11  Surge XT                      	       0x10093e060 Surge::Widgets::MultiSwitchAHMenuOnly::~MultiSwitchAHMenuOnly() + 28
12  Surge XT                      	       0x101012f80 std::__1::default_delete<juce::AccessibilityHandler>::operator()(juce::AccessibilityHandler*) const + 52
13  Surge XT                      	       0x101012ef4 std::__1::unique_ptr<juce::AccessibilityHandler, std::__1::default_delete<juce::AccessibilityHandler> >::reset(juce::AccessibilityHandler*) + 96
14  Surge XT                      	       0x101012e84 std::__1::unique_ptr<juce::AccessibilityHandler, std::__1::default_delete<juce::AccessibilityHandler> >::~unique_ptr() + 32
15  Surge XT                      	       0x100e58e7c std::__1::unique_ptr<juce::AccessibilityHandler, std::__1::default_delete<juce::AccessibilityHandler> >::~unique_ptr() + 28
16  Surge XT                      	       0x100e58664 juce::Component::~Component() + 684
17  Surge XT                      	       0x100928274 Surge::Widgets::MultiSwitch::~MultiSwitch() + 152
18  Surge XT                      	       0x10092f358 Surge::Widgets::MultiSwitchSelfDraw::~MultiSwitchSelfDraw() + 128
19  Surge XT                      	       0x1007dccd8 Surge::Widgets::SelfDrawButton::~SelfDrawButton() + 140
20  Surge XT                      	       0x1007dac20 Surge::Widgets::SelfDrawButton::~SelfDrawButton() + 28
21  Surge XT                      	       0x1007dac4c Surge::Widgets::SelfDrawButton::~SelfDrawButton() + 28
22  Surge XT                      	       0x1006757f4 std::__1::default_delete<Surge::Widgets::SelfDrawButton>::operator()(Surge::Widgets::SelfDrawButton*) const + 52
23  Surge XT                      	       0x100675768 std::__1::unique_ptr<Surge::Widgets::SelfDrawButton, std::__1::default_delete<Surge::Widgets::SelfDrawButton> >::reset(Surge::Widgets::SelfDrawButton*) + 96
24  Surge XT                      	       0x1006756f8 std::__1::unique_ptr<Surge::Widgets::SelfDrawButton, std::__1::default_delete<Surge::Widgets::SelfDrawButton> >::~unique_ptr() + 32
25  Surge XT                      	       0x100670a8c std::__1::unique_ptr<Surge::Widgets::SelfDrawButton, std::__1::default_delete<Surge::Widgets::SelfDrawButton> >::~unique_ptr() + 28
26  Surge XT                      	       0x100684940 Surge::Overlays::KeyBindingsListRow::~KeyBindingsListRow() + 48
27  Surge XT                      	       0x10068122c Surge::Overlays::KeyBindingsListRow::~KeyBindingsListRow() + 28
28  Surge XT                      	       0x100681258 Surge::Overlays::KeyBindingsListRow::~KeyBindingsListRow() + 28
29  Surge XT                      	       0x101065f4c std::__1::default_delete<juce::Component>::operator()(juce::Component*) const + 52
30  Surge XT                      	       0x100e9ac6c std::__1::unique_ptr<juce::Component, std::__1::default_delete<juce::Component> >::reset(juce::Component*) + 96
31  Surge XT                      	       0x101026d34 std::__1::unique_ptr<juce::Component, std::__1::default_delete<juce::Component> >::~unique_ptr() + 32
32  Surge XT                      	       0x100e9a8ec std::__1::unique_ptr<juce::Component, std::__1::default_delete<juce::Component> >::~unique_ptr() + 28
33  Surge XT                      	       0x100fa14c8 juce::ListBox::RowComponent::~RowComponent() + 72
34  Surge XT                      	       0x100fa0b48 juce::ListBox::RowComponent::~RowComponent() + 28
35  Surge XT                      	       0x100fa0b74 juce::ListBox::RowComponent::~RowComponent() + 28
36  Surge XT                      	       0x100f9f39c juce::ContainerDeletePolicy<juce::ListBox::RowComponent>::destroy(juce::ListBox::RowComponent*) + 64
37  Surge XT                      	       0x100f9f09c juce::OwnedArray<juce::ListBox::RowComponent, juce::DummyCriticalSection>::deleteAllObjects() + 104
38  Surge XT                      	       0x100f9efe0 juce::OwnedArray<juce::ListBox::RowComponent, juce::DummyCriticalSection>::~OwnedArray() + 28
39  Surge XT                      	       0x100f9e564 juce::OwnedArray<juce::ListBox::RowComponent, juce::DummyCriticalSection>::~OwnedArray() + 28
40  Surge XT                      	       0x100f9f708 juce::ListBox::ListViewport::~ListViewport() + 96
41  Surge XT                      	       0x100f9e590 juce::ListBox::ListViewport::~ListViewport() + 28
42  Surge XT                      	       0x100f9e5bc juce::ListBox::ListViewport::~ListViewport() + 28
43  Surge XT                      	       0x1010bb348 std::__1::default_delete<juce::ListBox::ListViewport>::operator()(juce::ListBox::ListViewport*) const + 52
44  Surge XT                      	       0x100efa6f0 std::__1::unique_ptr<juce::ListBox::ListViewport, std::__1::default_delete<juce::ListBox::ListViewport> >::reset(juce::ListBox::ListViewport*) + 96
45  Surge XT                      	       0x100e909fc juce::ListBox::~ListBox() + 84
46  Surge XT                      	       0x100e946c0 juce::ListBox::~ListBox() + 28
47  Surge XT                      	       0x100efa844 juce::ListBox::~ListBox() + 28
48  Surge XT                      	       0x1006759fc std::__1::default_delete<juce::ListBox>::operator()(juce::ListBox*) const + 52
49  Surge XT                      	       0x100675970 std::__1::unique_ptr<juce::ListBox, std::__1::default_delete<juce::ListBox> >::reset(juce::ListBox*) + 96
50  Surge XT                      	       0x100675900 std::__1::unique_ptr<juce::ListBox, std::__1::default_delete<juce::ListBox> >::~unique_ptr() + 32
51  Surge XT                      	       0x100670f64 std::__1::unique_ptr<juce::ListBox, std::__1::default_delete<juce::ListBox> >::~unique_ptr() + 28
52  Surge XT                      	       0x100671048 Surge::Overlays::KeyBindingsOverlay::~KeyBindingsOverlay() + 72
53  Surge XT                      	       0x1006710a0 Surge::Overlays::KeyBindingsOverlay::~KeyBindingsOverlay() + 28
54  Surge XT                      	       0x1006710e8 Surge::Overlays::KeyBindingsOverlay::~KeyBindingsOverlay() + 28
55  Surge XT                      	       0x100778948 std::__1::default_delete<juce::Component>::operator()(juce::Component*) const + 52
56  Surge XT                      	       0x1007788bc std::__1::unique_ptr<juce::Component, std::__1::default_delete<juce::Component> >::reset(juce::Component*) + 96
57  Surge XT                      	       0x10077884c std::__1::unique_ptr<juce::Component, std::__1::default_delete<juce::Component> >::~unique_ptr() + 32
58  Surge XT                      	       0x10076f144 std::__1::unique_ptr<juce::Component, std::__1::default_delete<juce::Component> >::~unique_ptr() + 28
59  Surge XT                      	       0x10076f28c Surge::Overlays::OverlayWrapper::~OverlayWrapper() + 132
60  Surge XT                      	       0x10076f36c Surge::Overlays::OverlayWrapper::~OverlayWrapper() + 28
61  Surge XT                      	       0x100771814 Surge::Overlays::OverlayWrapper::~OverlayWrapper() + 28
2

I’ve tried to repro this, both in the WidgetsDemo (modified to include a ListBox with custom rows containing TextButtons) and in Surge XT standalone, built from the current main branch. The KeyBindingsOverlay destructor is defaulted, so I assume it should still exhibit the buggy behaviour. Unfortunately, I haven’t been able to trigger a crash yet.

My test procedure is to ensure that the ListBox is showing on screen, and then to quit the app, either with cmd+Q, or from the dock, or by clicking ‘X’ in the titlebar. I’ve tested on macOS 10.15.

We’ve had reports of a very similar issue in the past, so I’m very keen to resolve this bug. Are you able to provide any more details that might help me to repro the problem? In particular, I’d be interested to know which OS you’re using for testing, and ideally the exact sequence of mouse clicks that causes the problem, as I suspect the issue is partly dependent on component focus. If you’re able to make a screen recording, I think that might help…

3

Oh I’m sorry I haven’t merged the crashing change yet since I just got it uncrashing last night. Let me push it to a branch this morning.

4

And yes sorry this is almost a good bug report but doesn’t have a repro … will send full instructions. Definitely focus related. If I use labels there’s no problem but any button handler with focus management crashes it

5

OK so sorry about that. I pushed the crashing change to a branch.

so in your surge build directory

git remote add bp https://github.com/baconpaul/surge.git
git fetch bp
git checkout bp/acc-crash-for-reuk

and build.

in that branch you’ll see there are two defines at the top of src/surge-xt/gui/overlays/KeyManagerOverlay.cpp

#define UNDO_MODEL_IN_DTOR 0
#define USE_JUCE_TEXTBUTTON 1

The first controls whether or not to null the model in the dtor the second switches between juce::TextButton and our custom button class, just to eliminate our class as the culprit.

To get a reliable crash, build surge there, then, on macOS 11 compiled as ARM (for me):

  1. Open the key manager overlay (menu / workflow / edit keybindings)
  2. Scroll up and down
  3. Focus and unfocus a button then click the button then dismiss the menu
  4. With the overlay still open, quit (cmd-Q)

and most of the time you will get a segv on exit. If you change the UNDO_MODEL_IN_DTOR to 1 you never do.

Hope that helps! And thanks as always for looking so quickly.

6

Thanks for the detailed instructions, I can repro the problem now.

At the moment, this looks like a fairly standard use-after-free. During its destructor, the ListBox and its AccessibilityHandlers may try to access the current ListBoxModel, but will end up dereferencing a dangling pointer if the ListBoxModel has already been freed without explicitly calling setModel (nullptr).

In Surge, I was also able to avoid the crash by switching the order of data members in the KeyBindingsOverlay:

std::unique_ptr<KeyBindingsListBoxModel> bindingListBoxModel;
std::unique_ptr<juce::ListBox> bindingList; // destroyed before the bindingListBoxModel

This problem only affects accessible ListBoxes because these end up trying to use the model pointer during the ListBox destructor. I think the issue is still present (but benign) in non-accessible ListBoxes: if the ListBoxModel was already destroyed, then the ListBox will still end up holding a dangling pointer, which seems like something best avoided.

It looks like we need to update the documentation for the ListBox to specify that the ListBoxModel must remain alive for as long as the ListBox holds a pointer to it. I think we can also add an assertion to debug-mode builds to explain what’s going on and how to fix the problem.

Hopefully that sounds like a reasonable resolution, but let me know if you have other ideas.

1 Like
7

Ha yeah I thought about flipping that order. And same thing as nulling. It explains why our mod list doesn’t have the same problem though :slight_smile:

Any thoughts in setting the model to null in the listbox destructor? Since the listbox doesn’t own the model this does end up being a bit of a sharp knife to trip over now. I guess in a perfect world there would be an option for the listbox to own it’s model like some other places with raw pointers then I could init with .release(), true.

But indeed these are both the same class of fix and I can apply them to surge!

8

This seems like a partial fix. If MyListBox calls one of its member functions from its destructor, and that member function happens to access the model pointer, we’ll still run into use-after-free problems if the model gets destroyed first (the destructor of MyListBox will run before the destructor of ListBox). It’s probably safer to add explicit sanity checks whenever the model is accessed in debug-mode builds, but to keep release-mode builds unchanged.

Having the ListBox own the Model might help, but it might also end up moving the problem elsewhere. It would also add more complexity, so I’m not sure it’d be a beneficial change overall. It’s something to keep in mind if we ever do a V2 version of the ListBox, though.

9

Makes perfect sense

Thanks as always for looking so quickly

10

Oh and the api I was thinking of was reiszablewindow setcontentowned. If listbox had a set model and a set model owned etc

Low pri thoigh!

11

I’ve added some additional sanity checks to ListBox on the juce7 branch:

12

Great thanks!

13

I’m hitting the same issue when I tried to implement the custom component from this tutorial:

https://docs.juce.com/master/tutorial_table_list_box.html

 thread #1, name = 'JUCE Message Thread', queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x7b0ea7a40a00)
  * frame #0: 0x0000000100934ff4 `juce::TableListBox::RowComp::RowAccessibilityHandler::getCurrentState(this=0x0000600002474af0) const at juce_TableListBox.cpp:257:44
    frame #1: 0x00000001007e0758 `juce::AccessibilityHandler::isIgnored(this=0x0000600002474af0) const at juce_AccessibilityHandler.cpp:89:50
    frame #2: 0x00000001007e0c20 `juce::getUnignoredAncestor(handler=0x0000600002474af0) at juce_AccessibilityHandler.cpp:162:25
    frame #3: 0x00000001007e0b94 `juce::AccessibilityHandler::getParent(this=0x0000600002150e00) const at juce_AccessibilityHandler.cpp:200:16
    frame #4: 0x00000001007e128c `juce::AccessibilityHandler::isParentOf(this=0x000060000244c3f0, possibleChild=0x0000600002150e00) const at juce_AccessibilityHandler.cpp:244:40
    frame #5: 0x00000001007e06b4 `juce::AccessibilityHandler::hasFocus(this=0x000060000244c3f0, trueIfChildFocused=true) const at juce_AccessibilityHandler.cpp:275:43
    frame #6: 0x00000001007e0450 `juce::AccessibilityHandler::giveAwayFocus(this=0x000060000244c3f0) const at juce_AccessibilityHandler.cpp:286:9
    frame #7: 0x00000001007e0390 `juce::AccessibilityHandler::~AccessibilityHandler(this=0x000060000244c3f0) at juce_AccessibilityHandler.cpp:71:5
    frame #8: 0x00000001007e048c `juce::AccessibilityHandler::~AccessibilityHandler(this=0x000060000244c3f0) at juce_AccessibilityHandler.cpp:70:1
    frame #9: 0x00000001007e04b8 `juce::AccessibilityHandler::~AccessibilityHandler(this=0x000060000244c3f0) at juce_AccessibilityHandler.cpp:70:1
    frame #10: 0x000000010098cf2c `std::__1::default_delete<juce::AccessibilityHandler>::operator(this=0x000000015671e5b0, __ptr=0x000060000244c3f0)(juce::AccessibilityHandler*) const at unique_ptr.h:57:5
    frame #11: 0x000000010098cea0 `std::__1::unique_ptr<juce::AccessibilityHandler, std::__1::default_delete<juce::AccessibilityHandler> >::reset(this=0x000000015671e5b0, __p=0x0000000000000000) at unique_ptr.h:318:7
    frame #12: 0x000000010098ce30 `std::__1::unique_ptr<juce::AccessibilityHandler, std::__1::default_delete<juce::AccessibilityHandler> >::~unique_ptr(this=0x000000015671e5b0) at unique_ptr.h:272:19
    frame #13: 0x00000001007e2ac0 `std::__1::unique_ptr<juce::AccessibilityHandler, std::__1::default_delete<juce::AccessibilityHandler> >::~unique_ptr(this=0x000000015671e5b0) at unique_ptr.h:272:17
    frame #14: 0x00000001007e22a8 `juce::Component::~Component(this=0x000000015671e4e0) at juce_Component.cpp:548:1
    frame #15: 0x000000010088c5e4 `juce::TableHeaderComponent::~TableHeaderComponent(this=0x000000015671e4e0) at juce_TableHeaderComponent.cpp:58:1
    frame #16: 0x0000000100a382c4 `juce::TableListBox::Header::~Header(this=0x000000015671e4e0) at juce_TableListBox.cpp:310:21
    frame #17: 0x0000000100a37de8 `juce::TableListBox::Header::~Header(this=0x000000015671e4e0) at juce_TableListBox.cpp:310:21
    frame #18: 0x0000000100a37e14 `juce::TableListBox::Header::~Header(this=0x000000015671e4e0) at juce_TableListBox.cpp:310:21
    frame #19: 0x00000001009dc6d0 `std::__1::default_delete<juce::Component>::operator(this=0x000000015a0641f8, __ptr=0x000000015671e4e0)(juce::Component*) const at unique_ptr.h:57:5
    frame #20: 0x0000000100823c7c `std::__1::unique_ptr<juce::Component, std::__1::default_delete<juce::Component> >::reset(this=0x000000015a0641f8, __p=0x0000000000000000) at unique_ptr.h:318:7
    frame #21: 0x0000000100819a40 `juce::ListBox::~ListBox(this=0x000000015a0640f8) at juce_ListBox.cpp:528:21
    frame #22: 0x000000010089062c `juce::TableListBox::~TableListBox(this=0x000000015a0640f8) at juce_TableListBox.cpp:356:1
    frame #23: 0x0000000100890658 `juce::TableListBox::~TableListBox(this=0x000000015a0640f8) at juce_TableListBox.cpp:355:1
    frame #24: 0x0000000100720c68 `::PluginListComponent::~PluginListComponent(this=0x000000015a064000) at PluginListComponent.cpp:298:1
    frame #25: 0x0000000100720db8 `::PluginListComponent::~PluginListComponent(this=0x000000015a064000) at PluginListComponent.cpp:296:1
    frame #26: 0x0000000100720e00 `::PluginListComponent::~PluginListComponent(this=0x000000015a064000) at PluginListComponent.cpp:296:1
    frame #27: 0x0000000100716ab8 `std::__1::default_delete<::PluginListComponent>::operator(this=0x00000001590379e0, __ptr=0x000000015a064000)(::PluginListComponent*) const at unique_ptr.h:57:5
    frame #28: 0x0000000100716a2c `std::__1::unique_ptr<::PluginListComponent, std::__1::default_delete<::PluginListComponent> >::reset(this=0x00000001590379e0, __p=0x0000000000000000) at unique_ptr.h:318:7
    frame #29: 0x00000001007169bc `std::__1::unique_ptr<::PluginListComponent, std::__1::default_delete<::PluginListComponent> >::~unique_ptr(this=0x00000001590379e0) at unique_ptr.h:272:19
    frame #30: 0x0000000100715950 `std::__1::unique_ptr<::PluginListComponent, std::__1::default_delete<::PluginListComponent> >::~unique_ptr(this=0x00000001590379e0) at unique_ptr.h:272:17
    frame #31: 0x0000000100715ae4 `MainComponent::~MainComponent(this=0x0000000159037800) at MainComponent.cpp:194:1
    frame #32: 0x0000000100715b98 `MainComponent::~MainComponent(this=0x0000000159037800) at MainComponent.cpp:192:1
    frame #33: 0x0000000100715be0 `MainComponent::~MainComponent(this=0x0000000159037800) at MainComponent.cpp:192:1

Edit: Fixed this by adding setTableModel(nullptr) in the destructor of PluginListComponent, which is a modified version of the one in JUCE.