BasicSurface: allow frame to be posted after surface destroyed

[wmww]

Reading through the code, it looks like setting all buffer stream callbacks to [](){} is allowed (that's what we already do in BasicSurface::set_streams(), and why shouldn't it be?). If an noop callback is allowed, a conditionally noop callback should also be allowed. I see no reason to throw this error, as it is perfectly reasonable for a surface to be destroyed before its buffer streams.

Fixes #1272

[wmww]

On second thought you should probably wait for me to write some tests

[AlanGriffiths]

It is only two months since I introduced that code, there was a reason for it.

(I think I encountered a zombie client that spammed the server. Let me dig...)

The bug was #1106, the change was #1110

[wmww]

Oh, I didn't notice the usage of this. However, I don't understand how it was any safer before this PR. Did throwing the error prevent a possible future crash?

[AlanGriffiths]

Oh, I didn't notice the usage of this. However, I don't understand how it was any safer before this PR. Did throwing the error prevent a possible future crash?

Throwing disconnected glmark and that stopped it spamming us with unwanted buffers.

[wmww]

Hmm.. I believe a fully safe solution would be to add

for(auto& layer : layers)
    layer.stream->set_frame_posted_callback([](auto){});

to the destructor. The only issue I can imagine would be it would dead lock if the surface is destroyed from a callback. I will push this along with some tests in a bit.

(Before you ask or have to go check, yes, mc::Stream::submit_buffer() does call the callback under lock)

[AlanGriffiths]

After discussion, this is correct, but not all that is needed.

[wmww]

This is now ready to merge. I'll let someone else bors after they check the tests are sensible.

[AlanGriffiths]

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push