202474 - profile sharing - enable in build, use in prefs, use in embedding samples

Status: RESOLVED FIXED

(Core Graveyard :: Profile: BackEnd, defect)

Description

[Conrad Carlen (not reading bugmail)]

This is the initial integration of the IPC/Transaction Manager code for the
purpose of profile sharing.

Comment 1

[Conrad Carlen (not reading bugmail)]

Attachment: top level build patch

Pulls the ipc code, builds ipc/ipcd, and enables profile sharing by default. It
can be turned off with --disable-profilesharing for minimal footprint embedding
apps.

Comment 2

[Conrad Carlen (not reading bugmail)]

Attachment: prefs patch

allows prefs to use profile sharing

Comment 3

[Conrad Carlen (not reading bugmail)]

Attachment: mfcEmbed changes to make use of it

This also allows mfcEmbed to build with either the profile mgr, or just single
profile support, and allows setting of the profile directory names via
resources.

Comment 4

[Conrad Carlen (not reading bugmail)]

Attachment: patch to PPEmbed to make use of it

Not part of the build - my personal testbed ;-)

Comment 5

[Conrad Carlen (not reading bugmail)]

Attachment: the rest

profile directory service provider, etc. The profile locking code, which needs
to be used independently from the profile manager, was cut and pasted from
where it used to be.

Comment 6

[Conrad Carlen (not reading bugmail)]

Comment on attachment 120906 [details] [diff] [review]
prefs patch

Sorry, some tabs crept in here - will get rid of them.

Comment 7

[Conrad Carlen (not reading bugmail)]

Comment on attachment 120907 [details] [diff] [review]
mfcEmbed changes to make use of it

Not really sure who owns this now - Adam?

Comment 8

[Adam Lock]

Comment on attachment 120907 [details] [diff] [review]
mfcEmbed changes to make use of it

r=adamlock

Comment 9

[hacker formerly known as [e-mail]]

Comment on attachment 120904 [details] [diff] [review]
top level build patch

The configure option should set MOZ_PROFILESHARING appropriately if
--enable-profilesharing is set.  Fix that && r=cls

Comment 10

[John Gaunt (redfive)]

Comment on attachment 120906 [details] [diff] [review]
prefs patch

I have a couple of nits, but they are on another computer without access to the
net right now. I'll post them in the bug later today. Overall it looks great.
r=jgaunt

Comment 11

[Alec Flett]

Comment on attachment 120906 [details] [diff] [review]
prefs patch

I could really use WAY more comments here.. for instance:
+nsresult nsPrefService::ReadUserPrefsInternal(nsIFile *aFile, PRBool
aIsSharedFile)
+{
+  nsIFile *whichFile;
+
+#ifdef MOZ_PROFILESHARING  
+  if (aIsSharedFile) {
+    if (mCurrentSharedFile == aFile)
+      return NS_OK;

why?

+      NS_WARNING("not writing prefs because they haven't changed");

I wonder if we should just remove this (consider this "approval" for removing
it if you feel like it)

-    
+
+  nsIFile*							       
mCurrentSharedFile;

woah, that's some wacky indenting - check this file for tabs.. (later, after
looking at the patch: ouch, I see tabs all over - please remove all tabs from
this patch!!)

+class nsBufferInputStream
+{

woah, what is this? looks copy 'n pasted from somewhere? does this belong in
xpcom maybe? This looks like nsBinaryInputStream, really. This just seems like
a LOT of code to end up in the pref library... there must be a way to reuse
nsIBinaryInputStream - even if you end up wrapping a nsIStringStream with it.

contact me over AIM to discuss this more.

Comment 12

[Conrad Carlen (not reading bugmail)]

Nominating for 1.4b.

Comment 13

[John Gaunt (redfive)]

Comment on attachment 120906 [details] [diff] [review]
prefs patch

Here are my comments for my review. 

in Index: modules/libpref/src/nsPrefService.cpp

@@ -308,26 +345,26 @@
   nsresult rv;
   nsCOMPtr<nsIFile> aFile;

-  // Anything which calls NS_InitXPCOM will have this
-  rv = NS_GetSpecialDirectory(NS_APP_PREFS_50_FILE, getter_AddRefs(aFile));
-
-  if (!aFile) {
-    // We know we have XPCOM directory services, but we might not have a
provider which
-    // knows about NS_APP_PREFS_50_FILE. Put the file in
NS_XPCOM_CURRENT_PROCESS_DIR.
-    rv = NS_GetSpecialDirectory(NS_XPCOM_CURRENT_PROCESS_DIR,
getter_AddRefs(aFile));
-    if (NS_FAILED(rv)) return rv;
-    rv = aFile->AppendNative(NS_LITERAL_CSTRING("default_prefs.js"));
-    if (NS_FAILED(rv)) return rv;
-  } 
+#ifdef MOZ_PROFILESHARING
+  // First, read the shared file.
+  if (isSharingEnabled()) {
//XXXjg space between SHARED and NS, I don't see NS_SHARED defined anywhere
either
+    rv = NS_GetSpecialDirectory(NS_SHARED NS_APP_PREFS_50_FILE,
getter_AddRefs(aFile));
+    if (NS_SUCCEEDED(rv))
+      rv = ReadUserPrefsInternal(aFile, PR_TRUE);
+  }
+  // Continue on to read the nonshared file.
+#endif

-  rv = ReadUserPrefs(aFile);
-  if (NS_SUCCEEDED(rv)) {
+  rv = NS_GetSpecialDirectory(NS_APP_PREFS_50_FILE, getter_AddRefs(aFile));
+  if (NS_FAILED(rv))
+    return rv;    
+  rv = ReadUserPrefsInternal(aFile, PR_FALSE);
+  if (NS_SUCCEEDED(rv))
     return rv;
-  }

// XXXjg do we not need to save the shared file? Why save the pref file after
we JUST read it in? 
   // need to save the prefs now
-  SavePrefFile(aFile); 
-
+  SavePrefFile(aFile);
+  
   return rv;
 }

@@ -336,7 +373,15 @@

-----------------------------------------------------------------

in Index: modules/libpref/src/nsPrefService.cpp

@@ -434,6 +545,20 @@
   if (NS_SUCCEEDED(rv))
     gDirty = PR_FALSE;
   return rv;
+}
+
// XXXjg, erhn? you set the global everytime here? and then test it?
+static PRBool isSharingEnabled()
+{
+  static PRBool gSharingEnabled = PR_FALSE;
+  
+  // If FALSE, query again. It may not have been set yet.
+  if (!gSharingEnabled) {
+    nsCOMPtr<nsIProfileSharingSetup> sharingSetup =
+	 do_GetService("@mozilla.org/embedcomp/profile-sharing-setup;1");
+    if (sharingSetup)
+      sharingSetup->GetSharingEnabled(&gSharingEnabled);
+  }
+  return gSharingEnabled;
 }

 static nsresult openPrefFile(nsIFile* aFile, PRBool aIsErrorFatal,
Index: modules/libpref/src/nsPrefService.h

-------------------------------------------------------------------

in Index: modules/libpref/src/nsSharedPrefHandler.cpp

+	 
+nsresult nsSharedPrefHandler::OnSessionBegin()
+{
+  nsresult rv = EnsureTransactionService();
+  NS_ENSURE_SUCCESS(rv, rv);
+
// XXXjg make a note that it is synchronous
+  // Attach. When we receive the reply, we'll read the prefs.
+  rv = mTransService->Attach(kPrefsTSQueueName, this, PR_TRUE);
+  NS_ASSERTION(NS_SUCCEEDED(rv), "tmITransactionService::Attach() failed");
+  
+  return rv;
+}
+
+nsresult nsSharedPrefHandler::OnSessionEnd()

----------------------------------------------------------------------

in Index: modules/libpref/src/nsSharedPrefHandler.cpp

+NS_IMETHODIMP nsSharedPrefHandler::OnFlushReply(PRUint32 aQueueID, PRUint32
aStatus)
+{
+    LOG(("tmModuleTest: nsSharedPrefHandler::OnFlushReply [%d]\n", aStatus));
+    
// XXXjg maybe a comment about the TS holding the lock here too.
+    // Call the internal method to write immediately
+    mPrefService->SavePrefFileInternal(nsnull);
+    return NS_OK;
+}
+
+//****************************************************************************
*
+// Static functions
+//****************************************************************************
*

Comment 14

[Conrad Carlen (not reading bugmail)]

Attachment: snippet to show use of nsIBinaryOutputStream

I implemented a class which allows you to make an nsIOutputStream from a
nsACString& and changed the prefs code to use it. Before posting a new patch,
I'll post this snippet as an argument for using the specialized classes I made.


  Notice that to start writing, we have one 1 factory method and one trip
though the component mgr. Then, every WriteXXX() method goes through 2 layers
of virtual calls to get to the underlying string's Append() method.
nsBufferOutputStream is a simple stack-based class with no virtual methods at
all.

  nsBufferOutputStream is better WRT error handling. It keeps a an error code
which is never cleared for the lifetime of the object. You can do a bunch of
writes to it, and then at the end, do one GetError() check.

  Using nsIBinaryOutputStream, the transaction is dependent on how the stream
decides to store its data. For instance, when a string is written, is length
encoded before the string, or is the string null-terinated? Using my
specialized classes, an byte-by-byte layout of the transaction data can be
given and frozen without dependencies.

  Yeah, we want to keep code size down and not duplicate code that's already in
xpcom. But, there's a balance between performance and code size and I think the
additional code size of nsBufferInput/OutputStream is very small and outweighed
by its performance and utility here. What I would like to do is rename it to
tmTransactionReader and tmTransactionWriter, build this as a static lib in the
transactionmngr module and then code like prefs and cookies could link with the
lib. That would at least reduce code maintainence.

Alec, John, what'd you say?

Comment 15

[Darin Fisher]

i think i agree with conrad here.  just know he and i spoke about adding some
utility classes to the ipc code base that could be referenced from either client
code (prefs, cookies) or daemon code (think daemon module to support shared
cache).  so, we can't reuse any xpcom stuff.  we decided to create a small
library under ipcd/util/ that would provide these classes:

  ipcMessageReader
  ipcMessageWriter

this seems like a better home for nsBufferInputStream and nsBufferOutputStream,
respectively.  we can play around with making this library static or shared. 
for the moment static is probably fine since the footprint is small.

Comment 16

[Darin Fisher]

some of the ipc list primitives (in ipcd/shared/src) might want to live in this
util library (especially if we end up wanting to make use of them in some daemon
module).  i think the daemon module to support shared certs will greatly
appreciate ipcMessageReader & ipcMessageWriter.

Comment 17

[Alec Flett]

[re-posting after mid-air-colliding with darin - I am willing to defer to
darin's judgement because he's been way more involved in the whole profile
sharing thing, but I do want these points to be heard :) ]

The double-vtable issue is really a minor issue - sure right now we'd take the
hit with each Write() but do we even know if this is a performance hit? sounds
like premature optimization.. there are other ways to optimize this, such as
buffering inside of nsBinaryInputStream.

I've got to be honest - the code you've just written looks pretty simple to me -
sure there are some extra objects/buffers created on the heap and such, but
we're talking less than 10 objects, not thousands. How often is this called?

Maybe at some point we can revamp the xpcom string stream classes to make them
easier to use, such as allowing them to be stack based.

The code might end up looking a lot simipler:

nsStringOutputStream stream(str);
nsBinaryOutputStream outStream(stream);

(obviously this is something we can work on long after fixing this bug)

The thing is, any current consumer of nsIBinaryOutputStream takes the
double-vtable hit, (such as fastload, which takes the hit right now!) so in some
sense that is an excuse to add a buffer to nsBinaryOutputStream, not a reason
not to use it at all.

regarding the string termination bit - yes, it is up to the implementation
behind nsIBinaryOutputStream, but that's kind of the point, no? The stream
implementation input/output pair has to be smart enough not to lose data. If
we're concerned that we need to write strings with embedded nulls, then lets add
a WriteString(const nsAString& aString) to nsIBinaryOutputStream (I've actually
wanted that for a little while, if only to pass in a known string length to the
stream)

I agree about the tradeoff but I feel like we're doing performance optimization
in an area that we haven't proven is a performance hotspot, nor how important
performance even is, let alone whether one or two vtable calls even make a
measurable impact. 

Comment 18

[Conrad Carlen (not reading bugmail)]

Attachment: prefs patch v2

Adresses jgaunt's comments and cleans up the logic of  UseDefaultPrefFile() and
friends.

1. From the 1st patch, the method ReadUserPrefsInternal() is gone. Having that
method operate on both the shared file or non-shared didn't save much code and
made it confusing. There are now two separate routines ReadAndOwnUserPrefFile()
and ReadAndOwnSharedUserPrefFile(). They do a better job in tracking errors in
reading the files. That used to be done with a single global and, now that
there are possibly two files, there need to be two variables for this. The
logic and commenting in UseDefaultFile for when it saves out a new file (when
there is no file to read) is clearer now, I think.

2. The buffer stream classes are gone from here. I've moved them to
ipc/ipcd/util per discussion with darin. A patch for that is coming up.

3. Got rid of pesky tabs.

other comments from John:
> //XXXjg space between SHARED and NS, I don't see NS_SHARED defined anywhere
You don't need one - "foo" "bar" is the same to the compiler as "foobar".
NS_SHARED is defined in nsAppDirectoryServiceDefs.h which, sorry, is part of
another patch here.

> // XXXjg do we not need to save the shared file? Why save the pref file after

we JUST read it in?
That's cleaned up now and does save both files. It's explained more above.

> // XXXjg, erhn? you set the global everytime here? and then test it?
+static PRBool isSharingEnabled()
+{
+  static PRBool gSharingEnabled = PR_FALSE;

static vars are only initialized once by the runtime lib. Also, I don't
actually need the PR_FALSE because static data is always inited to zero by
default, but it makes it more readable.

Comment 19

[Conrad Carlen (not reading bugmail)]

Attachment: ipcMessageReader/Writer

Same as before, but renamed things, and removed xpcom dependencies
(NS_ERROR_FAILURE, nsnull, nsMemory). Pure nspr now with little change.

Comment 20

[John Gaunt (redfive)]

Comment on attachment 121472 [details] [diff] [review]
ipcMessageReader/Writer

check those tabs
r=jgaunt

Comment 21

[John Gaunt (redfive)]

Comment on attachment 121464 [details] [diff] [review]
prefs patch v2

r=jgaunt

looks great, can't wait for this to actually be in!!

Comment 22

[Darin Fisher]

Comment on attachment 121472 [details] [diff] [review]
ipcMessageReader/Writer

>Index: util/public/ipcMessageReader.h

>+#ifndef ipcMessageReader_h__
>+#define ipcMessageReader_h__
>+
>+#include "prtypes.h"
>+
>+//*****************************************************************************
>+// ipcMessageReader
>+//*****************************************************************************   
>+
>+class ipcMessageReader
>+{
>+public:
>+                  ipcMessageReader(const PRUint8* inBuffer, PRUint32 bufferSize) :
>+                    mBuf(inBuffer), mBufEnd(inBuffer + bufferSize),
>+                    mBufPtr(mBuf),
>+                    mError(PR_FALSE)
>+                  { }
>+                  
>+                  ~ipcMessageReader()
>+                  { }
>+
>+  PRBool          HasError()
>+                  { return mError; }
>+  
>+  PRUint8         GetInt8();
>+  PRUint16        GetInt16();
>+  PRUint32        GetInt32();
>+  PRInt32         GetBytes(void* destBuffer, PRInt32 n);
>+  
>+  const PRUint8*  GetPtr()  // our buffer never changes
>+                  { return mBufPtr; }
>+  PRBool          AdvancePtr(PRInt32 n);

how about some documentation for these methods?  should i 
always call HasError after calling GetIntX to see if the
GetIntX function succeeded?

what is does the return value of AdvancePtr indicate?

maybe you'd want to document the ownership model up top
above the constructor or something like that.

comment about byte-ordering?


>Index: util/public/ipcMessageWriter.h

>+class ipcMessageWriter

same documentation comments apply here.


>Index: util/src/ipcMessageReader.cpp

>+PRUint16 ipcMessageReader::GetInt16()
>+{
>+  if (mBufPtr + sizeof(PRUint16) <= mBufEnd) {
>+    PRUint16 tempInt = *(PRUint16 *)mBufPtr;

hmm... do you have to worry about unaligned memory access here?  what
if someone does GetInt8 followed by GetInt16?  this line will result
in an unaligned memory access runtime warning on some platforms (Tru64?).

how about this instead?

  PRUint8 temp[2] = { mBufPtr[0], mBufPtr[1] };
  mBufPtr += sizeof(PRUint16);
  return *(PRUint16*)temp;


>+PRUint32 ipcMessageReader::GetInt32()
>+{
>+  if (mBufPtr + sizeof(PRUint32) <= mBufEnd) {
>+    PRUint32 tempInt = *(PRUint32 *)mBufPtr;

and here too...

  PRUint8 temp[4] = { mBufPtr[0], mBufPtr[1], mBufPtr[2], mBufPtr[3] };
  mBufPtr += sizeof(PRUint32);
  return *(PRUint32*)temp;

we encountered this problem recently with fastload :-/


>Index: util/src/ipcMessageWriter.cpp

>+    PR_Free(mBuf);

maybe just use malloc & free directly since this ownership of
this memory is not going to be changing hands.	so, no reason
for extra level of indirection.


>+void ipcMessageWriter::PutInt16(PRUint16 val)
>+{
>+  if (EnsureCapacity(sizeof(PRUint16))) {
>+    *(PRUint16 *)mBufPtr = val;

again, looks like a potentially misaligned memory access.  how
about this:

  PRUint8* temp = (PRUint8*)&val;
  *mBufPtr++ = temp++;
  *mBufPtr++ = temp; 


>+void ipcMessageWriter::PutInt32(PRUint32 val)

same fun here i think..


>+  PRInt32 newCapacity = (mBufPtr - mBuf) + sizeNeeded;
>+  while (newCapacity > mCapacity)
>+    mCapacity <<= 1;

worry about roll over??  probably not a reality, but what would
happen if newCapacity went negative?

>+  mBuf = (PRUint8*)PR_Realloc(mBuf, mCapacity);

realloc ;-)

Comment 23

[Conrad Carlen (not reading bugmail)]

Attachment: ipcMessageReader/Writer v2

With darin's comments addressed.

Comment 24

[Darin Fisher]

Comment on attachment 121504 [details] [diff] [review]
ipcMessageReader/Writer v2

add comment stating that AdvancePtr beyond end of buffer puts object in error
state??

add comments in the cpp files about the alignment issue you are combating.

otherwise, this looks great to me.  sr=darin

Comment 25

[Darin Fisher]

Comment on attachment 120907 [details] [diff] [review]
mfcEmbed changes to make use of it

sr=darin

Comment 26

[Conrad Carlen (not reading bugmail)]

Attachment 121504 [details] [diff] is checked into mozilla/ipc on IPC_BRANCH_20030304.

Comment 27

[Alec Flett]

Comment on attachment 121464 [details] [diff] [review]
prefs patch v2

+  PRPackedBool 	   mErrorOpeningUserPrefs;
+  
+  nsIFile*		   mCurrentSharedFile;
+  PRPackedBool 	   mErrorOpeningSharedUserPrefs;
 };

the compiler will word-align word-size members, so make sure that
PRPackedBool's are all next to each other - the above will take up 12 bytes,
but if you rearrange them they will only take up 8.

regarding ReadLine(), I guess I'm disappointed that we don't have something
that will already handle that. I would suggest nsILineInputStream but I think
it does some goofy conversion/copying. 

other than the packing though, this looks fine. sr=alecf

Comment 28

[[e-mail]]

nsILineInputStream does currently do some goofy stuff. see bug 197166 for making
it not suck. (maybe you want to add a dependency so we can come back and fix
this, if it's worth the time)

Comment 29

[Darin Fisher]

Attachment: review comments for "the rest" -- nothing bug nits

Comment 30

[Asa Dotzler [:asa]]

If we're going to get this for the 1.4 branch then it needs to make beta.
Setting blocking1.4b+ flag.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Irix, Linux static build, and OS/2 are red after this checkin (see ports
tinderbox)....

Comment 32

[Conrad Carlen (not reading bugmail)]

See bug 204177 for ports bustage.

Comment 33

[Scott MacGregor]

Just wondering...but it looks like the change to embedding\config\basebrowser-win

adds a duplicate listing for:
intl.xpt
(http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=&subdir=mozilla/embedding/config&command=DIFF_FRAMESET&root=&file=basebrowser-win&rev1=1.80&rev2=1.81)

Is there a reason why it needs to be listed twice? =)

Comment 34

[Conrad Carlen (not reading bugmail)]

This landed. As far as verification, I will make some documentation on how it
can be tested using mfcEmbed.