Since I don't actually believe we purposely pass these functions an empty string, I think I ran out of memory allocating a string and we passed that empty string to GetCookiesFromHost. While the request is nonsensical, we should deal more gracefully with this condition.
This applies to 1.9.1, 1.9.2, and trunk. It may be the cause of some of the crashreports in cookieservice, too. We should fix this ASAP and see what the stats say.
Created attachment 411270 [details] [diff] [review] patch v1 Check and assert the precondition that host is never empty or ".", and add xpcshell tests to that effect.
Comment on attachment 411270 [details] [diff] [review] patch v1 +++ b/netwerk/cookie/src/nsCookieService.cpp + NS_ENSURE_TRUE(!aDomain.IsEmpty() && !aDomain.EqualsLiteral("."), NS_ERROR_INVALID_ARG); 80 characters, please (in a few places)
Created attachment 411316 [details] [diff] [review] patch v2 http://hg.mozilla.org/mozilla-central/rev/a696d331ebf7 Requesting approval for 1.9.2 and 1.9.1 branch - this will probably clear up a bunch of crashes. (Not really enough to measure from trunk usage, though, so we should get this on 1.9.2 if we want to get an accurate measurement.)
Backed you out on the trunk, for failures like http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257819241.1257823829.5261.gz&fulltext=1 on very nearly every everythingelse run after your push.
(In reply to comment #6) > Backed you out on the trunk, for failures like > http://tinderbox.mozilla.org/showlog.cgi?log=Firefox-Unittest/1257819241.1257823829.5261.gz&fulltext=1 > on very nearly every everythingelse run after your push. You also caused a Txul regression. ;)
Comment on attachment 411316 [details] [diff] [review] patch v2 not going to make 220.127.116.11 -- you can try again for the next release after trunk/1.9.2 landing and testing.
Created attachment 411834 [details] [diff] [review] patch v3 Tests failed (in other parts of the tree) because of the behavior change here. Having empty hosts is common (e.g. file:// URI's). This patch is more conservative; it maintains behavior for CountCookiesFromHost and GetCookiesFromHost by having them handle the trivial case. For Add and Remove, the cookie data is most likely coming from either the cookieservice itself or some other cookie data source, so we probably want to throw rather than silently fail.
Note for branch approval - reward: clears up a bunch of UMRs, and potentially crashes, caused when passing empty host strings into cookieservice API methods; risk: we now explicitly throw in two of those methods, where previously we pretended things were fine. This could break addons if they're not catching those exceptions, but this is arguably the right thing to do.
Comment on attachment 411834 [details] [diff] [review] patch v3 a192=beltzner, I think the reward outweighs the risk here
Comment on attachment 411834 [details] [diff] [review] patch v3 See previous comments for risk details.
Comment on attachment 411834 [details] [diff] [review] patch v3 Approved for 18.104.22.168, a=dveditz for release-drivers
All - I just filed this regression for FF 3.6b5: https://bugzilla.mozilla.org/show_bug.cgi?id=536650 I'm not sure if that is related to this, but out of all of the issues that were resolved for FF 3.6b5, this bug seemed the most likely. Apologies in advance if that is not the case. Cheers, - Bill