fixes some XP_WIN review comments from dougt. fixed some startup/shutdown

races, etc.

modules/ipc/common/ipcLog.cpp

@@ -39,29 +39,50 @@
 
 #ifdef IPC_LOGGING
 
-#ifdef XP_UNIX
-#include <sys/types.h>
-#include <unistd.h>
-#define GETPID() ((PRUint32) getpid())
-#endif
-
-#ifdef XP_WIN
-#include <windows.h>
-#define GETPID() ((PRUint32) GetCurrentProcessId())
-#endif
-
-#ifndef GETPID
-#define GETPID 0
-#endif
-
 #include <string.h>
 
 #include "prenv.h"
 #include "prprf.h"
 #include "plstr.h"
 
-PRBool ipcLogEnabled;
+PRBool ipcLogEnabled = PR_FALSE;
-char ipcLogPrefix[10];
+char ipcLogPrefix[10] = {0};
+
+//-----------------------------------------------------------------------------
+// UNIX
+//-----------------------------------------------------------------------------
+#ifdef XP_UNIX
+#include <sys/types.h>
+#include <unistd.h>
+
+static inline PRUint32
+WritePrefix(char *buf, PRUint32 bufLen)
+{
+    return PR_snprintf(buf, bufLen, "[%u] %s ",
+                       (unsigned) getpid(),
+                       ipcLogPrefix);
+}
+#endif
+
+//-----------------------------------------------------------------------------
+// WIN32
+//-----------------------------------------------------------------------------
+#ifdef XP_WIN
+#include <windows.h>
+
+static inline PRUint32
+WritePrefix(char *buf, PRUint32 bufLen)
+{
+    return PR_snprintf(buf, bufLen, "[%u:%u] %s ",
+                       GetCurrentProcessId(),
+                       GetCurrentThreadId(),
+                       ipcLogPrefix);
+}
+#endif
+
+//-----------------------------------------------------------------------------
+// logging API impl
+//-----------------------------------------------------------------------------
 
 void
 IPC_InitLog(const char *prefix)
@@ -81,7 +102,7 @@ IPC_Log(const char *fmt, ... )
     char buf[512];
 
     if (ipcLogPrefix[0])
-        nb = PR_snprintf(buf, sizeof(buf), "[%u] %s ", GETPID(), ipcLogPrefix);
+        nb = WritePrefix(buf, sizeof(buf));
 
     PR_vsnprintf(buf + nb, sizeof(buf) - nb, fmt, ap);
     buf[sizeof(buf) - 1] = '\0';

modules/ipc/daemon/ipcdUnix.cpp

@@ -182,8 +182,8 @@ static void ShutdownDaemonDir()
 //
 // declared in ipcdPrivate.h
 //
-ipcClient *ipcClients;
+ipcClient *ipcClients = NULL;
-int        ipcClientCount;
+int        ipcClientCount = 0;
 
 //
 // the first element of this array is always zero; this is done so that the
@@ -252,6 +252,7 @@ static int RemoveClient(int clientIndex)
 static void PollLoop(PRFileDesc *listenFD)
 {
     // the first element of ipcClientArray is unused.
+    memset(ipcClientArray, 0, sizeof(ipcClientArray));
     ipcClients = ipcClientArray + 1;
     ipcClientCount = 0;
 

modules/ipc/daemon/ipcdWin.cpp

@@ -37,6 +37,8 @@
 
 #include <windows.h>
 
+#include "prthread.h"
+
 #include "ipcConfig.h"
 #include "ipcLog.h"
 #include "ipcMessage.h"
@@ -49,12 +51,13 @@
 //
 // declared in ipcdPrivate.h
 //
-ipcClient *ipcClients;
+ipcClient *ipcClients = NULL;
-int        ipcClientCount;
+int        ipcClientCount = 0;
 
 static ipcClient ipcClientArray[IPC_MAX_CLIENTS];
 
-static HWND ipcHwnd;
+static HWND   ipcHwnd = NULL;
+static PRBool ipcShutdown = PR_FALSE;
 
 #define IPC_PURGE_TIMER_ID 1
 #define IPC_WM_SENDMSG  (WM_USER + 1)
@@ -71,7 +74,7 @@ RemoveClient(ipcClient *client)
 
     int clientIndex = client - ipcClientArray;
 
-    client->Finalize(); // XXX pick another name.. 
+    client->Finalize();
 
     //
     // move last ipcClient object down into the spot occupied by this client.
@@ -89,7 +92,8 @@ RemoveClient(ipcClient *client)
     if (ipcClientCount == 0) {
         LOG(("  shutting down...\n"));
         KillTimer(ipcHwnd, IPC_PURGE_TIMER_ID);
-        PostMessage(ipcHwnd, IPC_WM_SHUTDOWN, 0, 0);
+        PostQuitMessage(0);
+        ipcShutdown = PR_TRUE;
     }
 }
 
@@ -142,7 +146,7 @@ AddClient(HWND hwnd, PRUint32 pid)
     ipcClient *client = &ipcClientArray[ipcClientCount];
     client->Init();
     client->SetHwnd(hwnd);
-    client->SetPID(pid);    // XXX one funhction instead of 3
+    client->SetPID(pid);    // XXX one function instead of 3
 
     ++ipcClientCount;
     LOG(("  num clients = %u\n", ipcClientCount));
@@ -167,12 +171,13 @@ GetClientByPID(PRUint32 pid)
 // message processing
 //-----------------------------------------------------------------------------
 
-void
+static void
 ProcessMsg(HWND hwnd, PRUint32 pid, const ipcMessage *msg)
 {
     LOG(("ProcessMsg [pid=%u len=%u]\n", pid, msg->MsgLen()));
 
     ipcClient *client = GetClientByPID(pid);
+
     if (client) {
         //
         // if this is an IPCM "client hello" message, then reset the client
@@ -181,15 +186,15 @@ ProcessMsg(HWND hwnd, PRUint32 pid, const ipcMessage *msg)
         if (msg->Target().Equals(IPCM_TARGET) &&
             IPCM_GetMsgType(msg) == IPCM_MSG_TYPE_CLIENT_HELLO) {
             RemoveClient(client);
-            client = AddClient(hwnd, pid);
+            client = NULL;
         }
     }
-    else
-        client = AddClient(hwnd, pid);
 
-    // XXX add logging
+    if (client == NULL) {
-    if (client == NULL)
+        client = AddClient(hwnd, pid);
-        return;
+        if (!client)
+            return;
+    }
 
     IPC_DispatchMsg(client, msg);
 }
@@ -225,6 +230,10 @@ WindowProc(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
     LOG(("got message [msg=%x wparam=%x lparam=%x]\n", uMsg, wParam, lParam));
 
     if (uMsg == WM_COPYDATA) {
+        if (ipcShutdown) {
+            LOG(("ignoring message b/c daemon is shutting down\n"));
+            return TRUE;
+        }
         COPYDATASTRUCT *cd = (COPYDATASTRUCT *) lParam;
         if (cd && cd->lpData) {
             ipcMessage msg;
@@ -237,7 +246,7 @@ WindowProc(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
                 //
                 // grab client PID and hwnd.
                 //
-                ProcessMsg((HWND) wParam, cd->dwData, &msg);
+                ProcessMsg((HWND) wParam, (PRUint32) cd->dwData, &msg);
             }
             else
                 LOG(("ignoring malformed message\n"));
@@ -245,12 +254,6 @@ WindowProc(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
         return TRUE;
     }
 
-    // XXX don't check wParam
-    if (uMsg == WM_TIMER && wParam == IPC_PURGE_TIMER_ID) {
-        PurgeStaleClients();
-        return 0;
-    }
-
     if (uMsg == IPC_WM_SENDMSG) {
         HWND hWndDest = (HWND) wParam;
         ipcMessage *msg = (ipcMessage *) lParam;
@@ -265,15 +268,29 @@ WindowProc(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
         LOG(("  done.\n"));
 
         delete msg;
-    }
-
-    if (uMsg == IPC_WM_SHUTDOWN) {
-        DestroyWindow(hWnd);  // XXX possibly move out... think about shutdown sync
-        ipcHwnd = NULL;
-        PostQuitMessage(0);
         return 0;
     }
 
+    if (uMsg == WM_TIMER) {
+        PurgeStaleClients();
+        return 0;
+    }
+
+#if 0
+    if (uMsg == IPC_WM_SHUTDOWN) {
+        //
+        // since this message is handled asynchronously, it is possible
+        // that other clients may have come online since this was issued.
+        // in which case, we need to ignore this message.
+        //
+        if (ipcClientCount == 0) {
+            ipcShutdown = PR_TRUE;
+            PostQuitMessage(0);
+        }
+        return 0;
+    }
+#endif
+
     return DefWindowProc(hWnd, uMsg, wParam, lParam);
 }
 
@@ -307,6 +324,7 @@ static void
 ReleaseLock()
 {
     if (ipcSyncEvent) {
+        LOG(("releasing lock...\n"));
         CloseHandle(ipcSyncEvent);
         ipcSyncEvent = NULL;
     }
@@ -326,13 +344,12 @@ main(int argc, char **argv)
     if (!AcquireLock())
         return 0;
 
-    // XXX add comments
+    // initialize global data
+    memset(ipcClientArray, 0, sizeof(ipcClientArray));
     ipcClients = ipcClientArray;
     ipcClientCount = 0;
 
-    //
     // create message window up front...
-    //
     WNDCLASS wc;
     memset(&wc, 0, sizeof(wc));
     wc.lpfnWndProc = WindowProc;
@@ -345,22 +362,31 @@ main(int argc, char **argv)
     if (!ipcHwnd)
         return -1;
 
-    //
     // load modules
-    //
     IPC_InitModuleReg(argv[0]);
 
-    //
-    // process messages
-    //
     LOG(("entering message loop...\n"));
-
     MSG msg;
     while (GetMessage(&msg, ipcHwnd, 0, 0))
         DispatchMessage(&msg);
 
+    // unload modules
+    IPC_ShutdownModuleReg();
+
+    //
+    // we release the daemon lock before destroying the window because the
+    // absence of our window is what will cause clients to try to spawn the
+    // daemon.
+    //
     ReleaseLock();
 
+    //LOG(("sleeping 5 seconds...\n"));
+    //PR_Sleep(PR_SecondsToInterval(5));
+
+    LOG(("destroying message window...\n"));
+    DestroyWindow(ipcHwnd);
+    ipcHwnd = NULL;
+
     LOG(("exiting\n"));
     return 0;
 }

modules/ipc/src/ipcTransport.cpp

@@ -210,13 +210,14 @@ ipcTransport::Observe(nsISupports *subject, const char *topic, const PRUnichar *
     LOG(("ipcTransport::Observe [topic=%s]\n", topic));
 
     if (strcmp(topic, "timer-callback") == 0) {
-        // 
-        // try reconnecting to the daemon
-        //
-        Shutdown();
-        Connect();
-
         mTimer = nsnull;
+        if (!mHaveConnection) {
+            // 
+            // try reconnecting to the daemon
+            //
+            Shutdown();
+            Connect();
+        }
     }
     else if (strcmp(topic, "xpcom-shutdown") == 0 ||
              strcmp(topic, "profile-change-net-teardown") == 0)

modules/ipc/src/ipcTransportWin.cpp

@@ -60,12 +60,12 @@
 #define IPC_WM_SHUTDOWN   (WM_USER + 0x2)
 
 static nsresult       ipcThreadStatus = NS_OK;
-static PRThread      *ipcThread;
+static PRThread      *ipcThread = NULL;
-static PRMonitor     *ipcMonitor;
+static PRMonitor     *ipcMonitor = NULL;
-static nsIEventQueue *ipcEventQ;
+static nsIEventQueue *ipcEventQ = NULL;
-static HWND           ipcDaemonHwnd;
+static HWND           ipcDaemonHwnd = NULL;
-static HWND           ipcHwnd;       // not accessed on message thread!!
+static HWND           ipcLocalHwnd = NULL;  // not accessed on message thread!!
-static ipcTransport  *ipcTrans;      // not accessed on message thread!!
+static ipcTransport  *ipcTrans = NULL;      // not accessed on message thread!!
 
 //-----------------------------------------------------------------------------
 // event proxy to main thread
@@ -164,8 +164,6 @@ ipcThreadWindowProc(HWND hWnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
     }
 
     if (uMsg == IPC_WM_SHUTDOWN) {
-        DestroyWindow(hWnd);
-        ipcHwnd = NULL;
         PostQuitMessage(0);
         return 0;
     }
@@ -193,22 +191,25 @@ ipcThreadFunc(void *arg)
     char wName[sizeof(IPC_CLIENT_WINDOW_NAME_PREFIX) + 20];
     PR_snprintf(wName, sizeof(wName), "%s%u", IPC_CLIENT_WINDOW_NAME_PREFIX, pid);
 
-    ipcHwnd = CreateWindow(IPC_CLIENT_WINDOW_CLASS, wName,
+    ipcLocalHwnd = CreateWindow(IPC_CLIENT_WINDOW_CLASS, wName,
-                           0, 0, 0, 10, 10, NULL, NULL, NULL, NULL);
+                                0, 0, 0, 10, 10, NULL, NULL, NULL, NULL);
 
     {
         nsAutoMonitor mon(ipcMonitor);
-        if (!ipcHwnd)
+        if (!ipcLocalHwnd)
             ipcThreadStatus = NS_ERROR_FAILURE;
         mon.Notify();
     }
 
-    if (ipcHwnd) {
+    if (ipcLocalHwnd) {
         MSG msg;
-        while (GetMessage(&msg, ipcHwnd, 0, 0))
+        while (GetMessage(&msg, ipcLocalHwnd, 0, 0))
             DispatchMessage(&msg);
     }
 
+    DestroyWindow(ipcLocalHwnd);
+    ipcLocalHwnd = NULL;
+
     LOG(("exiting message thread\n"));
     return;
 }
@@ -240,7 +241,7 @@ ipcThreadInit(ipcTransport *transport)
     // wait for hidden window to be created
     {
         nsAutoMonitor mon(ipcMonitor);
-        while (!ipcHwnd && NS_SUCCEEDED(ipcThreadStatus))
+        while (!ipcLocalHwnd && NS_SUCCEEDED(ipcThreadStatus))
             mon.Wait();
     }
 
@@ -255,8 +256,7 @@ ipcThreadInit(ipcTransport *transport)
 static PRStatus
 ipcThreadShutdown()
 {
-    PostMessage(ipcHwnd, IPC_WM_SHUTDOWN, 0, 0);
+    PostMessage(ipcLocalHwnd, IPC_WM_SHUTDOWN, 0, 0);
-    ipcHwnd = NULL; // don't access this anymore
 
     PR_JoinThread(ipcThread);
     ipcThread = NULL;
@@ -287,6 +287,8 @@ ipcTransport::Shutdown()
     if (ipcThread)
         ipcThreadShutdown();
 
+    // clear our reference to the daemon's HWND.
+    ipcDaemonHwnd = NULL;
     return NS_OK;
 }
 
@@ -323,7 +325,16 @@ ipcTransport::Connect()
     SendMsg_Internal(new ipcmMessageClientHello());
     mSentHello = PR_TRUE;
 
-    return NS_OK;
+    //
+    // begin a timer.  if the timer fires before we get a CLIENT_ID, then
+    // assume the connection attempt failed.
+    //
+    nsresult rv;
+    mTimer = do_CreateInstance(NS_TIMER_CONTRACTID, &rv);
+    if (NS_SUCCEEDED(rv))
+        rv = mTimer->Init(this, 1000, nsITimer::TYPE_ONE_SHOT);
+
+    return rv;
 }
 
 nsresult
@@ -331,10 +342,9 @@ ipcTransport::SendMsg_Internal(ipcMessage *msg)
 {
     LOG(("ipcTransport::SendMsg_Internal\n"));
 
-    NS_ENSURE_TRUE(ipcDaemonHwnd, NS_ERROR_NOT_INITIALIZED);
+    NS_ENSURE_TRUE(ipcLocalHwnd, NS_ERROR_NOT_INITIALIZED);
-    NS_ENSURE_TRUE(ipcHwnd, NS_ERROR_NOT_INITIALIZED);
 
-    if (!PostMessage(ipcHwnd, IPC_WM_SENDMSG, 0, (LPARAM) msg)) {
+    if (!PostMessage(ipcLocalHwnd, IPC_WM_SENDMSG, 0, (LPARAM) msg)) {
         LOG(("  PostMessage failed w/ error = %u\n", GetLastError()));
         delete msg;
         return NS_ERROR_FAILURE;