Potentially unsafe iteration in SurfaceStack (if used wrong)

[Chris Halse Rogers]

RecursiveReadLocks don't exclude modification by the same thread.

This means that code like:

void ms::SurfaceStack::for_each(std::function<void(std::shared_ptr<mi::Surface> const&)> const& callback)
{
    RecursiveReadLock lg(guard);
    for (auto &surface : surfaces)
    {
        if (surface->query(mir_surface_attrib_visibility) ==
            MirSurfaceVisibility::mir_surface_visibility_exposed)
        {
            callback(surface);
        }
    }
}

is unsafe; if the callback calls into SurfaceStack::add_surface or SurfaceStack::remove_surface it will happily take a RecursiveWriteLock and mutate the surfaces vector, possibly invalidating the iterators.

[Daniel van Vugt (vanvugt)]

Yes; in theory. In practice it might be simpler to just document that callbacks should not modify the list.

To iterate efficiently and support callbacks that can delete entries you ideally want a callback that returns a flag to tell the loop to do the deletion (and update the iterator appropriately).

[Michał Sawicz (saviq)]

Syncing task from Mir.