Merge lp:~compiz-team/compiz/compiz.fix_1015151.1 into lp:compiz/0.9.8

Proposed by Sam Spilsbury on 2012-06-20
Status: Merged
Merged at revision: 3255
Proposed branch: lp:~compiz-team/compiz/compiz.fix_1015151.1
Merge into: lp:compiz/0.9.8
Diff against target: 25 lines (+5/-3)
1 file modified
plugins/wall/src/offset_movement/src/offset-movement.cpp (+5/-3)
To merge this branch: bzr merge lp:~compiz-team/compiz/compiz.fix_1015151.1
Reviewer Review Type Date Requested Status
Daniel van Vugt 2012-06-20 Approve on 2012-06-20
Review via email: mp+111149@code.launchpad.net

This proposal supersedes a proposal from 2012-06-20.

Description of the change

== Problem ==

Crash in compiz::wall::movementWindowOnScreen (const CompRect &serverBorderRect, const CompRegion &screenRegion) because we were iterating a temporary CompRect::vector which went out of scope within the for statement.

(LP: #1015151)

== Solution ==

Copy the vector, then iterate it.

== Test ==

Covered already, but this wasn't caught by the testsuite as I had to use valgrind to find the memory error. we should run tests under valgrind in CI to catch these ones in future.

To post a comment you must log in.
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Diff, since bzr push :parent bugged out and pushed to lp:compiz (reverted in r3252)

=== modified file 'plugins/wall/src/offset_movement/src/offset-movement.cpp'
--- plugins/wall/src/offset_movement/src/offset-movement.cpp 2012-06-19 23:36:57 +0000
+++ plugins/wall/src/offset_movement/src/offset-movement.cpp 2012-06-19 23:42:45 +0000
@@ -28,7 +28,7 @@
 compiz::wall::movementWindowOnScreen (const CompRect &serverBorderRect,
           const CompRegion &screenRegion)
 {
- CompRegion sbrRegion (serverBorderRect);
+ CompRegion sbrRegion (static_cast <const CompRect &> (serverBorderRect));

     /* If the window would be partially offscreen
      * after it was moved then we should move it back
@@ -41,10 +41,8 @@
     int dx = 0;
     int dy = 0;

- CompRect::vector rects (rem.rects ());
-
- for (std::vector <CompRect>::const_iterator it = rects.begin ();
- it != rects.end ();
+ for (std::vector <CompRect>::const_iterator it = rem.rects ().begin ();
+ it != rem.rects ().end ();
   it++)
     {
  const CompRect &r = *it;

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Correction:

=== modified file 'plugins/wall/src/offset_movement/src/offset-movement.cpp'
--- plugins/wall/src/offset_movement/src/offset-movement.cpp 2012-06-19 23:42:45 +0000
+++ plugins/wall/src/offset_movement/src/offset-movement.cpp 2012-06-19 23:35:09 +0000
@@ -28,7 +28,7 @@
 compiz::wall::movementWindowOnScreen (const CompRect &serverBorderRect,
           const CompRegion &screenRegion)
 {
- CompRegion sbrRegion (static_cast <const CompRect &> (serverBorderRect));
+ CompRegion sbrRegion (serverBorderRect);

     /* If the window would be partially offscreen
      * after it was moved then we should move it back
@@ -41,8 +41,10 @@
     int dx = 0;
     int dy = 0;

- for (std::vector <CompRect>::const_iterator it = rem.rects ().begin ();
- it != rem.rects ().end ();
+ CompRect::vector rects (rem.rects ());
+
+ for (std::vector <CompRect>::const_iterator it = rects.begin ();
+ it != rects.end ();
   it++)
     {
  const CompRect &r = *it;

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Empty diff?!

review: Resubmit
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Thanks to a launchpad bug, the diff is going to be empty anyways. You'll need to look at the revision history.

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Please try resubmitting from a different branch name. That might work around the launchpad issue.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

It wont. The problem is the revisions themselves.

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

Please try resubmitting from a different branch name. That might work around the launchpad issue.
--
https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1015151/+merge/111140
Your team Compiz Maintainers is subscribed to branch lp:compiz.

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

The diff should be fixed now

Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

Since rects is now outside the for loop, you can safely convert it to a reference:
  CompRect::vector &rects (rem.rects ());

Then it becomes a reference to a temporary that is not destroyed till the end of the block/function. And that way only 2 temporary vectors are created (one inside rects() and one outside). In its current form, 3 temporary vectors could be created. Though its possible gcc will sometimes optimize that to 2...

review: Needs Fixing
Daniel van Vugt (vanvugt) wrote : Posted in a previous version of this proposal

BTW, if performance is important and you want to avoid temporary container construction/copying/destruction, then consider API design that avoids temporaries in future:

  void CompRegion::getRects(std::vector <CompRect> &r);

Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Thanks, TIL

Daniel van Vugt (vanvugt) wrote :

Looks good. And WallOffsetMovementTest no longer crashes under:
  ctest -D ExperimentalMemCheck

:)

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'plugins/wall/src/offset_movement/src/offset-movement.cpp'
2--- plugins/wall/src/offset_movement/src/offset-movement.cpp 2012-06-19 23:42:45 +0000
3+++ plugins/wall/src/offset_movement/src/offset-movement.cpp 2012-06-20 06:36:21 +0000
4@@ -28,7 +28,7 @@
5 compiz::wall::movementWindowOnScreen (const CompRect &serverBorderRect,
6 const CompRegion &screenRegion)
7 {
8- CompRegion sbrRegion (static_cast <const CompRect &> (serverBorderRect));
9+ CompRegion sbrRegion (serverBorderRect);
10
11 /* If the window would be partially offscreen
12 * after it was moved then we should move it back
13@@ -41,8 +41,10 @@
14 int dx = 0;
15 int dy = 0;
16
17- for (std::vector <CompRect>::const_iterator it = rem.rects ().begin ();
18- it != rem.rects ().end ();
19+ const CompRect::vector &rects (rem.rects ());
20+
21+ for (std::vector <CompRect>::const_iterator it = rects.begin ();
22+ it != rects.end ();
23 it++)
24 {
25 const CompRect &r = *it;

Subscribers

People subscribed via source and target branches