Subject: ipc review notes so far
From: Brendan Eich
Date: Fri, 21 Mar 2003 15:15:01 -0800
To: Darin Fisher

I'm not sure when I'll finish -- maybe you should just land this and I'll do it after?  None of these are showstoppers to landing, needless to say.

Is conrad landing soon?

/be



client/src/ipcService.h - template usage ok? typedef ipcList<class ipcClientQuery> ipcClientQueryQ; - //PRBool InWaitMessage() { return mInWaitMessage; } vs. #if 0 for members - how about a major doc-comment talking about ipcService class? - can you use an nsCOMPtr<ipcTransport> for mTransport? client/src/ipcSocketProviderUnix.h - XP_UNIX downcast in ~ipcTransport could use NS_STATIC_CAST? avoid .get()? Why does only Unix need the ClearTransport call? - why a monitor instead of just a lock, or a lock and a condvar? - if (!mMonitor), nsAutoMonitor will assert but not crash, but you'll race among multiple threads; can't fail the ctor, check in Init method? client/src/ipcModuleFactory.cpp - Mixing #if 0 with commenting out to hide ipcService{R,Unr}egisterProc client/src/ipcService.cpp - Use PR_STATIC_CALLBACK, not static PRBool PR_CALLBACK, for max portability oh wait, this matters only on WIN16 -- still, worth using that macro? - ipcClientQuery::IsCanceled() use nsnull, not NULL, and do you need to use .get() to compare to 0? i thought not any longer (old gcc bug, we're past it?) - ipcClientQuery::OnQueryComplete() - check for malloc failure - move const char *lastName and *lastTarget inside the loop bodies, no need for useless inits and outer-scope decls - Use nsnull not NULL? (throughout, last time i'll mention it -- ah, there's an nsnull in the ~ipcService assertion) - ipcService::Init() just return mTransport->Init(this); (no need for rv dance, unless you have to normalize NS_COMFALSE or another success code into NS_OK) - ipcService::OnIPCMClientID() unchecked new in mTransport->SendMsg call - ipcService::OnIPCMClientInfo() and ipcService::OnIPCMError() seem awfully similar, but they're short; just wondering if they could share code (not source code, compiled code) - hey, new failure checks and NS_ERROR_OUT_OF_MEMORY! i thought i was out to lunch wanting those earlier (Photon's philosophy is very big-virtual-memory system: who cares? you'll thrash to death first; put up a dialog and close other apps; whatever -- but i digress) - whoops, ipcService::QueryClientByName() does not null test query = new ... - ditto for ipcService::QueryClientByID() - more unchecked new calls in SetMessageObserver -- check throughout - does passing null to SendMsg make it fail immediately with NS_ERROR_OOM? it fails, but not with OOM -- shouldn't it? or else callers here should not pass in new ... (potentially pass in null) - why does the ipcService observe "profile-change-net-..."? what's the -net- mean in those strings? comments would be helpful here - why the PRUint8 vs. char types in const pointer params that seem to require casts without adding much value? - does SendMsg always consume its msg param? it depends on whether SendMsg_Internal does; i see a case in the Unix version that returns early due to evil NS_ENSURE_TRUE's hidden return, w/o freeing msg.... client/src/ipcSocketProviderUnix.cpp - ipcIOLayerConnect - do the stat after fstat OSX 10.1.5 nonsense only for that platform - ipcSocketProviderUnix::NewSocket - don't you need to null *securityInfo before returning NS_OK? or is that not an out param? client/src/ipcTransport.cpp - general comment: are you using a monitor just to get a lock and a condvar, or do you need reentrancy? it seems better to me to use a separate lock and condvar if you don't need a monitor, but if that just bloats the code, then never mind