[Gnash-commit] /srv/bzr/gnash/trunk r12067: Stop relying on signals to kill gnash when launched from the plugin.

[Bastiaan Jacques]

------------------------------------------------------------
revno: 12067
committer: Bastiaan Jacques <address@hidden>
branch nick: trunk
timestamp: Tue 2010-03-16 19:04:59 +0100
message:
  Stop relying on signals to kill gnash when launched from the plugin. 
  Instead, add another file descriptor (pipe) argument to gnash, which the 
  (plugin-compatible) GUI monitors for SIGHUP. Additionally:
  
  plugin/plugin.cpp: Don't leak the GIOChannel.
  gui/gtk.cpp: Remove LIRC callback that does nothing.
modified:
  gui/Player.cpp
  gui/Player.h
  gui/gnash.cpp
  gui/gtk.cpp
  gui/gtksup.h
  gui/gui.cpp
  gui/gui.h
  plugin/plugin.cpp
  plugin/plugin.h

=== modified file 'gui/Player.cpp'
--- a/gui/Player.cpp    2010-03-14 02:26:46 +0000
+++ b/gui/Player.cpp    2010-03-16 18:04:59 +0000
@@ -104,6 +104,7 @@
     _fpsDebugTime(0.0),
 #endif
     _hostfd(-1),
+    _controlfd(-1),
     _startFullscreen(false),
     _hideMenu(false)
 {
@@ -418,6 +419,10 @@
     // Set host requests fd (if any)
     if ( _hostfd != -1 ) root.setHostFD(_hostfd);
 
+    if (_controlfd != -1) {
+        _gui->setFDCallback(_controlfd, boost::bind(&Gui::quit, 
boost::ref(_gui)));
+    }
+
     _gui->setStage(&root);
     
     // When startStopped is true, stop here after the stage has been 

=== modified file 'gui/Player.h'
--- a/gui/Player.h      2010-03-14 02:26:46 +0000
+++ b/gui/Player.h      2010-03-16 18:04:59 +0000
@@ -148,6 +148,10 @@
     int getHostFD() const {
         return _hostfd;
     }
+
+    void setControlFD(int fd) {
+        _controlfd = fd;
+    }
     
     void setStartFullscreen(bool x) {
         _startFullscreen = x;
@@ -354,6 +358,8 @@
     // Filedescriptor to use for host application requests, -1 if none
     int _hostfd;
     
+    int _controlfd;
+
     // Whether to start Gnash in fullscreen mode.
     // (Or what did you think it meant?)
     bool _startFullscreen;

=== modified file 'gui/gnash.cpp'
--- a/gui/gnash.cpp     2010-03-14 02:26:46 +0000
+++ b/gui/gnash.cpp     2010-03-16 18:04:59 +0000
@@ -141,6 +141,8 @@
             "\"FlashVars=A=1&b=2\")\n") 
     << _("  -F,  --fd <fd>           Filedescriptor to use for external "
             "communications\n") 
+    << _("  -G,  --controlfd <fd>    File descriptor for external application"
+            " control\n")
 #ifdef GNASH_FPS_DEBUG
     << _("  -f,  --debug-fps num     Print FPS every num seconds (float)\n") 
 #endif // def GNASH_FPS_DEBUG
@@ -236,6 +238,7 @@
         { 'V', "version",           Arg_parser::no  },        
         { 'f', "debug-fps",         Arg_parser::yes },        
         { 'F', "fd",                Arg_parser::yes },
+        { 'G', "controlfd",         Arg_parser::yes },
         { 'A', "dump",              Arg_parser::yes },
         { 259, "screenshot",        Arg_parser::yes },
         { 260, "screenshot-file",   Arg_parser::yes },
@@ -329,6 +332,17 @@
                     player.setHostFD ( fd );
                     break;
                 }
+                case 'G':
+                {
+                    const int fd = parser.argument<long>(i);
+                    if (fd < 1) {
+                        cerr << boost::format(_("Invalid host communication "
+                                    "filedescriptor %d\n")) % fd << endl;
+                        exit(EXIT_FAILURE);
+                    }
+                    player.setControlFD ( fd );
+                    break;
+                }
                 case 'j':
                     widthGiven = true;
                     player.setWidth(parser.argument<long>(i));

=== modified file 'gui/gtk.cpp'
--- a/gui/gtk.cpp       2010-03-14 02:26:46 +0000
+++ b/gui/gtk.cpp       2010-03-16 18:04:59 +0000
@@ -109,7 +109,10 @@
     void menuQualityLow(GtkMenuItem *menuitem, gpointer instance); 
     void menuQualityMedium(GtkMenuItem *menuitem, gpointer instance); 
     void menuQualityHigh(GtkMenuItem *menuitem, gpointer instance); 
-    void menuQualityBest(GtkMenuItem *menuitem, gpointer instance); 
+    void menuQualityBest(GtkMenuItem *menuitem, gpointer instance);
+
+    gboolean fd_callback_handler(GIOChannel *source, GIOCondition condition,
+                                 gpointer data);
 
     void timeoutQuit(gpointer data);
 
@@ -151,13 +154,6 @@
 
 }
 
-#ifdef USE_LIRC
-// This is global so it can be accessed by the event handler, which
-// isn't part of this class. 
-Lirc *lirc;
-bool lirc_handler(void*, int, void* data);
-#endif
-
 GtkGui::~GtkGui()
 {
 }
@@ -326,10 +322,7 @@
     
 #ifdef USE_LIRC
     lirc = new Lirc();
-    if (lirc->init("/dev/lircd")) {
-        int fd = lirc->getFileFd();
-        addFDListener(fd, lirc_handler, &fd);
-    } else {
+    if (!lirc->init("/dev/lircd")) {
         log_debug(_("LIRC daemon not running"));
     }
 #endif
@@ -367,8 +360,9 @@
     g_timeout_add(timeout, (GSourceFunc)timeoutQuit, this);
 }
 
+
 bool
-GtkGui::addFDListener(int fd, callback_t callback, void* data)
+GtkGui::watchFD(int fd)
 {
     // NOTE: "The default encoding for GIOChannel is UTF-8. If your application
     // is reading output from a command using via pipe, you may need to set the
@@ -381,13 +375,13 @@
         return false;
     }
     
-    if (!g_io_add_watch (gio_read, GIOCondition(G_IO_IN | G_IO_HUP),
-                         GIOFunc (callback), data)) {
+    if (!g_io_add_watch (gio_read, GIOCondition(G_IO_HUP),
+                         GIOFunc (fd_callback_handler), this)) {
         g_io_channel_unref(gio_read);
-        return false;    
+        return false;
     }
     
-    return true;    
+    return true;
 }
 
 void
@@ -2244,19 +2238,6 @@
 
 }
 
-#ifdef USE_LIRC
-bool
-lirc_handler(void*, int, void* /*data*/)
-{ 
-    
-    // want to remove this handler. You may want to close fd.
-    log_debug("%s\n", lirc->getButton());
-  
-    // Want to keep this handler
-    return true;
-}
-#endif
-
 // This assumes that the parent of _drawingArea is _window, which
 // isn't the case in the plugin fullscreen (it's _overlay). Currently
 // we return from fullscreen when Gui::stop() is called, which
@@ -2843,6 +2824,18 @@
     gui->setQuality(QUALITY_BEST);
 }
 
+gboolean
+fd_callback_handler(GIOChannel *source, GIOCondition condition,
+                    gpointer data)
+{
+    Gui* gui = static_cast<Gui*>(data);
+
+    gui->callCallback(g_io_channel_unix_get_fd (source));
+
+    return true;
+}
+
+
 } // anonymous namespace
 
 } // end of namespace gnash

=== modified file 'gui/gtksup.h'
--- a/gui/gtksup.h      2010-03-14 02:26:46 +0000
+++ b/gui/gtksup.h      2010-03-16 18:04:59 +0000
@@ -52,9 +52,6 @@
 {
 public:
 
-    /// For the Gtk GUI FD listener, whatever that might be for.
-    typedef bool (*callback_t)(void*, int, void *data);
-
     GtkGui(unsigned long xid, float scale, bool loop, RunResources& r);
     
     virtual ~GtkGui();
@@ -109,7 +106,7 @@
     ///        callback should return false if the listener is to be removed.
     /// @param data A pointer to a user-defined data structure.
     /// @return true on success, false on failure.
-    bool addFDListener(int fd, callback_t callback, void* data);
+    bool watchFD(int fd);
 
     /// Grab focus so to receive all key events
     //

=== modified file 'gui/gui.cpp'
--- a/gui/gui.cpp       2010-03-14 02:26:46 +0000
+++ b/gui/gui.cpp       2010-03-16 18:04:59 +0000
@@ -1324,6 +1324,30 @@
 }
 
 void
+Gui::setFDCallback(int fd, boost::function<void ()> callback)
+{
+    _fd_callbacks[fd] = callback;
+
+    watchFD(fd);
+}
+
+
+void
+Gui::callCallback(int fd)
+{
+    std::map<int, boost::function<void ()> >::iterator it = 
_fd_callbacks.find(fd);
+
+    if (it == _fd_callbacks.end()) {
+        log_error("Attempted to call a callback for an unregistered fd.");
+        return;
+    }
+
+    boost::function<void()>& f = it->second;
+
+    f();
+}
+
+void
 ScreenShotter::saveImage(const std::string& id) const
 {
     // Replace all "%f" in the filename with the frameAdvance.

=== modified file 'gui/gui.h'
--- a/gui/gui.h 2010-03-14 06:02:38 +0000
+++ b/gui/gui.h 2010-03-16 18:04:59 +0000
@@ -37,6 +37,7 @@
 
 #include <boost/intrusive_ptr.hpp>
 #include <boost/scoped_ptr.hpp>
+#include <boost/function.hpp>
 #include <vector>
 #include <cstdlib> 
 #include <string>
@@ -172,6 +173,10 @@
       _interval = interval;
     }
 
+    void setFDCallback(int fd, boost::function<void ()> callback);
+
+    void callCallback(int fd);
+
     /// Return the clock provided by this Gui.
     //
     /// The Gui clock will be paused when the gui is put
@@ -505,6 +510,11 @@
         std::exit(EXIT_SUCCESS);
     }
 
+    virtual bool watchFD(int /* fd */)
+    {
+        log_unimpl("This GUI does not implement FD watching.");
+    }
+
 
     /// Determines if playback should restart after the movie ends.
     bool            _loop;
@@ -557,6 +567,8 @@
     virtual void playHook() {}
 
 private:
+    std::map<int /* fd */, boost::function<void ()> > _fd_callbacks;
+
     /// Width of a window pixel, in stage pseudopixel units.
     float _xscale;
 

=== modified file 'plugin/plugin.cpp'
--- a/plugin/plugin.cpp 2010-01-11 06:41:38 +0000
+++ b/plugin/plugin.cpp 2010-03-16 18:04:59 +0000
@@ -411,6 +411,7 @@
     _width(0),
     _height(0),
     _streamfd(-1),
+    _controlfd(-1),
     _ichan(0),
     _ichanWatchId(0),
     _childpid(0),
@@ -467,6 +468,17 @@
             << " should be unlinked!" << std::endl;
 #endif
     }
+
+    if (_childpid > 0) {
+        // When the child has terminated (signaled by _controlfd), it remains
+        // as a defunct process and we remove it from the kernel table now.
+        int status;
+        waitpid(_childpid, &status, 0);
+#if GNASH_PLUGIN_DEBUG > 1
+        std::cout << "Child process exited with status " << status << 
std::endl;    
+#endif
+    }
+    _childpid = 0;
 }
 
 /// \brief Initialize an instance of the plugin object
@@ -509,21 +521,14 @@
     std::cout << "Gnash plugin shutting down" << std::endl;
 #endif
 
-    if (_childpid > 0) {
-        // it seems that waiting after a SIGINT hangs firefox
-        // IFF not run from the console (see bug#17082).
-        // SIGTERM instead solves this problem
-        kill(_childpid, SIGTERM);
-        int status;
-        waitpid(_childpid, &status, 0);
+    int ret = close(_controlfd);
 #if GNASH_PLUGIN_DEBUG > 1
-        std::cout << "Child process exited with status " << status << 
std::endl;    
-#endif
+    if (ret != 0) {
+        std::cout << "Gnash plugin failed to close the control socket!"
+                  << std::endl;
     }
-
-    _childpid = 0;
+#endif
 }
-
 /// \brief Set the window to be used to render in
 ///
 /// This sets up the window the plugin is supposed to render
@@ -1165,6 +1170,7 @@
     // 0 For reading, 1 for writing.
     int p2c_pipe[2];
     int c2p_pipe[2];
+    int p2c_controlpipe[2];
     
     int ret = pipe(p2c_pipe);
     if (ret == -1) {
@@ -1183,17 +1189,29 @@
 #endif
     }
 
+    ret = pipe(p2c_controlpipe);
+    if (ret == -1) {
+#ifdef GNASH_PLUGIN_DEBUG
+        std::cout << "ERROR: child to parent pipe() failed: " << 
+            std::strerror(errno) << std::endl;
+#endif
+    }
+
+    _controlfd = p2c_controlpipe[1];
+
     /*
     Setup the command line for starting Gnash
     */
 
     // Prepare width, height and window ID variables
     const size_t buf_size = 30;
-    char xid[buf_size], width[buf_size], height[buf_size], hostfd[buf_size];
+    char xid[buf_size], width[buf_size], height[buf_size], hostfd[buf_size],
+         controlfd[buf_size];
     snprintf(xid, buf_size, "%ld", win);
     snprintf(width, buf_size, "%d", _width);
     snprintf(height, buf_size, "%d", _height);
     snprintf(hostfd, buf_size, "%d", c2p_pipe[1]);
+    snprintf(controlfd, buf_size, "%d", p2c_controlpipe[0]);
 
     // Prepare Actionscript variables (e.g. Flashvars).
     std::vector<std::string> paramvalues;
@@ -1219,7 +1237,7 @@
     ADD NEW ARGUMENTS
     */ 
 
-    const size_t maxargc = 18 + paramvalues.size() * 2;
+    const size_t maxargc = 20 + paramvalues.size() * 2;
     const char **argv = new const char *[maxargc];
 
 #ifdef CREATE_STANDALONE_GNASH_LAUNCHER
@@ -1269,6 +1287,10 @@
     argv[argc++] = "-F";
     argv[argc++] = hostfd;
 
+    // Control FD
+    argv[argc++] = "-G";
+    argv[argc++] = controlfd; 
+
     // Base URL is the page that the SWF is embedded in. It is 
     // by Gnash for resolving relative URLs in the movie. If the
     // embed tag "base" is specified, its value overrides the -U
@@ -1347,6 +1369,8 @@
                                 << std::endl;
 #endif
         }
+
+        ret = close (p2c_controlpipe[0]); // close read descriptor
     
 
 #if GNASH_PLUGIN_DEBUG > 1
@@ -1360,7 +1384,7 @@
         _ichanWatchId = g_io_add_watch(_ichan, 
                 (GIOCondition)(G_IO_IN|G_IO_HUP), 
                 (GIOFunc)handlePlayerRequestsWrapper, this);
-
+        g_io_channel_unref(_ichan);
         return;
     }
 
@@ -1378,6 +1402,8 @@
 #endif
     }
 
+    ret = close(p2c_controlpipe[1]);
+
     // close standard input and direct read-fd1 to standard input
     ret = dup2 (p2c_pipe[0], fileno(stdin));
     
@@ -1399,6 +1425,8 @@
     for ( ; numfailed < 10; anfd++) {
         if ( anfd == c2p_pipe[1] ) continue; // don't close this
         if ( anfd == c2p_pipe[0] ) continue; // don't close this either 
(correct?)
+        if ( anfd == p2c_controlpipe[0] ) continue; // don't close this either 
(correct?)
+        if ( anfd == p2c_controlpipe[1] ) continue; // don't close this either 
(correct?)
         ret = close (anfd);
         if (ret < 0) {
            numfailed++;

=== modified file 'plugin/plugin.h'
--- a/plugin/plugin.h   2010-01-11 06:41:38 +0000
+++ b/plugin/plugin.h   2010-03-16 18:04:59 +0000
@@ -123,6 +123,7 @@
     int                                _streamfd;
     GIOChannel*                        _ichan;
     int                                _ichanWatchId;
+    int                                _controlfd;
     pid_t                              _childpid;
     int                                _filefd;