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!

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.)

this is the full monty, part b.

the beast for review, part c.

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 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.

thanks for the amazingly fast review! learnt something new about bitfields today. nice stuff.

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

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 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.

the full monty, part 3. obsoletes previous patches.

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 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)

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

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...

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 on attachment 120960 [details] [diff] [review] v3.1 complete patch sr=darin

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).

revving milestone -> 1.5a.

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 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 :)

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.

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.

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

This caused bug 394243.