Settings API can make a remote crash [Fixed in Roon 1.8]

Hi @ben,

I’m running into an issue with the settings API and I hope you can help me.

The issue is that opening the settings of extension A and then switching to the settings of extension B can make the remote crash. Whether it crashes seems to depend on the settings layout, especially the use of groups seems to be relevant.

I have been trying to track this issue down for a couple of hours, and at this point my impression is that the issue is not in my code. I succeeded in isolation the issue by creating 2 very basic extensions. I hope that you can reproduce the issue by running these extensions from the code below.

The steps I take are:

  • Open the settings of extension 2 and open the drop down item
  • Cancel the dialog by pressing outside the dialog (all is well)
  • Open the settings of extension 1 and open the drop down item
  • Cancel the dialog by pressing outside the dialog (all is well)
  • Open the settings of extension 2 and try to open the drop down item (remote crashes)

I noticed that placing the zone picker of extension 1 in a group prevents the crash.

Extension 1:

"use strict";

var RoonApi          = require("node-roon-api"),
    RoonApiSettings  = require('node-roon-api-settings');

var core = undefined;

var roon = new RoonApi({
    extension_id:        'com.example.1',
    display_name:        'Example 1',
    display_version:     '0.1.0',
    publisher:           'Example',
    email:               'one@example.com',

    core_paired: function(core_) {
        core = core_;
    },
    core_unpaired: function(core_) {
        core = undefined;
    }
});

var my_settings = roon.load_config("settings") || {
};

function makelayout(settings) {
    let l = {
        values:    settings,
        layout:    [],
        has_error: false
    };

    l.layout.push({
        type:    "zone",
        title:   "Zone",
        setting: "zone"
    });

    return l;
}

var svc_settings = new RoonApiSettings(roon, {
    get_settings: function(cb) {
        cb(makelayout(my_settings));
    },
    save_settings: function(req, isdryrun, settings) {
        let l = makelayout(settings.values);
        req.send_complete(l.has_error ? "NotValid" : "Success", { settings: l });

        if (!isdryrun && !l.has_error) {
            my_settings = l.values;
            svc_settings.update_settings(l);
            roon.save_config("settings", my_settings);
        }
    }
});

roon.init_services({
    required_services:   [ ],
    provided_services:   [ svc_settings ]
});

roon.start_discovery();

Extension 2:

"use strict";

var RoonApi          = require("node-roon-api"),
    RoonApiSettings  = require('node-roon-api-settings');

var core = undefined;

var roon = new RoonApi({
    extension_id:        'com.example.2',
    display_name:        'Example 2',
    display_version:     '0.1.0',
    publisher:           'Example',
    email:               'two@example.com',

    core_paired: function(core_) {
        core = core_;
    },
    core_unpaired: function(core_) {
        core = undefined;
    }
});

var my_settings = roon.load_config("settings") || {
};

function makelayout(settings) {
    let l = {
        values:    settings,
        layout:    [],
        has_error: false
    };

    l.layout.push({
        type:    "group",
        title:   "group1",
        items:   [{
            type:    "dropdown",
            title:   "Drop Down",
            values: [
                { "title": "Off", "value": 0 },
                { "title": "On",  "value": 1 }
            ],
            setting: "dropdown"
        }]
    });

    return l;
}

var svc_settings = new RoonApiSettings(roon, {
    get_settings: function(cb) {
        cb(makelayout(my_settings));
    },
    save_settings: function(req, isdryrun, settings) {
        let l = makelayout(settings.values);
        req.send_complete(l.has_error ? "NotValid" : "Success", { settings: l });

        if (!isdryrun && !l.has_error) {
            my_settings = l.values;
            svc_settings.update_settings(l);
            roon.save_config("settings", my_settings);
        }
    }
});

roon.init_services({
    required_services:   [ ],
    provided_services:   [ svc_settings ]
});

roon.start_discovery();

I think I have seen this before too. The fundamental bug is that the GUI layout code for the extension settings page is totally unhardened against invalid data, so any time something is missing or broken in the settings pages it crashes.

I’ll add this to our test cases at least, thanks for the clear report :slight_smile:

Do you see any invalid data in the given example code?

It might be that some property is missing. The difficulty with the settings API is that there is no documentation available, all possibilities have to be extracted from the Roon extensions. I made an attempt to collect all UI elements and their properties, both required and optional. It would be great if you can have a look at it and make it complete/fix errors.

I have searched for a switch element, like the one that’s used in the Roon UI, just by guessing a type (e.g. button, switch, toggle) but without success. Is there a switch available within the settings API?

Something like this:
switches

This is the UI element overview I created:

type type title subtitle values setting items min max max
length
collaps
able
needs_
feedback
zone R R O R
dropdown R R O R R O
integer R R O R O O
string R R O R O
group R R O R O
label R R O

R = Required, O = Optional

Looking at this crash more carefully, I think I agree that it’s a Roon client bug. I agree that there doesn’t seem to be a problem with the data, and the actual location of the crash is different from the ones I found.

Regarding the switch element question, here’s a slightly expanded table:

type type title setting values items min max max
length
collaps
able
group button_id
password R R R O
string R R R O
integer R R R O O
ip R R R
checkbox R R R
radio R R R R
dropdown R R R R
zone R R R
title R R
status R R
group R R R O
button R R R

R = Required, O = Optional

I am not sure what you mean by “needs_feedback” or “subtitle”. I also don’t know where you got the label element type.

I think the IP, Password, and String types are basically the same right now, and the “status” type also needs a “is_error” bool. I think the checkbox type does the thing you need.

2 Likes

Thanks Ben, very useful information!
I have some settings dialog rework to do :slight_smile:

I found them in roon-extension-iport-sm-buttons:

subtitle (line 77-84)

let v = {
    type:    "integer",
    min:     100,
    max:     2000,
    title:   "Long Press Timeout (milliseconds)",
    subtitle: "This is how long you have to hold the button down to register as a long press.",
    setting: "longpresstimeout",
};

needs_feedback (line 165-171)

ledgroup.items.push({
    type:           "dropdown",
    title:          "Music Playing Color",
    setting:        "led_playing_" + i,
    values:         colorvalues,
    needs_feedback: true
});

label (line 226-229)

group.items.push({
    type:    "label",
    title:   "Connecting to " + ip,
});

The label type is the only one I have used, it might be equal to a group without items.

I tried the checkbox by adding the following code to the makelayout function of extension 1:

l.layout.push({
    type:    "checkbox",
    title:   "Checkbox",
    setting: "checkbox"
});

Unfortunately only the title is shown, there is no switch:

Do you know what’s going on?

Hi @ben,

I got a report from a user about a crashing Roon Remote.

Is there any action going on to solve the Settings API issue?

Hi I tried the button to trigger a custom action, but it is not showing up on the extension menu.

items.push({
type:    "button",
title:   "Button ",
button_id: "pushButton",
});

I also tried an integer for the button_id.
The text is shown normally but no button is drawn.

Hi @ben, @danny,

Is the fixing of this crash still on your TODO list? Currently there is always the risk that two extensions are running side by side and the bug hits the user, causing a pretty bad user experience. It is also not very efficient if the individual extension developers have to implement a workaround for this.

I hope it can get some attention.

Has anyone discovered a way too make a group pre-expanded, but still collapsible?

On the subject of groups, it seems another way to crash the settings UI is as follows:

  • Create a non collapsible group, stick some drop downs in it.
  • In response to a change in a setting (outside the group), omit one of the drop downs from the group on the next save_settings with dryrun = true callback.

This seems to regularly crash the UI, OTOH if the groups are collapsible, then it works fine.

I searched for it but no luck :frowning:

Did anyone ever figure this out? I’m running into the same issue. @ben?

You need to look carefully at your own code to make sure that you are completing the callback properly and in a timely manner (including catching exceptions) and that the structure you are generating and passing back is correct and that names match up to supported variable types vs used widgets etc.

Most times when it crashes it is because I have done something wrong even if initially I thought all was good from my end.

There are at least a couple of extensions that come to mind (Deep Harmony and Extension Manager) that have huge and complex configurations that seem to work.

To be clear, I’m not having any crashing problems. I meant the comment I quoted – where the “checkbox” type doesn’t seem to work.

Sorry my bad - early morning and very little sleep :slight_smile:

As for checkboxes and some other more widgets I suspect exist - no - no idea :slight_smile:

Thanks anyway. I’ve just faked it with a “yes / no” drop-down for now. It’s good enough.

Unfortunately no checkbox, a drop-down is as fancy as it gets :frowning:

My Alarm Clock extension was always affected by this bug when a zone transfer action was selected, now I noticed that this no longer is the case with Roon 1.8. For that reason I repeated the steps that I described in the first post to see if that now works as well. Conclusion: it does!

So I think this bug is fixed. Always nice to discover a surprise in a release :slight_smile: