Merge lp:~thomas-voss/unity-mir/replace_get_env_with_thread_safe_variant into lp:unity-mir

[Thomas Voß]

Switch to process-cpp for querying the environment in a thread-safe manner.

Switch to process-cpp for querying the environment in a thread-safe manner.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:145
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/154/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/15/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/15/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/15/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-mir-ci/154/rebuild

review: Needs Fixing (continuous-integration)

[lp:~thomas-voss/unity-mir/replace_get_env_with_thread_safe_variant]

146. By Thomas Voß on 2013-11-26

Leverage default values for env::get().

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:146
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/166/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/27/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/27/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/27/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/166/rebuild

review: Needs Fixing (continuous-integration)

[Michał Sawicz (saviq)]

As discussed on IRC, posix::this_process::env would give us false confidence that we're safe, when in fact any non-wrapped call to setenv in a library, for example, would result in a segfault just the same.

There's some other changes here that still make sense, please extract them or revert the env-related changes alone.

review: Needs Fixing

[lp:~thomas-voss/unity-mir/replace_get_env_with_thread_safe_variant]

147. By Thomas Voß on 2013-12-05

Merged dependent branches.

148. By Thomas Voß on 2013-12-05

Reverted changes to getenv.

149. By Thomas Voß on 2013-12-05

Ensure that panel height is adjusted for grid unit pixel units.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:149
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/177/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/40/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/41/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/40/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/177/rebuild

review: Needs Fixing (continuous-integration)

[lp:~thomas-voss/unity-mir/replace_get_env_with_thread_safe_variant]

150. By Thomas Voß on 2013-12-09

[ Gerry Boland ]
* Unity.Application is not a shared library, but a plugin. (LP:
  #1256014)
* Implement preStart callback added to upstart-app-launch-2. (LP:
  #1243665)
[ Ubuntu daily release ]
* Automatic snapshot from revision 157
[ Gerry Boland ]
* Install ServerStatusListener to be notified of mir server start, pause
  and resume. Use start notification to send SIGSTOP signal to upstart,
  so it knows mir is ready for other clients.
* Bump version number
* InputArea: don't use lambda function as slot, can cause crash on
  shutdown Using lambda function as slot can introduce crash as the
  slot's object deletion is not registered unlike with traditional
  signal/slot connections. As a result, on signal emission, the lambda
  can be called on a deleted object. (LP: #1253685)
* Misc fixes for Mir 0.1.2 support. (LP: #1254986)
[ Alan Griffiths ]
* ApplicationSession is a Mir implementation class that shouldn't be
  used by unity-mir, use shell::session instead.
[ Kevin Gunn ]
* mir server abi and api broke, updating dependency to deb 0.1.1.
* update mir deb build dep to 0.1.2
[ Victor Thompson ]
* Fix mir to not suspend the music-app, or any other app granted a
  lifecycle exception, when switching between apps. (LP: #1241185,
  #1241045)
[ Daniel van Vugt ]
* Force the screen to redraw after turning the display back on (LP:
  #1255045). Also stop the compositor when the screen is off.
  Otherwise it will spin in the background, eating battery. (LP:
  #1255045)
[ Ubuntu daily release ]
* Automatic snapshot from revision 154
* Cherry-pick upstream patch to avoid Unity8 crashing on stop
  (LP: #1253685)
[ Gerry Boland ]
* On MirSurface destruction, any InputAreas on that surface will be
  notified and remove their links to that surface. Fixes crash
  bug:1243444. (LP: #1243444)
[ Alan Griffiths ]
* Remove dependency on mir::shell::SessionManager.
* Remove dependency on mir::surfaces::SurfaceController.
* Remove dependency on msh::OrganisingSurfaceFactory.
* Avoid relying on an explicit Mir typename that changes in a
  refactoring coming soon.
* Do all the hacky surface creation customization in
  SurfaceFactory::create_surface() - and don't mention SurfaceBuilder
  at all. (That will allow Mir to get rid of that interface.).
[ Albert Astals ]
* Don't include the QtQml megaheader Include the one we really need.
[ Ubuntu daily release ]
* Automatic snapshot from revision 144

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:150
http://jenkins.qa.ubuntu.com/job/unity-mir-ci/180/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-amd64-ci/43/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-armhf-ci/44/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-mir-trusty-i386-ci/43/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-mir-ci/180/rebuild

review: Needs Fixing (continuous-integration)

[Gerry Boland (gerboland)]

Is this MR still relevant?

review: Needs Information

[Michał Sawicz (saviq)]

No, we've discussed and decided it's a trap.

review: Disapprove

Unmerged revisions

150. By Thomas Voß on 2013-12-09

[ Gerry Boland ]
* Unity.Application is not a shared library, but a plugin. (LP:
  #1256014)
* Implement preStart callback added to upstart-app-launch-2. (LP:
  #1243665)
[ Ubuntu daily release ]
* Automatic snapshot from revision 157
[ Gerry Boland ]
* Install ServerStatusListener to be notified of mir server start, pause
  and resume. Use start notification to send SIGSTOP signal to upstart,
  so it knows mir is ready for other clients.
* Bump version number
* InputArea: don't use lambda function as slot, can cause crash on
  shutdown Using lambda function as slot can introduce crash as the
  slot's object deletion is not registered unlike with traditional
  signal/slot connections. As a result, on signal emission, the lambda
  can be called on a deleted object. (LP: #1253685)
* Misc fixes for Mir 0.1.2 support. (LP: #1254986)
[ Alan Griffiths ]
* ApplicationSession is a Mir implementation class that shouldn't be
  used by unity-mir, use shell::session instead.
[ Kevin Gunn ]
* mir server abi and api broke, updating dependency to deb 0.1.1.
* update mir deb build dep to 0.1.2
[ Victor Thompson ]
* Fix mir to not suspend the music-app, or any other app granted a
  lifecycle exception, when switching between apps. (LP: #1241185,
  #1241045)
[ Daniel van Vugt ]
* Force the screen to redraw after turning the display back on (LP:
  #1255045). Also stop the compositor when the screen is off.
  Otherwise it will spin in the background, eating battery. (LP:
  #1255045)
[ Ubuntu daily release ]
* Automatic snapshot from revision 154
* Cherry-pick upstream patch to avoid Unity8 crashing on stop
  (LP: #1253685)
[ Gerry Boland ]
* On MirSurface destruction, any InputAreas on that surface will be
  notified and remove their links to that surface. Fixes crash
  bug:1243444. (LP: #1243444)
[ Alan Griffiths ]
* Remove dependency on mir::shell::SessionManager.
* Remove dependency on mir::surfaces::SurfaceController.
* Remove dependency on msh::OrganisingSurfaceFactory.
* Avoid relying on an explicit Mir typename that changes in a
  refactoring coming soon.
* Do all the hacky surface creation customization in
  SurfaceFactory::create_surface() - and don't mention SurfaceBuilder
  at all. (That will allow Mir to get rid of that interface.).
[ Albert Astals ]
* Don't include the QtQml megaheader Include the one we really need.
[ Ubuntu daily release ]
* Automatic snapshot from revision 144

149. By Thomas Voß on 2013-12-05

Ensure that panel height is adjusted for grid unit pixel units.

148. By Thomas Voß on 2013-12-05

Reverted changes to getenv.

147. By Thomas Voß on 2013-12-05

Merged dependent branches.

146. By Thomas Voß on 2013-11-26

Leverage default values for env::get().

145. By Thomas Voß on 2013-11-13

Switch to process-cpp for querying the environment in a thread-safe manner.

144. By Thomas Voß on 2013-11-12

Refactor sigstop'ing and sigcont'ing of application process groups to rely on process-cpp.

=== modified file 'src/modules/Unity/Application/applicationscreenshotprovider.cpp'
--- src/modules/Unity/Application/applicationscreenshotprovider.cpp	2013-11-26 10:32:56 +0000
+++ src/modules/Unity/Application/applicationscreenshotprovider.cpp	2013-12-09 16:43:13 +0000
@@ -25,6 +25,9 @@
 // mir
 #include "mirserver/mir/shell/session.h"
 
+// STL
+#include <sstream>
+
 // fallback grid unit used if GRID_UNIT_PX is not in the environment.
 const int defaultGridUnitPx = 8;
 
@@ -46,7 +49,6 @@
     }
 
     int densityPixelPx = qFloor( (float)gridUnitPx / defaultGridUnitPx );
-
     m_panelHeight = 3 * gridUnitPx + 2 * densityPixelPx;
 }
 
@@ -57,7 +59,7 @@
 }
 
 QImage ApplicationScreenshotProvider::requestImage(const QString &appId, QSize * size,
-                                                     const QSize &requestedSize)
+                                                   const QSize &requestedSize)
 {
     DLOG("ApplicationScreenshotProvider::requestPixmap (this=%p, id=%s)", this, appId.toLatin1().constData());
 
@@ -95,17 +97,17 @@
     QImage image;
 
     app->session()->take_snapshot(
-        [&](mir::shell::Snapshot const& snapshot)
-        {
-            DLOG("ApplicationScreenshotProvider - Mir snapshot ready with size %d x %d",
-                 snapshot.size.height.as_int(), snapshot.size.width.as_int());
+                [&](mir::shell::Snapshot const& snapshot)
+    {
+        DLOG("ApplicationScreenshotProvider - Mir snapshot ready with size %d x %d",
+             snapshot.size.height.as_int(), snapshot.size.width.as_int());
 
-            image = QImage( (const uchar*)snapshot.pixels, // since we mirror, no need to offset starting position
-                            snapshot.size.width.as_int(),
-                            snapshot.size.height.as_int() - yOffset,
-                            QImage::Format_ARGB32_Premultiplied).mirrored();
-            wait.wakeOne();
-        });
+        image = QImage( (const uchar*)snapshot.pixels, // since we mirror, no need to offset starting position
+                        snapshot.size.width.as_int(),
+                        snapshot.size.height.as_int() - yOffset,
+                        QImage::Format_ARGB32_Premultiplied).mirrored();
+        wait.wakeOne();
+    });
 
     wait.wait(&mutex, 5000);
 

=== modified file 'src/modules/Unity/Application/taskcontroller.cpp'
--- src/modules/Unity/Application/taskcontroller.cpp	2013-12-09 16:43:13 +0000
+++ src/modules/Unity/Application/taskcontroller.cpp	2013-12-09 16:43:13 +0000
@@ -45,14 +45,12 @@
 
 namespace
 {
-void ensureProcessIsUnlikelyToBeKilled(pid_t pid)
+void ensureProcessIsUnlikelyToBeKilled(core::posix::Process process)
 {
     // By system default, we set the oom_score_adj of Unity8 to -10 (via lightdm).
     // As we want to avoid that any app's oom_score_adj is <= Unity8's oom_score_adj,
     // we choose a default increase of +1.
     static const int default_increase = 1;
-    
-    core::posix::Process process(pid);
 
     try
     {
@@ -84,14 +82,12 @@
     }
 }
 
-void ensureProcessIsLikelyToBeKilled(pid_t pid)
+void ensureProcessIsLikelyToBeKilled(core::posix::Process process)
 {
     // We avoid extremal values for oom_score_adj. For that, we
     // set it to 80% of the total available range.
     static const float defaultPercentage = 0.8;
 
-    core::posix::Process process(pid);
-
     try
     {
         plpp::OomScoreAdj shellScore;
@@ -150,7 +146,15 @@
     focusCallback = [](const gchar * appId, gpointer userData) {
         Q_UNUSED(userData)
         pid_t pid = upstart_app_launch_get_primary_pid(appId);
-        ensureProcessIsUnlikelyToBeKilled(pid);
+
+        try
+        {
+            ensureProcessIsUnlikelyToBeKilled(core::posix::Process{pid});
+        } catch(const std::runtime_error&)
+        {
+            LOG("TaskController::startCallback: pid queried from upstart app launch is invalid.");
+        }
+
         Q_EMIT TaskController::singleton()->requestFocus(QString(appId));
     };
 
@@ -232,17 +236,32 @@
     DLOG("TaskController::suspend (this=%p, application=%p)", this, qPrintable(appId));
     pid_t pid = upstart_app_launch_get_primary_pid(appId.toLatin1().constData());
 
-    ensureProcessIsLikelyToBeKilled(pid);
-
-    if (pid) {
-        // We do assume that the app was launched by upstart and with that,
-        // in its own process group. For that, we interpret the pid as pgid and
-        // sigstop the complete process group on suspend.
-        kill(-pid, SIGSTOP);
-        return true;
-    } else {
-        return false;
-    }
+    core::posix::Process process = core::posix::Process::invalid();
+    try
+    {
+        process = core::posix::Process{pid};
+    } catch(const std::runtime_error&)
+    {
+        // If the pid reported by upstart app launch is invalid, we error out here.
+        LOG("TaskController::suspend: pid queried from upstart app launch is invalid.");
+        return false;
+    }
+
+    ensureProcessIsLikelyToBeKilled(process);
+
+    // We do assume that the app was launched by upstart and with that,
+    // in its own process group.
+    try
+    {
+        auto pg = process.process_group_or_throw();
+        pg.send_signal_or_throw(core::posix::Signal::sig_stop);
+    } catch(const std::system_error& e)
+    {
+        LOG("TaskController::suspend: Exception sig-stop'ing process group (%s)", e.what());
+        return false;
+    }
+
+    return true;
 }
 
 bool TaskController::resume(const QString& appId)
@@ -250,15 +269,30 @@
     DLOG("TaskController::resume (this=%p, application=%p)", this, qPrintable(appId));
     pid_t pid = upstart_app_launch_get_primary_pid(appId.toLatin1().constData());
 
-    ensureProcessIsUnlikelyToBeKilled(pid);
-
-    if (pid) {
-        // We do assume that the app was launched by upstart and with that,
-        // in its own process group. For that, we interpret the pid as pgid and
-        // sigcont the complete process group on resume.
-        kill(-pid, SIGCONT);
-        return true;
-    } else {
-        return false;
-    }
+    core::posix::Process process = core::posix::Process::invalid();
+    try
+    {
+        process = core::posix::Process{pid};
+    } catch(const std::runtime_error&)
+    {
+        // If the pid reported by upstart app launch is invalid, we error out here.
+        LOG("TaskController::resume: pid queried from upstart app launch is invalid.");
+        return false;
+    }
+
+    ensureProcessIsUnlikelyToBeKilled(process);
+
+    // We do assume that the app was launched by upstart and with that,
+    // in its own process group.
+    try
+    {
+        auto pg = process.process_group_or_throw();
+        pg.send_signal_or_throw(core::posix::Signal::sig_cont);
+    } catch(const std::system_error& e)
+    {
+        LOG("TaskController::resume: Exception sig-continue'ing process group (%s)", e.what());
+        return false;
+    }
+
+    return true;
 }

=== modified file 'src/modules/Unity/Application/ubuntukeyboardinfo.cpp'
--- src/modules/Unity/Application/ubuntukeyboardinfo.cpp	2013-10-14 18:39:55 +0000
+++ src/modules/Unity/Application/ubuntukeyboardinfo.cpp	2013-12-09 16:43:13 +0000
@@ -19,16 +19,16 @@
 #include <QDir>
 
 namespace {
-    const int gConnectionAttemptIntervalMs = 5000;
-    const int gMaxConsecutiveAttempts = 10;
-    const char gServerName[] = "ubuntu-keyboard-info";
+const int gConnectionAttemptIntervalMs = 5000;
+const int gMaxConsecutiveAttempts = 10;
+const char gServerName[] = "ubuntu-keyboard-info";
 }
 
 UbuntuKeyboardInfo::UbuntuKeyboardInfo(QObject *parent)
     : QObject(parent),
-    m_consecutiveAttempts(0),
-    m_lastWidth(0),
-    m_lastHeight(0)
+      m_consecutiveAttempts(0),
+      m_lastWidth(0),
+      m_lastHeight(0)
 {
     connect(&m_socket, &QLocalSocket::stateChanged, this, &UbuntuKeyboardInfo::onSocketStateChanged);
     connect(&m_socket, &QIODevice::readyRead,
@@ -149,5 +149,4 @@
     } else {
         m_socketFilePath = QDir("/tmp").filePath(gServerName);
     }
-
 }