185706 - Create a transaction manager module for the IPC Daemon to allow sharing of profile data

Status: RESOLVED FIXED

(Core Graveyard :: Profile: BackEnd, defect)

Description

[John Gaunt (redfive)]

bug 178806 discusses an IPC daemon for communication between two or more running
mozilla processes. A module needs to be written for the IPC daemon to convey
changes in profile information. 

The transaction manager needs a client side piece that can be talked to through
XPCOM by services that represent profile data ( like the pref service or cookie
service ) as well as a server side module for the IPC daemon that receives the
transactions and sends the data to any processes interested in the changes.

Documentation on the profile sharing problem and this solution can be found at:
http://www.mozilla.org/projects/embedding/shared_profiles.html
,however the tech spec documents are slightly out of date already. (I'll be
updating them soon - this week)

Comment 1

[John Gaunt (redfive)]

Attachment: working transaction manager moduel for ipc daemon

This tarball contains a transactionmngr directory. It needs the code from
darin's ipc daemon to be built (see bug ) in modules/ipc. This code expects to
be placed in modules alongside the ipc directory (modules/transactionmngr). The
placement is just temporary until we find a final resting place for ipc and
it's modules.

This contains a test app: tmModuleTest, that spins up the ipc daemon and a
client that sends messages through the tmTransactionService (client side)
across the ipc layer and to the tmTransactionModule (in the ipcDaemon). There
are some different modes you can run it in: debug, listener, broadcaster ...
that are documented in the file.

Comment 2

[John Gaunt (redfive)]

whoops forget to add the bug number in the last comment, it is of coures bug
178806 the dependency for this bug.

Comment 3

[John Gaunt (redfive)]

Attachment: ready for review

Cleaned up patch, ready for review. 

untars to ./transactionmngr/

like the previous, goes in modules like the ipc code of darins (see dependency)


oh, haven't tested this on linux or mac, but will be checking that out this
afternoon, tomorrow AM at the latest.

Comment 4

[timeless]

Comment on attachment 112827 [details]
ready for review

don't stick things in modules.
don't include Makefile.
don't include *.dll *.exe *.exp *.ilk *.obj *.lib *.pdb *.res _xpidlgen
do include a .cvsignore which would prevent those files from appearing
don't license files NPL
don't copyright files 1998
don't use PR_ASSERT (use NS_ASSERTION &| NS_ENSURE_...)
tmTransaction::SetRawMessage(const PRUint8 *aRawMessage, PRUint32 aLength) {
  mData = new tmMsg();
  mData->action = *index;
don't crash when new fails!
NS_INIT_ISUPPORTS(); is obsolete, don't use it.

javadoc style is:
/**
 * like this
 */
not:
/**
  * like this
  */

I don't think tmUtils.h should include prlog.h/stdio.h, but that's a personal
opinion.


tmIPCModule::Init() {
  //printf("tmIPCModule::Init()\n");
  if (tm == nsnull)
    tm = new tmTransactionManager();
}
what happens if new fails here?
ah, here we go:
void
tmIPCModule::HandleMsg(ipcClientHandle client, const nsID &target, const void
*data, PRUint32 dataLen) {
  tmTransaction *trans = new tmTransaction(IPC_GetClientID(client),
					   (PRUint8 *)data,
					   dataLen);
!! you don't check to see if new fails
  if (tm == nsnull)
    Init();
!! you crash if init fails
  tm->HandleTransaction(trans);

tmQueue::PostTransaction(tmTransaction *aTrans) {
  mTM->SendTransaction(ownerID, new tmTransaction(ownerID,
						  TM_POST_REPLY,
						  mID,
						  status));

I hope sendtransaction is null safe for its second param.
of course it isn't:
void tmIPCModule::SendMsg(PRUint32 aDestClientIPCID, tmTransaction
*aTransaction) {
  IPC_SendMsg(aDestClientIPCID,
	      kTransModuleID,
	      (void *)aTransaction->GetRawMessage(),
!!crash here
	      aTransaction->GetRawMessageLength());
}

tmVector::tmVector() : mIterator(0), mIndex(0), mNext(0) , mSize(0),
mCapacity(75),
		       mGrowthIncrement(3) {
  mElements = new void*[mCapacity];
}
you shouldn't allocate memory in constructors, it may fail.
PRUint32 tmVector::Add(void *aElement){
  mElements[mNext] = aElement;
!! crash here.

Methods in idl should be interCaps not InitialCaps:

interface tmITransactionService : nsISupports
{
void Init(in ACString aProfileName);

  qm->queueName = PL_strdup(PromiseFlatCString(aQueueName).get());
if you were willing to trade PL_strfree for nsMemory::Free you could use
ToNewCString(aQueueName) and save an alloc.

tmModule.h claims to be part of IPC:
 * The Original Code is Mozilla IPC.
i suppose it is, but is this really what it should say?

why are you including #if 0 code in tmModule.cpp?


template <class Type>
inline
mPtr<Type>& mPtr<Type>::operator=(const mPtr<Type>& mp) {
  if (this != &mp) {
    delete t;
    t = new Type(*(mp.t));  // assumes the assignment operator is good for the
Type
  }
  return *this;
}

This code is fragile.  Most people assume operator= will succeed. but if new
Type(..) fails, then the assignment will fail.

RETURN_IF_FAILED is reinventing NS_ENSURE_SUCCESS


  // helps with shutdown on some cases
  PR_Sleep(PR_SecondsToInterval(4));
this is broken. but i suppose i can fix it later.

Comment 5

[John Gaunt (redfive)]

Attachment: gzip of revised code

addressed timeless's comments, conrad's point about the component thing,
plugged some leaks, cleaned a lot of crap up and doc'd almost everything. have
specific replies in a file I'm going to attach next

Comment 6

[John Gaunt (redfive)]

Attachment: reply to comment 4 and description of patch above

Comment 7

[John Gaunt (redfive)]

Attachment: patch to make TM build

This is a patch to Makefile.in and allmakefiles.sh that adds the neccessary foo
to make the transaction manager code build. You must define MOZ_TM=1 in your
environment and have unpacked the tarball so that it looks like:
mozilla/modules/transactionmngr

Comment 8

[Conrad Carlen (not reading bugmail)]

Oops - I should have mentioned that I had done something like this. It goes a
little further though: It adds a autoconf switch which makes a #define, and
turns on MOZ_IPC as well.

Comment 9

[John Gaunt (redfive)]

Comment on attachment 115160 [details] [diff] [review]
patch to make TM build

This patch isn't quite right and doesn't apply because I had to edit it.

Instead, you can pull the PROFILE_SHARING_1_BRANCH and build there! :-)

Comment 10

[John Gaunt (redfive)]

Attachment: patch of transmngr ipc module

This is a patch (really) containing the revisions of Darin's review of the
transaction manager IPC module. The code is also in it's new home, the
ipc/ipcd/extensions/ directory.

Comment 11

[Conrad Carlen (not reading bugmail)]

Comment on attachment 120409 [details] [diff] [review]
patch of transmngr ipc module

Looks good. The doc in the header files is really nice. Just a few nits...

>? Makefile
>? build/Makefile
>? common/Makefile
>? module/Makefile
>? public/Makefile
>? src/Makefile
>? test/Makefile

Don't forget to check in a .cvsignore file for each directory containing a
Makfile.in. That will prevent the '?' output. Of course, if using Windows,
you're gonna be buried by the other '?' output.


>Index: build/tmModule.cpp
>===================================================================
>+
>+#if 0
>+NS_METHOD
>+tmTransactionServiceRegisterProc(nsIComponentManager *aCompMgr,
>+                       nsIFile *aPath,
>+                       const char *registryLocation,
>+                       const char *componentType,
>+                       const nsModuleComponentInfo *info)

Might as well get rid of this #if 0 code. I doubt we will ever need to load the
component in this way. Even if we did, I think the preferred way is to use the
stuff in embedding/components/appstartup.


>Index: common/tmVector.cpp
>===================================================================
>Index: common/tmVector.h
>===================================================================

A nice, compact implementation, but why couldn't nsVoidArray be used?

>Index: public/tmITransactionService.idl
>===================================================================
>+  void attach(in ACString aDomainName, 
>+              in tmITransactionObserver aObserver, 
>+              in PRBool aLockingCall);

Something should be said about what sort of reference the transaction service
keeps to the observer. It's just a plain weak reference, which people might not
expect.

>Index: src/tmTransactionService.cpp
>===================================================================
>+tm_waiting_msg::~tm_waiting_msg() {
>+  printf("destroying a tm_waiting_msg struct\n");

Put that in #if DEBUG. There are a few other printfs in this file and we
wouldn't want to drag in StdCLib just for that.

>+tmTransactionService::~tmTransactionService() {
>+
>+  // just destroy this, it contains 2 pointers it doesn't own.
>+  if (mObservers)
>+    PL_HashTableDestroy(mObservers);
>+
>+  PRUint32 index = 0;
>+  PRUint32 size = mWaitingMessages.Size();
>+  tm_waiting_msg *msg = nsnull;
>+  for ( ; index < size; index ++) {
>+    msg = (tm_waiting_msg*) mWaitingMessages[index];
>+    if (msg)

No need for the "if" with delete.

>+      delete msg;
>+  }
>+

Comment 12

[John Gaunt (redfive)]

Comment on attachment 120409 [details] [diff] [review]
patch of transmngr ipc module

super review from darin?

Comment 13

[Darin Fisher]

so i just have one big nit about the transaction service.  specifically, since
it has been moved into the IPC module, i think the IDL prefix should be switched
from "tm" to "ipc".  not necessary to change it throughout, but i think renaming
the interfaces would be a good thing. likewise, the contract id should be 

  "@mozilla.org/ipc/transaction-service;1"

or something like that.  also, i think you want to do away with building
transaction service as its own XPCOM DLL.  it should just be statically built
into the ipcdc XPCOM DLL (just like what is done with the lock service extension).

Comment 14

[Darin Fisher]

Comment on attachment 120409 [details] [diff] [review]
patch of transmngr ipc module

sr=darin fwiw... all this code is on the IPC branch anyways.  i just need to
flip it over to the trunk at some point.  but, this bug is essentially fixed i
think.

Comment 15

[Darin Fisher]

This has been on the trunk for a while now... marking FIXED.