Activity log for bug #1477430

Date Who What changed Old value New value Message
2015-07-23 07:41:55 Daniel van Vugt bug added bug
2015-07-23 07:42:36 Daniel van Vugt description MirSurfaceItem::dropPendingBuffers contains a potentially infinite/indefinite loop: void MirSurfaceItem::dropPendingBuffers() { QMutexLocker locker(&m_mutex); const void* const userId = (void*)123; // TODO: Multimonitor support while (m_surface->buffers_ready_for_compositor(userId) > 0) { // The line below looks like an innocent, effect-less, getter. But as this // method returns a unique_pointer, not holding its reference causes the // buffer to be destroyed/released straight away. m_surface->compositor_snapshot(userId)->buffer(); qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::dropPendingBuffers()" << "surface =" << this << "buffer dropped." << m_surface->buffers_ready_for_compositor(userId) << "left."; } } The issue is the value of buffers_ready_for_compositor() could theoretically grow just as quickly as you're consuming the buffers. So that loop has no definite number of iterations before exiting. The simple solution is to check buffers_ready_for_compositor() once, save the result and use a for loop up to that limit. However event that's an overkill. If you look at the purpose of dropPendingBuffers, it's a misnomer and it should really be "dropPendingBuffer" as it only really needs to consume a single frame to achieve its goal; no loop required. MirSurfaceItem::dropPendingBuffers contains a potentially infinite/indefinite loop: void MirSurfaceItem::dropPendingBuffers() {     QMutexLocker locker(&m_mutex);     const void* const userId = (void*)123; // TODO: Multimonitor support     while (m_surface->buffers_ready_for_compositor(userId) > 0) {         // The line below looks like an innocent, effect-less, getter. But as this         // method returns a unique_pointer, not holding its reference causes the         // buffer to be destroyed/released straight away.         m_surface->compositor_snapshot(userId)->buffer();         qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::dropPendingBuffers()"             << "surface =" << this             << "buffer dropped."             << m_surface->buffers_ready_for_compositor(userId)             << "left.";     } } The issue is the value of buffers_ready_for_compositor() could theoretically grow just as quickly as you're consuming the buffers. So that loop has no definite number of iterations before exiting. The simple solution is to check buffers_ready_for_compositor() once, save the result and use a for loop up to that limit. However even that's an overkill. If you look at the purpose of dropPendingBuffers, it's a misnomer and it should really be "dropPendingBuffer" as it only really needs to consume a single frame to achieve its goal; no loop required.
2015-07-23 07:43:25 Daniel van Vugt bug task added qtmir (Ubuntu)
2015-07-23 07:44:20 Daniel van Vugt description MirSurfaceItem::dropPendingBuffers contains a potentially infinite/indefinite loop: void MirSurfaceItem::dropPendingBuffers() {     QMutexLocker locker(&m_mutex);     const void* const userId = (void*)123; // TODO: Multimonitor support     while (m_surface->buffers_ready_for_compositor(userId) > 0) {         // The line below looks like an innocent, effect-less, getter. But as this         // method returns a unique_pointer, not holding its reference causes the         // buffer to be destroyed/released straight away.         m_surface->compositor_snapshot(userId)->buffer();         qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::dropPendingBuffers()"             << "surface =" << this             << "buffer dropped."             << m_surface->buffers_ready_for_compositor(userId)             << "left.";     } } The issue is the value of buffers_ready_for_compositor() could theoretically grow just as quickly as you're consuming the buffers. So that loop has no definite number of iterations before exiting. The simple solution is to check buffers_ready_for_compositor() once, save the result and use a for loop up to that limit. However even that's an overkill. If you look at the purpose of dropPendingBuffers, it's a misnomer and it should really be "dropPendingBuffer" as it only really needs to consume a single frame to achieve its goal; no loop required. MirSurfaceItem::dropPendingBuffers contains a potentially infinite/indefinite loop: void MirSurfaceItem::dropPendingBuffers() {     QMutexLocker locker(&m_mutex);     const void* const userId = (void*)123; // TODO: Multimonitor support     while (m_surface->buffers_ready_for_compositor(userId) > 0) {         // The line below looks like an innocent, effect-less, getter. But as this         // method returns a unique_pointer, not holding its reference causes the         // buffer to be destroyed/released straight away.         m_surface->compositor_snapshot(userId)->buffer();         qCDebug(QTMIR_SURFACES) << "MirSurfaceItem::dropPendingBuffers()"             << "surface =" << this             << "buffer dropped."             << m_surface->buffers_ready_for_compositor(userId)             << "left.";     } } The issue is the value of buffers_ready_for_compositor() could theoretically grow just as quickly as you're consuming the buffers. So that loop has no definite number of iterations before exiting. The simple solution is to check buffers_ready_for_compositor() once, save the result and use a for loop up to that limit. However even that's an overkill. If you look at the purpose of dropPendingBuffers, it's a misnomer and it should really be "dropPendingBuffer" as it only really needs to consume a single frame to achieve its goal of unblocking the client. No loop required.
2015-07-23 14:51:36 Daniel d'Andrada bug added subscriber Daniel d'Andrada
2015-07-24 07:31:48 Daniel van Vugt qtmir: assignee Daniel van Vugt (vanvugt)
2015-07-24 07:31:52 Daniel van Vugt qtmir: status New In Progress
2015-07-24 07:50:49 Daniel van Vugt branch linked lp:~vanvugt/qtmir/smoother-mirsurfaceitem
2015-07-28 08:45:46 Daniel van Vugt branch linked lp:~vanvugt/qtmir/fix-1477430
2015-07-28 08:47:54 Daniel van Vugt qtmir: importance Undecided Medium
2015-07-28 08:47:57 Daniel van Vugt qtmir (Ubuntu): importance Undecided Medium
2015-08-21 03:30:15 Daniel van Vugt qtmir: status In Progress Fix Committed
2015-08-24 05:06:13 Launchpad Janitor qtmir (Ubuntu): status New Fix Released
2015-09-02 14:57:51 Gerry Boland qtmir: status Fix Committed Fix Released
2017-03-13 17:35:15 Michał Sawicz qtmir (Ubuntu): assignee Daniel van Vugt (vanvugt)
2017-03-13 17:35:23 Michał Sawicz bug task deleted qtmir