201611 - [cookies] implement profile sharing

Status: RESOLVED WONTFIX

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

Ok, so I finally had a chance to give this a go. i've whipped up a patch for
cookies to give us profile sharing. it's based on the pref sharing stuff
ccarlen's written on the branch - thanks for the help, darin and ccarlen!

it hasn't yet been compiled/tested, but i thought i'd post it just to let you
guys look and see what you think. (if anyone wants to play with it, this patch
is _not_ diffed against trunk, it's diffed against a cookie rewrite that's about
to land. specifically, you'll need to apply a patch in bug 200632,
http://bugzilla.mozilla.org/attachment.cgi?id=120069&action=view, before
applying this one. also note that said patch is against trunk, not against the
profile sharing branch, which i heard may be out of date, so you may have to
update extensions/cookie first ;).

this transmgr stuff is neat, this was pretty easy to do!

Comment 1

[[e-mail]]

Attachment: v1 patch

Comment 2

[Conrad Carlen (not reading bugmail)]

Nice. A few things:

1. You probably don't need this for cookies:

+nsCookieService::IsSharingEnabled()
+{
+  // If FALSE, query again. It may not have been set yet.

Prefs needed that because the prefs service can be inited so early in startup -
before NS_InitEmbedding() returns and the app can turn on sharing. Unless the
cookie service gets created that early (through nsIAppStartupNotifier), you can
just get the sharing setup iface and check it once.

2. Just a matter of style but, for me, these macros

CHECK_PROFILE_SHARING(OnSessionBegin(), Read())

don't help the readability much. I have to do a double-take to see what's going
on there.

3. This "Ensure" code

+nsresult
+nsCookieService::EnsurePendingFlush()
+{
+  if (mPendingFlushReply)
+    return NS_OK;

doesn't need to be here (or in prefs anymore) It was originally there for when
flushing could happen asynchronously. Now that it's always sync, this isn't
really needed anymore.

4. I'll have to make the stream classes I made in prefs available in a lib
somewhere. They make stuff like this

+  const PRBool isSession = *NS_STATIC_CAST(const PRBool*, aData);
+  aData += sizeof(PRBool);

easier and cleaner.

Comment 3

[benc]

can you provide more expected behavior?

Comment 4

[Darin Fisher]

benc: that's a question for profile sharing in general.  not best answered here,
but i'll give it a go anyways.  presuming you have two profile-sharing-enabled
mozilla browsers, launch each using the same profile.  now browse to some site
that sets a cookie, and using the second browser open the cookie manager and
confirm that indeed the cookie exists in the second browser as well.  same goes
for removing cookies.

Comment 5

[Darin Fisher]

dwitte: thanks for the patch btw! ;-)

the deserialization code has alignment problems.  if one of the char arrays
happens to be odd in length, you'll get very poor performance (and runtime
warnings on some platforms) when you dereference segments of the input buffer as
a DWORD (PRBool, nsCookieStatus, etc.).  you can assume the input buffer is
properly aligned, so i would put all the DWORD fields at the top of the buffer
and move the variable length strings to the end.

might also be a good idea to assert that the input buffer has 4 byte alignment.
 something like this works:

  NS_ASSERTION((PRUptrdiff(aData) & 0x3) == 0, "unexpected data alignment");

then you can safely proceed to dereference an array of PRUint32 followed by
arrays of char.

Comment 6

[[e-mail]]

thanks for the comments guys!

ok so i finally managed to pull the PROFILE_SHARING_1_BRANCH (took forever, grr)
and build the sucker. compile errors on a clean checkout. two broken makefiles,
well done ;)

i take it this branch hasn't been used/compiled in a while...?

now as ccarlen pointed out, the cookie module in the branch is yonks old so i
had to update it to current trunk + phase 3 (patch in bug 200632). this was fun
too, because phase 3 depends on an xpcom change that was landed in jan, so i had
to rebuild xpcom too. i can provide a complete diff on request if you guys wanna
test profile sharing w/ cookies.

question: how do i test it, exactly? i've got a build with my patch, but i can't
figure out how to enable profile sharing. i can do it by calling EnableSharing()
from somewhere, but afaics this doesn't get around the profile lock. so it's not
possible to start 2 instances on the same profile.

so, am i missing something, or is there a cunning way to make it all work?

Comment 7

[[e-mail]]

Attachment: bustage fixes for branch

this fixes the two makefiles busted on the branch. i think you'll need these to
get it compiling... ;)

Comment 8

[[e-mail]]

Comment on attachment 120351 [details] [diff] [review]
bustage fixes for branch

marking r=ccarlen? just as a reminder to check it in sometime ;)

Comment 9

[Conrad Carlen (not reading bugmail)]

Comment on attachment 120351 [details] [diff] [review]
bustage fixes for branch

r=ccarlen. That's right, "transmngr" is the new module name. I still had the
old stuff in my tree.

Comment 10

[Conrad Carlen (not reading bugmail)]

> but afaics this doesn't get around the profile lock. so it's not
possible to start 2 instances on the same profile.

Each instance has to pass a different name to enableSharing(). Otherwise,
they'll both be attempting to use the same nonshared data and the locking will
thwart that. I've been using PPEmbed to test where that name comes from its
properties file. Later today, I'll add something to MfcEmbed to make testing
this easy. 

Comment 11

[Michael Buckland]

Sharing cookies does not appear that interesting to current embedding customer.
 Discussed in edt.  Minusing.

Comment 12

[[e-mail]]

Conrad: aha, I see. that makes more sense. did you manage to check that in? I'm
still not sure on the exact steps to enable & test, so if you have some
instructions somewhere that'd be neat, or i'll catch you on #mozilla or
something. thx!

so, re comment 11, does this mean a) this won't be landing for 1.4, or b) if
it's finished it'll go in?

Comment 13

[Conrad Carlen (not reading bugmail)]

dwitte: I checked in the Makefile changes, stuff I was working on with prefs,
and the testbed code for MfcEmbed. To test with MfcEmbed:

(1) Build it.
(2) Make a copy of MfcEmbed in dist/bin. Say it's called ZZEmbed.
(2) Open up ZZEmbed.exe as Resources with Visual Studio.
(3) Change the string (value == 181) from "Browser" to something else.
(4) Save it.

Now, you can open MfcEmbed and ZZEmbed at the same time, each will use its own
nonshared data, and you should be able to share stuff between then. For fun with
prefs, open about:config, and change font.default from serif to sans-serif.

Comment 14

[[e-mail]]

okay - in that case, it'll be a little difficult for me to test. i discovered
this after darin informed me that mfcembed is only built on windows (i build on
linux). ;)

so, would you be happy to test it out when i post some patches here? there are
three basic functions to test:

1) Adding cookies (eg browsing sites).
2) Removing cookies (via cookie manager UI).
3) Removing all cookies (again via cookiemgr "remove all" UI).

and then lots of permutations of course, for the usual
profile-do-change/shutdown-cleanse kinda stuff.

by the way, for shutdown-cleanse: we currently don't get a lock before removing
the cookie file, and there's no transaction sent indicating the removal - is
this okay? i.e. do we assume that all clients must be in kiosk mode _and_ shut
down before the profile gets blown away? seems ok to me but i just wanted to
check with you.

Comment 15

[[e-mail]]

Attachment: v2 patch

updated per darin and ccarlen's comments, and thought about things a bit more.

most importantly: now includes a notification for RemoveAll(), which is exposed
through the cookiemanager UI. this adds a bit of complexity because now we have
more than one transaction type, so we need to add that field to the serialized
data.

Comment 16

[[e-mail]]

Comment on attachment 120668 [details] [diff] [review]
v2 patch

oh, one note: you don't want to apply or test this patch, it won't apply to the
branch in its current state. for review purposes only.

it will apply cleanly when the branch merges with trunk, and phase 3 of the
cookie rewrite lands on the trunk.

Comment 17

[[e-mail]]

Attachment: v2 complete patch (diffed against branch)

this is the patch to use for testing.

it's the same as the v2 patch, but is diffed against the branch, so it includes
all the dependencies the previous v2 patch requires (it's enormous...).
basically a complete replacement of extensions/cookie, and a small change to
xpcom/base (nsAutoPtr was updated & exported in january, and recent cookie
landings depend on it).

because of the xpcom change, i think you'll have to clobber your build for
things to work. a toplevel make dep won't cut it, it gave me all kinds of
trouble with the cookieservice not starting properly at runtime. sorry!

so, if you get around to testing it out, let me know how things go ;)

Comment 18

[Conrad Carlen (not reading bugmail)]

Comment on attachment 120668 [details] [diff] [review]
v2 patch

>@@ -312,28 +343,51 @@ nsresult nsCookieService::Init()
>   nsresult rv;
> 
>   // cache and init the preferences observer
>   mPrefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>   if (NS_SUCCEEDED(rv)) {
>     InitPrefObservers();
>   } else {
>     InitDefaultPrefs();
>   }
> 
>+  // check whether profile sharing is enabled - this must be done before we
>+  // init mCookieFile, since we need to know whether sharing is enabled or not
>+  // to figure out the cookie file path. note that we only need to check this
>+  // once (rather than polling) since the cookieservice is inited fairly late
>+  // in the startup process - if this ever changes, we may need to poll.
>+#ifdef MOZ_PROFILESHARING
>+  nsCOMPtr<nsIProfileSharingSetup> sharingSetup =
>+    do_GetService("@mozilla.org/embedcomp/profile-sharing-setup;1", &rv);
>+  if (NS_SUCCEEDED(rv)) {
>+    PRBool sharingEnabled; // mSharingEnabled is a PRPackedBool
>+    sharingSetup->GetSharingEnabled(&sharingEnabled);
>+    mSharingEnabled = sharingEnabled;
>+  }
>+  mTransService = do_GetService("@mozilla.org/transaction/service;1");
>+
>+  // read cookies from file
>+  if (mSharingEnabled) {
>+    OnSessionBegin();
>+  } else {
>+    Read();
>+  }
>+#else
>+  Read();
>+#endif
>+

Why is the file not being gotten until after Read()?

>   // cache mCookieFile
>-  rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(mCookieFile));
>+  rv = NS_GetSpecialDirectory(GET_PROFILE_DIR, getter_AddRefs(mCookieFile));
>   if (NS_SUCCEEDED(rv)) {
>     rv = mCookieFile->AppendNative(NS_LITERAL_CSTRING(kCookieFileName));
>   }
> 
>-  Read();
>-

>+nsCookieService::OnCookieChanged(const nsCookie *aCookie,
>+                                 PRBool         aForceExpiry)
>+{
>+  if (mProcessingTransaction)
>+    return NS_OK;
>+
>+  if (!mTransService)
>+    return NS_ERROR_FAILURE;
>+
>+  PRUint8 *data;
>+  PRUint32 dataLen;
>+  SerializeCookie(aCookie, aForceExpiry, data, dataLen);
>+  if (!data)
>+    return NS_ERROR_FAILURE;
>+
>+  // PostTransaction will only fire OnTransactionAvailable for attached
>+  // processes *other* than this one, so the caller can continue after this
>+  // method returns.
>+  nsresult rv;
>+  rv = mTransService->PostTransaction(NS_LITERAL_CSTRING(kCookiesTSQueueName), data, dataLen);
>+  NS_ASSERTION(NS_SUCCEEDED(rv), "tmITransactionService::PostTransaction() failed");
>+

You need to free the data passed to PostTransaction or it'll leak. The
transaction mgr makes a copy.

>+  return rv;
>+}
>+
>+nsresult
>+nsCookieService::OnRemoveAll()
>+{
>+  if (!mTransService)
>+    return NS_ERROR_FAILURE;
>+
>+  // alloc space for the transaction data; return if failed
>+  const size_t dataSize = 2*sizeof(PRUint32);
>+  PRUint8 *data = NS_STATIC_CAST(PRUint8*, nsMemory::Alloc(dataSize));
>+  if (!data)
>+    return NS_ERROR_FAILURE;
>+
>+  // write the transaction data version, and the 'remove all' token
>+  PRUint8 *dest = data;
>+  *NS_REINTERPRET_CAST(PRUint32*, dest) = kCurrentCookiesTransactionDataVersion;
>+  dest += sizeof(PRUint32);
>+  *NS_REINTERPRET_CAST(PRUint32*, dest) = kCookiesRemoveAllTransaction;
>+
>+  // PostTransaction will only fire OnTransactionAvailable for attached
>+  // processes *other* than this one, so the caller can continue after this
>+  // method returns.
>+  nsresult rv;
>+  rv = mTransService->PostTransaction(NS_LITERAL_CSTRING(kCookiesTSQueueName), data, PRUint32(dataSize));

Same here.

>+  NS_ASSERTION(NS_SUCCEEDED(rv), "tmITransactionService::PostTransaction() failed");
>+
>+  return rv;
>+}
>+
>+/******************************************************************************
>+ * nsCookieService impl:
>+ * tmITransactionObserver
>+ ******************************************************************************/
>+
>+NS_IMETHODIMP
>+nsCookieService::OnTransactionAvailable(PRUint32      aQueueID,
>+                                        const PRUint8 *aData,
>+                                        PRUint32      aDataLen)
>+{
>+  // first, check the input buffer is 4-byte aligned, for perf considerations
>+  NS_ASSERTION((PRUptrdiff(aData) & 0x3) == 0, "unexpected data alignment!");
>+

Is this worth asserting here? It seems to depend only on transaction mgr
design.

Comment 19

[[e-mail]]

> Why is the file not being gotten until after Read()?

meh. that was dumb. thanks for catching :)

> You need to free the data passed to PostTransaction or it'll leak. The
> transaction mgr makes a copy.

that was dumb too.

> Is this worth asserting here? It seems to depend only on transaction mgr
> design.

darin asked for this one, so I'll leave the decision up to him... ;)

Comment 20

[[e-mail]]

Attachment: v3 patch

new patch. addresses comments and makes one extra change:

in OnRemoveAll(), we're calling up Alloc() for two measly PRUint32's. so i just
stack-alloced them, by making a struct RemoveAllStruct. i'm not sure if this is
really necessary (can I just declare |PRUint32 foo; PRUint32 bar;| and be
certain that they're contiguous in memory? i figure the struct is a safe way to
do it, but maybe it's not necessary).

Comment 21

[[e-mail]]

Attachment: v3 complete patch

ok, this one should actually be testable now... ;)

Comment 22

[Conrad Carlen (not reading bugmail)]

Comment on attachment 120952 [details] [diff] [review]
v3 patch

r=ccarlen

Comment 23

[[e-mail]]

Comment on attachment 120952 [details] [diff] [review]
v3 patch

thx ccarlen, requesting sr=darin, feel free to ignore this since you know
there's no rush :)

in particular, comments on the |RemoveAllStruct| thingy i mentioned in a
previous comment, would be appreciated.

Comment 24

[[e-mail]]

Attachment: v3.1 patch

hmm, what was I thinking? using a struct is just wrong. int[] is so much
better. ;)

same comments as v3 apply, only change is killing RemoveAllStruct.

Comment 25

[[e-mail]]

Attachment: v3.1 complete patch

again, the complete version, for testing purposes.

Comment 26

[[e-mail]]

Comment on attachment 121153 [details] [diff] [review]
v3.1 patch

carrying over r=ccarlen for the simple change.

Comment 27

[Alec Flett]

Comment on attachment 121153 [details] [diff] [review]
v3.1 patch

so I hadn't looked at this patch but then I was directed to look at
SerializeCookie - this is kind of nasty and conrad is trying to solve this same
problem for cookies.
I've directed him at nsIBinaryInputStream - he's going to be writing a class
that allows you to wrap an nsIOutputStream around an existing buffer, which
would enable you to wrap the nsIOutputStream with nsIBinaryOutputStream, which
would allow you to serialize arbitrary data in a standard way.

you can then use nsIBinaryInputStream to deserialize it. 

Something to consider... would be a lot less work on your side and would
probably make things like Location Independence/etc much easier

Comment 28

[Darin Fisher]

Comment on attachment 121153 [details] [diff] [review]
v3.1 patch

i'm afraid this patch has bit-rotted some.  sorry for not reviewing this
earlier!!!

>+REQUIRES	+= transmngr \

  transmngr --> ipcd


>+#ifdef MOZ_PROFILESHARING
>+NS_IMPL_ISUPPORTS7(nsCookieService,
>+                   nsICookieService,
>+                   nsICookieManager,
>+                   nsICookieManager2,
>+                   tmITransactionObserver,
>+                   nsIObserver,
>+                   nsIWebProgressListener,
>+                   nsISupportsWeakReference)
>+#else
> NS_IMPL_ISUPPORTS6(nsCookieService,
>                    nsICookieService,
>                    nsICookieManager,
>                    nsICookieManager2,
>                    nsIObserver,
>                    nsIWebProgressListener,
>                    nsISupportsWeakReference)
>+#endif

nit: how about using the INTERFACE_MAP macros directly instead?


>+  mTransService = do_GetService("@mozilla.org/transaction/service;1");

this contract-id has changed.


>+#ifdef MOZ_PROFILESHARING
>+  if (mSharingEnabled) {
>+    return EnsurePendingFlush();
>+  } else {

else after return is extraneous


>+nsCookieService::SerializeCookie(const nsCookie *aCookie,
>+                                 PRBool         aForceExpiry,
>+                                 PRUint8        *&aData,
>+                                 PRUint32       &aDataLen)

line up the a's maybe? ;-)


>+  // write the transaction data version first
>+  *NS_REINTERPRET_CAST(PRUint32*, dest) = kCurrentCookiesTransactionDataVersion;
>+  dest += sizeof(PRUint32);
>+  // ... and the transaction type
>+  *NS_REINTERPRET_CAST(PRUint32*, dest) = kCookiesAddTransaction;
>+  dest += sizeof(PRUint32);
>+
>+  // copy!
>+  // if we need to force expiry of the cookie, override its expiry time.
>+  if (aCookie->IsSession() || aForceExpiry) {
>+    *NS_REINTERPRET_CAST(nsInt64*, dest) = nsInt64(LL_MININT);
>+  } else {
>+    *NS_REINTERPRET_CAST(nsInt64*, dest) = aCookie->Expiry();
>+  }
>+  dest += sizeof(nsInt64);
>+  *NS_REINTERPRET_CAST(nsInt64*, dest) = aCookie->LastAccessed();
>+  dest += sizeof(nsInt64);
>+  // ... and make sure we override IsSession too, so the expirytime is checked
>+  *NS_REINTERPRET_CAST(PRBool*, dest) = !aForceExpiry && aCookie->IsSession();
>+  dest += sizeof(PRBool);
>+  *NS_REINTERPRET_CAST(PRBool*, dest) = aCookie->IsDomain();
>+  dest += sizeof(PRBool);
>+  *NS_REINTERPRET_CAST(PRBool*, dest) = aCookie->IsSecure();
>+  dest += sizeof(PRBool);
>+  *NS_REINTERPRET_CAST(nsCookieStatus*, dest) = aCookie->Status();
>+  dest += sizeof(nsCookieStatus);
>+  *NS_REINTERPRET_CAST(nsCookiePolicy*, dest) = aCookie->Policy();
>+  dest += sizeof(nsCookiePolicy);

might this be cleaner if you made |dest| be a pointer to PRUint32?
then you can just write things like:

  *dest++ = field;

of course you'll need something special for the nsInt64 cases, but
overall i think the code will be far more readable.


>+  // we use the knowledge that nsCookie stores its member strings
>+  // contiguously, Name() first, to save calls to memcpy().
>+  memcpy(dest, aCookie->Name().get(), strSize);

does this include a null byte?	should it?


>+  PRUint32 data[2];
>+  const size_t dataSize = 2*sizeof(PRUint32);

			  = sizeof(data);


>+#include "tmITransactionService.h"

ipcITransactionService.h


>+                      , public tmITransactionObserver

ipcITransactionObserver

etc for the renames.

sr=darin with these changes.

Comment 29

[[e-mail]]

oh hmm I didn't realize I still had this in your queue - thanks for the sr. I
was planning to update it with a few things I had in mind (just some small stuff
like using conrad's serializer helpers and stuff), so I may update this patch...

now that phase 3 has landed, I'll get to this soon :)

Comment 30

[Darin Fisher]

thanks, dwitte!

Comment 31

[[e-mail]]

no plans to drive this in, since i'm not sure the whole shared-profile thing is a road we're really going down, and this does cost codesize. would be happy to pick this up and work on it again if profilesharing becomes a focus.

-> WONTFIX pending future developments