200632 - cookie rewrite, phase 3

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

okay! round three.

Objectives:
-----------
1) merge nsCookies and nsCookieManager into nsCookieService (yay)!
   this means nsCookieService now implements nsICookieService, nsICookieManager,
and nsICookieManager2.

2) improve mem efficiency of our base cookie storage class, and COMify it. while
this may seem like an evil word, it's a "good thing" here, because we need it
COMified for passing to the prompter and the UI. so doing it in our base class
saves conversions and just makes things nicer.

Notes:
------
a) making nsCookieService implement both the cookieservice and cookiemanager
contracts introduces a slight complication. if nsICookieService and
nsICookieManager have separate contractid's, then if you do_GetService both of
them, you end up with two instances of nsCookieService, which is wrong. I asked
bz about this and he suggested making the two contractid's the same; that is, we
now have one contract that specifies we implement both the cookieservice and
cookiemanager interfaces. since nsICookieManager is frozen, I'm not sure if I'm
allowed to change its contractid - so instead I've changed (the unfrozen)
nsICookieService's contractid to be the same as nsICookieManager's. I could use
some feedback on this approach ;)

b) our rewritten nsCookie class implements the frozen nsICookie (as before), but
this interface slightly bites. so I've made a new interface, nsICookie2, that
fixes the annoying mistakes. I haven't updated any consumers to use it yet, but
that will come when we decide to attack wallet...

c) I should explain the new nsCookie class. it contiguously allocates its member
strings and stores ptrs to them, and hands them out as string types when
required. we use flat strings for now, because some consumers require that
(logging and writing-to-file). the class makes use of bitmasking, and also has a
custom refcount impl (since we use a PRInt16 instead of PRInt32 refcount). so
this class does a pretty good job with mem efficiency, though I have some ideas
to make it better in future - perhaps when we implement arena allocation &
placement new.

d) oh, I merged nsCookiePrefObserver into nsCookieService too, since we really
don't need it to be separate anymore.

e) alecf, don't be upset at me for not using COMArray :) I stuck to nsVoidArray
with manual addrefing and releasing, for two very good reasons. i) it's more
convenient to store the cookies as a concrete nsCookie type rather than an
nsICookie/nsICookie2 type: this is because nsCookie has some "fast" non-xpcom
getters on it, for use in local code. and COMPtr's won't work with nsCookie
because the cast to nsISupports is ambiguous. ii) when we switch to hashtables
(very soon), we need to manually addref/release anyway, so we might as well
start now. and we'll need to roll our own nsISimpleEnumerator then anyway, so
there's no incentive to use COMArray's inbuilt one.

ok, comments welcome please!

Comment 1

[[e-mail]]

Attachment: baseline patch

I've split phase 3 up into a few parts, for reviewing convenience:

a) a "baseline" patch, that makes no functional changes but simply moves the
code from nsCookies and nsCookieManager into nsCookieService, renames
functions, and shifts those horrible global vars (sCookieList,
gCookiePrefObserver) into member vars. this patch is enormous and boring,
because there are no changes of interest, so the idea is you can just ignore
it. it's there for reference.

b) the actual phase 3 patch for checkin. this is also very hard to read because
it includes all the baseline stuff...

c) a diffdiff between the actual patch and the "baseline". this only shows the
changes of interest, so this one should be a lot easier to review. (note that
~1000 lines is just code movement from the pref observer, since it's now part
of the cookieservice.)

Comment 2

[[e-mail]]

Attachment: v1 complete patch

this is the full monty, part b.

Comment 3

[[e-mail]]

Attachment: v1 diffdiff (wrt baseline)

the beast for review, part c.

Comment 4

[[e-mail]]

oh, one last note. there are a few questions in the code i'm not sure about,
which i've marked with XXX comments. when you get around to reviewing, could you
take a look at them and see what you think? (only the one's i've added, i.e. +'ed).

thanks!

Comment 5

[Darin Fisher]

Comment on attachment 119424 [details] [diff] [review]
v1 diffdiff (wrt baseline)

>+#include "nsWeakReference.h" // XXX required?

if not, then remove it =)


>+class nsCookie : public nsICookie2
...
>+    NS_IMETHOD QueryInterface(REFNSIID aIID, void** aInstancePtr);
>+    NS_IMETHOD_(nsrefcnt) AddRef(void);
>+    NS_IMETHOD_(nsrefcnt) Release(void);

use NS_DECL_ISUPPORTS


>+++ mozilla/extensions/cookie/nsCookie.cpp	2003-04-03 04:53:48.000000000 -0800

>+  // bitmask the bools, and pack them into one PRUint8
>+  mCookieFlags = ((aIsSession != PR_FALSE) << COOKIE_ISSESSION_BIT) |
>+                 ((aIsDomain  != PR_FALSE) << COOKIE_ISDOMAIN_BIT ) |
>+                 ((aIsSecure  != PR_FALSE) << COOKIE_ISSECURE_BIT );
>+
>+  // bitmask the two p3p flags, and pack them into one PRUint8.
>+  // each flag needs 3 bits.
>+  mP3PFlags = (aStatus << COOKIE_STATUS_BIT) |
>+              (aPolicy << COOKIE_POLICY_BIT);
> }

what about using bit fields instead to avoid all this bit shifting?

  PRUint32 mIsSession : 1;
  PRUint32 mIsDomain  : 1;
  PRUint32 mIsSecure  : 1;
  PRUint32 mStatus    : 4;
  PRUint32 mPolicy    : 4;

the compiler will create almost exactly the same code you have
manually created with all the bit shifting.. and the compiler might
even have ways of optimizing it further in some cases.


>+// custom addref/release impl
>+NS_IMETHODIMP_(nsrefcnt) nsCookie::AddRef(void)
>+NS_IMETHODIMP_(nsrefcnt) nsCookie::Release(void)

stock macros should work here.

Comment 6

[[e-mail]]

Attachment: v2 complete patch

thanks for the amazingly fast review!

learnt something new about bitfields today. nice stuff.

Comment 7

[[e-mail]]

Attachment: v2 diffdiff (wrt baseline)

Comment 8

[Darin Fisher]

semicolon after NS_IMPL_ISUPPORTSX macro is not needed.  more comments later...

Comment 9

[Darin Fisher]

Comment on attachment 119422 [details] [diff] [review]
baseline patch

sr=darin

in order to have one class implement a pair of CID/COntractIDs (in this case
the cookie manager and cookie service IDs), you should create a custom
Constructor method instead of using NS_GENERIC_FACTORY_CONSTRUCTOR.  
NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR might be the right thing to use in
this case or you could roll your own.

Comment 10

[Darin Fisher]

Comment on attachment 119482 [details] [diff] [review]
v2 diffdiff (wrt baseline)

>+  mPrefBranch = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
>   if (NS_FAILED(rv)) return rv;

does cookies really depend on prefs?  or can it run without
the pref service.  i don't think it should have to be a fatal
error to not have a pref service.


>+      // cast the nsCookie to an nsICookie2.
>+      // XXX is this valid - does the frozen nsICookieManager interface
>+      // require that we hand these out as nsICookies, or can we assume
>+      // the consumer will perform a QI (and _not_ a cast)?
>+      nsICookie2 *cookieInList = NS_STATIC_CAST(nsICookie2*, NS_STATIC_CAST(nsCookie*, mCookieList.ElementAt(mCookieIndex++)));

since nsCookie : nsICookie2 : nsICookie this is ok, but why not cast to
nsICookie instead?


otherwise, this patch looks great =)


sr=darin with these issues addressed.  also, please check to see who
depends on nsCookie supporting weak reference.	maybe it is just old
baggage that we can do away with.

Comment 11

[[e-mail]]

Attachment: v3 complete patch

the full monty, part 3. obsoletes previous patches.

Comment 12

[[e-mail]]

Attachment: v3 diffdiff (wrt v2)

okay, so this is becoming slightly ridiculous, but here goes...

this is a diffdiff between v2 and v3 complete patches, so it shows the changes
between those versions. addresses darin's comments (thanks!). notes:

1) removed weak refs from nsCookie.h. the only relevant code snippet I could
find in our codesbase, was some usage of nsIMutableArray in
nsCookiePromptService.cpp:

  nsCOMPtr<nsIMutableArray> objects;
  (...)
  rv = objects->AppendElement(aCookie, PR_FALSE);

which explicitly declares strong refs... so <shrug> i have no idea why we ever
had or currently need weak refs. poof.

2) backed out the contractid changes, so we have unique nsICookieService &
nsICookieManager contractid's again. implemented the modulefactory portion as a
singleton instead, per darin's suggestion, so we use stock
NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR along with two new functions,
|nsCookieService::GetSingleton()| and |nsCookieService::FreeSingleton()|. most
of the impl was taken from other examples in the codebase, so I'm not positive
it's correct... a quick sanity check would be real nice. it does appear to work
though. ;)

3) added a method to init prefs to their defaults, in case we can't get the
prefservice. this removes the dependency on mPrefBranch so we no longer die if
it's not available. (yes i know i could've just inited the prefs in the ctor,
but i factored it into a function to keep the #ifdef MOZ_PHOENIXes in one
place)

4) removed semicolons per darin's nit. ;)

5) fixed a minor list bug (sigh...) in RemoveExpiredCookies(). we need to
include session cookies in the search for the oldest one.

that's it! i'll carry over sr=darin and request r=alecf (though darin, if you
could look at those modulefac changes, that'd be great).

i spoke with darin and we decided that once this is in, we can merge cookies
into necko (i.e. all dependencies on permissions code are failure-tolerant).
which will give us a footprint & perf win because nsCookieHTTPNotify can die...
yay!

(not to mention we get to cvs remove nsCookies & nsCookieManager!)

Comment 13

[[e-mail]]

Comment on attachment 120069 [details] [diff] [review]
v3 complete patch

carrying over sr=darin

alecf: just to be clear... when you review this, you want to review the "v2
diffdiff (wrt baseline)" and then "v3 diffdiff (wrt v2)". don't look at the
whole thing, it's really hard to review.

the baseline & complete patches are there for reference (see comment 1 for why
i split them up)

Comment 14

[[e-mail]]

Attachment: v3.1 complete patch

updated with a few improvements, comments to follow...

Comment 15

[[e-mail]]

Attachment: v3.1 diffdiff (wrt v3)

ok, here's a diffdiff of what changed between v3.1 and v3. man, these
version-diffdiffs are becoming insane ;)

explanations (in order of changes):
1) quieted some silly signed/unsigned compiler warnings (cookies is
warning-free now).
2) removed a patch of dead code. this is live code in current trunk, but it's
really not necessary (must be leftover cruft) because we deal with null
aFirstURI in a graceful and correct manner. i must have missed deleting it at
an earlier stage, sorry...
3) quieted a warning that we added in a while back - one of the cookie prefs is
optional, so pretty much everyone was seeing the warning "error reading cookie
prefs", which isn't good. so we just ignore the rv for that one pref.
4) changed the mailnews check in a subtle way - we now check currentURI if
firstURI is null (previously we skipped all checks if firstURI was null).
shouldn't really make a difference to anything, just makes slightly more sense.

5) fixed a glaring omission in CheckPath() - we weren't getting the path from
the host before operating on it! that was stupid. (we didn't see this bug
because it's a rarely, if never, executed codepath).

other than that it's testing out well (the testsuite i just submitted is happy
with it, and the singleton creation/destruction is working just fine).

i'll re-request sr=darin because the changes here probably need looking at...

Comment 16

[Darin Fisher]

maybe bracket this for readability:

   if (mCookiesDisabledForMailNews &&
+      (aFirstURI &&
+       IsFromMailNews(firstURIScheme) ||
        IsFromMailNews(currentURIScheme))) {

   if (mCookiesDisabledForMailNews &&
+      ((aFirstURI && IsFromMailNews(firstURIScheme)) ||
        IsFromMailNews(currentURIScheme))) {

otherwise the v3.1 diffdiff looks fine to me.

Comment 17

[Darin Fisher]

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

sr=darin

Comment 18

[[e-mail]]

alecf, darin:

the v3.1 patch has a few important fixes that should probably ship with 1.4b
(separate from the refactoring etc). since it hasn't gone in yet, what do you
think would be best?

i can either split out said fixes into a small patch against trunk, and get
those into 1.4b - or, (more work but hopefully a little better) if we can get
the full v3.1 patch reviewed and approved in the next few days, we could check
the whole thing in. if you think drivers will have it ;)

the latter option is fairly low-risk since the bulk of the patch is trivial code
movement, and i've been testing it for a while now. it would also make the 1.4
branch a little easier to maintain (since it'll be our new stable branch when
firebird comes along in 1.5).

Comment 19

[[e-mail]]

revving milestone -> 1.5a.

Comment 20

[[e-mail]]

Comment on attachment 119482 [details] [diff] [review]
v2 diffdiff (wrt baseline)

alecf said he might take a look at this soon, so... two comments:

1) i've since learned that declaring |inline foo();| in the header and then
putting the fn body in the cpp is wrong - it won't get inlined (unless it's
static, but that's another story). i do this quite a bit in nsCookieService.h.
i'm not going to post a new patch to change that, but consider those bodies
moved into the .h or otherwise uninlined.

2) the bitmasking in nsCookie may be overkill; i'll leave that to alecf's
judgement. if i change the bitmasked bools into packedbools (and leave
nsCookieStatus & nsCookiePolicy bitmasked as one byte), we increase
sizeof(nsCookie) by 4 bytes. considering the overall sizeof a typical nsCookie
is ~80 bytes, we're talking 5%, or 1k of 30k total (for 300 cookies). we use
some of the bools (in particular mIsSession) frequently inside loops.

the patches for review are "v2 diffdiff" and "v3 diffdiff". the "v3.1 diffdiff"
is already in the tree, since we backported it. see comment 13.

Comment 21

[Alec Flett]

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

ok, finally starting to take a look at this.. 

regarding StrBlockCopy() - are you ready to switch to arenas now? :)

+    PRUint32 mIsSession : 1;
+    PRUint32 mIsDomain  : 1;
+    PRUint32 mIsSecure  : 1;
+    PRUint32 mStatus	 : 3;
+    PRUint32 mPolicy	 : 3;
+    PRUint32 mRefCnt	 : 16;

maybe make mRefCnt:16 come first? I dunno, I just wonder if it might make a
difference in case compilers do funny alignment?

+  } else if (!nsCRT::strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID)) {
+    // which pref changed?
+    NS_ConvertUCS2toUTF8 pref(aData);

pref names are ASCII - really I need to document that better, but you can use
NS_LossyConvertUCS2toASCII here

the rest of this looks really good! sr=alecf - if you want to use arenas after
all, let me know :)

Comment 22

[[e-mail]]

oh neat, thanks alec! :)

so I thought about arenas, but after asking a few folk I decided against it. for
permissions it kinda made sense because the objects themselves were tiny and
they were pretty much static. but here, cookies will be deleted pretty
frequently - so do we really want to leak that stuff? if you have any ideas i
didn't think of, do let me know. (i don't see how fixed size allocator would
mesh well with the strings either)

there may be some slight codesize bloat here due to the inlined nsCookie class
members (notably nsDependentCString ctors and the bitmasking). i'll keep an eye
on it after I check in, and i'll file a followup bug if things need attention.

Comment 23

[[e-mail]]

checked in, with some changes to the #include sections (used a bunch of forward
declarations instead), and removed the erroneous inline qualifiers. I also
changed a few other occurrences of NS_ConvertUCS2toUTF8 in ::Observe methods per
alecf's comment.

codesize numbers for reference:
+ 354 on winnt beast
+1328 on comet (2.96)
+ 469 on luna  (3.2)

so nothing major, but i'll play with it as time permits and see if I can refine
things a little more.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So, um, did you notice that this caused a major leak increase on tinderbox?  I
filed bug 209571.

Comment 25

[[e-mail]]

This caused bug 394243.