Merge lp:~dandrader/mir/switchingBundle_lp1270964 into lp:mir

[Daniel d'Andrada]

Only use SwitchingBundle::last_consumed after it has been set.
Otherwise SwitchingBundle::compositor_acquire could follow a bogus code path.
(LP: #1270964)

Fixes bug 1270964.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1342
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/689/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/677
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/673
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/279/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/419/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/423/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/279/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/689/rebuild

review: Needs Fixing (continuous-integration)

[Daniel van Vugt (vanvugt)]

(1) The new test fails:
[ RUN ] SwitchingBundleTest.compositor_client_interleaved
/home/dan/bzr/mir/tmp.964/tests/unit-tests/compositor/test_switching_bundle.cpp:389: Failure
Value of: bundle.first_ready
  Actual: 1
Expected: 0
[ FAILED ] SwitchingBundleTest.compositor_client_interleaved (0 ms)
The same failure is occurring on desktop amd64 and armhf (Nexus 10).

(2) I'm undecided about this approach:
47 + friend class SwitchingBundleTest_compositor_client_interleaved_Test;
It's certainly a convenient solution for introspection. But that then allows the test to couple to internal members it shouldn't be coupled to.

And before anyone asks, this has nothing to do with bug 1270245 (which still happens with this branch).

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

(2) Please remove "friend class ..." and all the ASSERT_EQ statements that depend on it. They're not needed to produce a correct regression test, as shown here:
  https://code.launchpad.net/~vanvugt/mir/test-1270964/+merge/202406
Feel free to merge that branch into this one.

review: Needs Fixing

[Daniel d'Andrada (dandrader)]

> (1) The new test fails:
> [ RUN ] SwitchingBundleTest.compositor_client_interleaved
> /home/dan/bzr/mir/tmp.964/tests/unit-
> tests/compositor/test_switching_bundle.cpp:389: Failure
> Value of: bundle.first_ready
> Actual: 1
> Expected: 0
> [ FAILED ] SwitchingBundleTest.compositor_client_interleaved (0 ms)
> The same failure is occurring on desktop amd64 and armhf (Nexus 10).

Sorry, I rushed with that proposal right before my EOD.

=== modified file 'src/server/compositor/switching_bundle.cpp'
--- src/server/compositor/switching_bundle.cpp	2014-01-20 16:40:37 +0000
+++ src/server/compositor/switching_bundle.cpp	2014-01-21 12:34:43 +0000
@@ -77,6 +77,7 @@
       first_client{0}, nclients{0},
       snapshot{-1}, nsnapshotters{0},
       last_consumed{0},
+      compositor_ever_acquired{false},
       overlapping_compositors{false},
       framedropping{false}, force_drop{0}
 {
@@ -273,7 +274,7 @@
     int compositor;
 
     // Multi-monitor acquires close to each other get the same frame:
-    bool same_frame = (frameno == last_consumed);
+    bool same_frame = compositor_ever_acquired && (frameno == last_consumed);
 
     int avail = nfree();
     bool can_recycle = ncompositors || avail;
@@ -303,6 +304,7 @@
         nready--;
         ncompositors++;
         last_consumed = frameno;
+        compositor_ever_acquired = true;
     }
 
     overlapping_compositors = (ncompositors > 1);

=== modified file 'src/server/compositor/switching_bundle.h'
--- src/server/compositor/switching_bundle.h	2014-01-13 06:12:33 +0000
+++ src/server/compositor/switching_bundle.h	2014-01-21 12:34:43 +0000
@@ -98,7 +98,10 @@
     mutable std::mutex guard;
     std::condition_variable cond;
 
+    // the last frame acquired by the compositor. its value is invalid
+    // if compositor_ever_acquired is false
     unsigned long last_consumed;
+    bool compositor_ever_acquired;
 
     bool overlapping_compositors;
 

=== modified file 'tests/unit-tests/compositor/test_switching_bundle.cpp'
--- tests/unit-tests/compositor/test_switching_bundle.cpp	2014-01-13 06:12:33 +0000
+++ tests/unit-tests/compositor/test_switching_bundle.cpp	2014-01-21 12:34:43 +0000
@@ -329,6 +329,38 @@
     }
 }
 
+/*
+ Regression test for LP#1270964
+ In the situation emulated here SwitchingBundle::last_consumed ise used without ever being set,
+ thus its initial value of zero will wrongly be used and lead to bogus behavior.
+ */
+TEST_F(SwitchingBundleTest, compositor_client_interleaved)
+{
+    int nbuffers = 3;
+    mc::SwitchingBundle bundle(nbuffers, allocator, basic_properties);
+    mg::Buffer* client_buffer = nullptr;
+
+    client_buffer = bundle.client_acquire();
+
+    bundle.client_release(client_buffer);
+
+    client_buffer = bundle.client_acquire();
+
+    bundle.compositor_acquire(0);
+
+    //(nbufs=3,fcomp=2,ncomp=1,fready=0,nready=1,fclient=1,nclient=1) <-- BUG starts here
+    // Should be:
+    //(nbufs=3,fcomp=0,ncomp=1,fready=0,nready=0,fclient=1,nclient=1)
+
+    bundle.client_release(client_buffer);
+
+    //(nbufs=3,fcomp=2,ncomp=1,fready=0,nready=2,fclient=2,nclient=0) 
+    // Should be:
+    //(nbufs=3,fcomp=0,ncomp=1,fready=1,nready=1,fclient=2,nclient=0)
+
+    client_buffer = bundle.client_acquire(); // <- would lock here if in buggy state
+}
+
 TEST_F(SwitchingBundleTest, overlapping_compositors_get_different_frames)
 {
     // This test simulates bypass behaviour
@@ -868,4 +900,3 @@
         }
     }
 }
-