ping! This is a crash fix.

Isn't the real bug from that trace here:

datadevice_interface.cpp:204
void DataDeviceInterface::sendSelection(DataDeviceInterface *other)
d->createDataOffer(other->selection());

it's valid for other->selection() to be null, so this should be guarded there. If I do that your modified unit test passes.

Guarding inside the method I fear will mask future bugs.

In D7078#134111, @davidedmundson wrote:

Isn't the real bug from that trace here:

datadevice_interface.cpp:204
void DataDeviceInterface::sendSelection(DataDeviceInterface *other)
d->createDataOffer(other->selection());

it's valid for other->selection() to be null, so this should be guarded there. If I do that your modified unit test passes.

Guarding inside the method I fear will mask future bugs.

Guarding there would be result in incorrect state. We went into sendSelection because there is a selection. That the source is null doesn't really matter. But we have to inform the DDI which gets the selection which selection there is. If not it still operates on outdated data. So I thought it's better to continue there and ensure the DataOffer is created.

I don't think we did go in there because we have a selection, otherwise other->selection() wouldn't be null.

If we want DDI::sendSelection(DDI *other) to always keep the client in sync we should do:

auto selection = other->selection();
if (!selection) {
    sendClearSelection();
    return;
}

the send_selection spec says we should be passing a null resource if we have no selection, whereas this version creates a data offer with nothing in it.

> the send_selection spec says we should be passing a null resource if we have no selection, whereas this version creates a data offer with nothing in it.

But we have a selection, that's the point. We just don't have a datasource on the selection.

Relevant code in KWin which goes into the code path:

if (selection) {
                     xclipboard->sendSelection(selection);
                 } else {
                     xclipboard->sendClearSelection();
                 }

We do check whether we have a selection. So from KWayland Server side we have a selection. If we would not have a Selection it would go into sendClearSelection.

That we don't send any useful DataOffer doesn't really matter. We also don't check whether the DataSource has any offers.

So from KWayland Server side we have a selection

from KWayland server side we have a Seat:selection, we don't check if that has a DDI::selection

If a client had called wl_set_selection(dataDevice, nulltptr) the server would still have a registered DDI.

If the client has called clearSelection to us, I think we should be sending clearSelection to X.

In D7078#135036, @davidedmundson wrote:

So from KWayland Server side we have a selection

from KWayland server side we have a Seat:selection, we don't check if that has a DDI::selection

---

If a client had called wl_set_selection(dataDevice, nulltptr) the server would still have a registered DDI.

If the client has called clearSelection to us, I think we should be sending clearSelection to X.

of course we do that :-)

Anyway, I would like to move further with that. This is a nullptr access fix which makes sense IMHO. If you think that we should not even send out the DataOffer I think that's a separate thing, which could be done in addition. So my suggestion would be to merge in this crash fix and in addition do the change you suggest.

Please note that I doubt that your suggestion would help against the crash I fixed here. It fixes the autotest, but the autotest does not 100 % match the crash case we had in KWin. In KWin the crash was caused by an application crashing. When creating the autotest I simplified it to the common pattern. If we go for your change only the test would have to be adjusted to properly cover the situation from KWin.

But what is point to have a DataOfferInterface without valid source?

of course we do that :-)

We're not.

Kwin does not check if a client has a selection.
It merely checks if the client has an interface from a selection can be fetched, it does not check that device actually has a valid selection.

you think that we should not even send out the DataOffer I think that's a separate thing,

No, that's not what I think.
We're getting crosswired. I'll submit my coment as a patch, so maybe you'll understand what I'm trying to say.

Please note that I doubt that your suggestion would help against the crash I fixed here.

If your patch works, then mine would too. It's checking the exact same pointer, just one frame earlier.

If your patch works, then mine would too. It's checking the exact same pointer, just one frame earlier.

But then it's an easy thing: let's do both. Your patch and mine.

https://phabricator.kde.org/D7316 has been committed, and the referenced bug marked as fixed.

Reading above comments, this patch can/should be committed, too. Please check if this is still true, and either approve this patch, or discard it.

I'm still in favor of pushing this one too.

I'm still against it.
Passing unexpectedly invalid pointers about all over the place and then trying to guard them afterwards is an anti pattern that leads to future problems.