Merge lp:~alan-griffiths/mir/draft-surfaces-report into lp:mir

[Alan Griffiths]

surface: draft debugging report to track surface management issues

surface: draft debugging report to track surface management issues

This checks an environment variable (MIR_DEBUG_SURFACES_REPORT) for the value "debug" and, if it finds that value dumps some debug information about surface management to stdout. It looks like:

DebugSurfacesReport::surface_created(0x7fece8000e58 ["eglappsurface"])
DebugSurfacesReport::create_surface_call(0x7fece8000e58 ["eglappsurface"]) - INFO surface count=1
DebugSurfacesReport::surface_created(0x7fecdc0011e8 ["eglappsurface"])
DebugSurfacesReport::create_surface_call(0x7fecdc0011e8 ["eglappsurface"]) - INFO surface count=2
DebugSurfacesReport::delete_surface_call(0x7fece8000e58 ["eglappsurface"]) - INFO surface count=2
DebugSurfacesReport::surface_deleted(0x7fece8000e58 ["eglappsurface"]) - INFO surface count=1
DebugSurfacesReport::delete_surface_call(0x7fecdc0011e8 ["eglappsurface"]) - INFO surface count=1
DebugSurfacesReport::surface_deleted(0x7fecdc0011e8 ["eglappsurface"]) - INFO surface count=0

This allows for checking that surfaces are not owned outside the surface stack. If that is happening then delete_surface_calls will not be followed by surface_deleted() calls and the surface count will not be decremented.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1128
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/174/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/2374
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/2259
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/171
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-saucy-amd64-ci/171/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/mir-team-mir-development-branch-ci/174/rebuild

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

OK for the purpose of debugging this particular problem, but also +1 for SurfacesReport gradually evolving to a configurable, publicly accessible report, like the other reports we have in the code.

I find SurfacesReport::create_surface_call() being called after SurfacesReport::surface_created() a bit unintuitive without knowledge of the internals (I would expect the opposite order). Perhaps we could find a more descriptive name for the *_call() methods, but not a blocker at this point.

review: Approve

[Daniel van Vugt (vanvugt)]

From what I can see, this does not affect our ability to move around and clean up the many /Surface/ classes post saucy. So that's good. But then, I still don't support the idea of reports being classes at all.

review: Abstain

[Robert Carr (robertcarr)]

Ok for now.

review: Approve

[Kevin DuBois (kdub)]

l135, why use getenv instead of using the options that we already have? i'd like to avoid too much split between env and command line variables.

review: Needs Fixing

=== modified file 'src/server/surfaces/CMakeLists.txt'
--- src/server/surfaces/CMakeLists.txt	2013-08-28 03:41:48 +0000
+++ src/server/surfaces/CMakeLists.txt	2013-10-15 14:45:43 +0000
@@ -6,4 +6,5 @@
   surface_data.cpp
   surface_stack.cpp
   surface_controller.cpp
+  surfaces_report.cpp
 )
 
=== modified file 'src/server/surfaces/surface.cpp'
--- src/server/surfaces/surface.cpp	2013-10-03 06:01:11 +0000
+++ src/server/surfaces/surface.cpp	2013-10-15 14:45:43 +0000
@@ -24,6 +24,8 @@
 #include "mir/input/input_channel.h"
 #include "mir/graphics/buffer.h"
 
+#include "surfaces_report.h"
+
 #include <boost/throw_exception.hpp>
 
 #include <stdexcept>
@@ -43,6 +45,7 @@
       server_input_channel(input_channel),
       surface_in_startup(true)
 {
+    the_surfaces_report()->surface_created(this);
 }
 
 void ms::Surface::force_requests_to_complete()
@@ -52,6 +55,7 @@
 
 ms::Surface::~Surface()
 {
+    the_surfaces_report()->surface_deleted(this);
 }
 
 std::shared_ptr<ms::BufferStream> ms::Surface::buffer_stream() const
 
=== modified file 'src/server/surfaces/surface_stack.cpp'
--- src/server/surfaces/surface_stack.cpp	2013-10-03 06:01:11 +0000
+++ src/server/surfaces/surface_stack.cpp	2013-10-15 14:45:43 +0000
@@ -29,6 +29,8 @@
 #include "mir/surfaces/input_registrar.h"
 #include "mir/input/input_channel_factory.h"
 
+#include "surfaces_report.h"
+
 #include <boost/throw_exception.hpp>
 
 #include <algorithm>
@@ -84,6 +86,7 @@
 
     input_registrar->input_channel_opened(surface->input_channel(), surface->input_surface(), params.input_mode);
 
+    the_surfaces_report()->create_surface_call(surface.get());
     emit_change_notification();
 
     return surface;
@@ -92,6 +95,8 @@
 void ms::SurfaceStack::destroy_surface(std::weak_ptr<ms::Surface> const& surface)
 {
     auto keep_alive = surface.lock();
+    the_surfaces_report()->delete_surface_call(keep_alive.get());
+
     bool found_surface = false;
     {
         std::lock_guard<std::recursive_mutex> lg(guard);
 
=== added file 'src/server/surfaces/surfaces_report.cpp'
--- src/server/surfaces/surfaces_report.cpp	1970-01-01 00:00:00 +0000
+++ src/server/surfaces/surfaces_report.cpp	2013-10-15 14:45:43 +0000
@@ -0,0 +1,154 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+#include "surfaces_report.h"
+
+#include "mir/surfaces/surface.h"
+
+#include <cstdlib>
+#include <cstring>
+
+#include <iostream>
+#include <map>
+#include <memory>
+#include <mutex>
+
+namespace ms = mir::surfaces;
+
+namespace
+{
+class NullSurfacesReport : public ms::SurfacesReport
+{
+public:
+    virtual void surface_created(ms::Surface* const /*surface*/) {}
+    virtual void create_surface_call(ms::Surface* const /*surface*/) {}
+
+    virtual void delete_surface_call(ms::Surface* const /*surface*/) {}
+    virtual void surface_deleted(ms::Surface* const /*surface*/) {}
+};
+
+class DebugSurfacesReport : public ms::SurfacesReport
+{
+public:
+    virtual void surface_created(ms::Surface* const /*surface*/);
+    virtual void create_surface_call(ms::Surface* const /*surface*/);
+
+    virtual void delete_surface_call(ms::Surface* const /*surface*/);
+    virtual void surface_deleted(ms::Surface* const /*surface*/);
+
+private:
+    std::mutex mutable mutex;
+
+    std::map<ms::Surface*, std::string> surfaces;
+};
+
+bool is_debug()
+{
+    auto env = std::getenv("MIR_DEBUG_SURFACES_REPORT");
+    return env && !std::strcmp(env, "debug");
+}
+}
+
+
+void DebugSurfacesReport::surface_created(ms::Surface* const surface)
+{
+    std::lock_guard<std::mutex> lock(mutex);
+    surfaces[surface] = surface->name();
+
+    std::cout << "DebugSurfacesReport::surface_created(" <<
+        surface << " [\"" << surface->name() << "\"])" << std::endl;
+}
+
+void DebugSurfacesReport::create_surface_call(ms::Surface* const surface)
+{
+    std::lock_guard<std::mutex> lock(mutex);
+
+    auto const i = surfaces.find(surface);
+
+    std::cout << "DebugSurfacesReport::create_surface_call(" <<
+        surface << " [\"" << surface->name() << "\"])";
+
+    if (i == surfaces.end())
+    {
+        std::cout << " - ERROR not reported to surface_created()";
+    }
+    else if (surface->name() != i->second)
+    {
+        std::cout << " - WARNING name changed from " << i->second;
+    }
+
+    std::cout << " - INFO surface count=" << surfaces.size() << std::endl;
+}
+
+void DebugSurfacesReport::delete_surface_call(ms::Surface* const surface)
+{
+    std::lock_guard<std::mutex> lock(mutex);
+
+    auto const i = surfaces.find(surface);
+
+    std::cout << "DebugSurfacesReport::delete_surface_call(" <<
+        surface << " [\"" << surface->name() << "\"])";
+
+    if (i == surfaces.end())
+    {
+        std::cout << " - ERROR not reported to surface_created()";
+    }
+    else if (surface->name() != i->second)
+    {
+        std::cout << " - WARNING name changed from " << i->second;
+    }
+
+    std::cout << " - INFO surface count=" << surfaces.size() << std::endl;
+}
+
+void DebugSurfacesReport::surface_deleted(ms::Surface* const surface)
+{
+    std::lock_guard<std::mutex> lock(mutex);
+
+    auto const i = surfaces.find(surface);
+
+    std::cout << "DebugSurfacesReport::surface_deleted(" <<
+        surface << " [\"" << surface->name() << "\"])";
+
+    if (i == surfaces.end())
+    {
+        std::cout << " - ERROR not reported to surface_created()";
+    }
+    else if (surface->name() != i->second)
+    {
+        std::cout << " - WARNING name changed from " << i->second;
+    }
+
+    if (i != surfaces.end())
+        surfaces.erase(i);
+
+    std::cout << " - INFO surface count=" << surfaces.size() << std::endl;
+}
+
+ms::SurfacesReport* ms::the_surfaces_report()
+{
+    static auto const result =
+        is_debug() ?
+            std::shared_ptr<ms::SurfacesReport>(std::make_shared<DebugSurfacesReport>()) :
+            std::shared_ptr<ms::SurfacesReport>(std::make_shared<NullSurfacesReport>());
+
+    return result.get();
+}
+
+
+
 
=== added file 'src/server/surfaces/surfaces_report.h'
--- src/server/surfaces/surfaces_report.h	1970-01-01 00:00:00 +0000
+++ src/server/surfaces/surfaces_report.h	2013-10-15 14:45:43 +0000
@@ -0,0 +1,51 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Alan Griffiths <[e-mail]>
+ */
+
+
+#ifndef MIR_SURFACES_SURFACES_REPORT_H_
+#define MIR_SURFACES_SURFACES_REPORT_H_
+
+namespace mir
+{
+namespace surfaces
+{
+class Surface;
+
+class SurfacesReport
+{
+public:
+    virtual void surface_created(Surface* const surface) = 0;
+    virtual void create_surface_call(Surface* const surface) = 0;
+
+    virtual void delete_surface_call(Surface* const surface) = 0;
+    virtual void surface_deleted(Surface* const surface) = 0;
+
+protected:
+    SurfacesReport() = default;
+    virtual ~SurfacesReport() = default;
+    SurfacesReport(SurfacesReport const&) = delete;
+    SurfacesReport& operator=(SurfacesReport const&) = delete;
+};
+
+// For the moment this is a debugging frig - it should probably grow into a real PfA report
+SurfacesReport* the_surfaces_report();
+}
+}
+
+
+#endif /* MIR_SURFACES_SURFACES_REPORT_H_ */