Last Comment Bug 526789 - UMR in nsCookieService::CountCookiesFromHostInternal, GetCookiesFromHost, and GetCookieInternal when given empty host string
: UMR in nsCookieService::CountCookiesFromHostInternal, GetCookiesFromHost, and...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: dwitte@gmail.com
:
Mentors:
: 527437 (view as bug list)
Depends on:
Blocks: 536650
  Show dependency treegraph
 
Reported: 2009-11-05 09:37 PST by Blake Kaplan (:mrbkap)
Modified: 2009-12-24 00:10 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta5-fixed
.8-fixed


Attachments
patch v1 (12.89 KB, patch)
2009-11-09 13:30 PST, dwitte@gmail.com
cbiesinger: review+
Details | Diff | Splinter Review
patch v2 (13.12 KB, patch)
2009-11-09 16:28 PST, dwitte@gmail.com
dwitte: review+
dveditz: approval1.9.1.6-
Details | Diff | Splinter Review
patch v3 (14.09 KB, patch)
2009-11-11 16:26 PST, dwitte@gmail.com
dwitte: review+
mbeltzner: approval1.9.2+
dveditz: approval1.9.1.8+
Details | Diff | Splinter Review

Description Blake Kaplan (:mrbkap) 2009-11-05 09:37:22 PST
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.
Comment 1 dwitte@gmail.com 2009-11-09 10:43:06 PST
*** Bug 527437 has been marked as a duplicate of this bug. ***
Comment 2 dwitte@gmail.com 2009-11-09 10:47:57 PST
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.
Comment 3 dwitte@gmail.com 2009-11-09 13:30:53 PST
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 4 Christian :Biesinger (don't email me, ping me on IRC) 2009-11-09 13:53:50 PST
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)
Comment 5 dwitte@gmail.com 2009-11-09 16:28:31 PST
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.)
Comment 6 Phil Ringnalda (:philor) 2009-11-09 19:50:05 PST
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.
Comment 7 Reed Loden [:reed] (use needinfo?) 2009-11-09 20:19:17 PST
(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 8 Daniel Veditz [:dveditz] 2009-11-10 17:24:01 PST
Comment on attachment 411316 [details] [diff] [review]
patch v2

not going to make 1.9.1.6 -- you can try again for the next release after trunk/1.9.2 landing and testing.
Comment 9 dwitte@gmail.com 2009-11-11 16:26:22 PST
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.
Comment 11 dwitte@gmail.com 2009-11-25 13:24:06 PST
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 12 Mike Beltzner [:beltzner, not reading bugmail] 2009-11-25 13:46:21 PST
Comment on attachment 411834 [details] [diff] [review]
patch v3

a192=beltzner, I think the reward outweighs the risk here
Comment 14 dwitte@gmail.com 2009-11-30 16:28:40 PST
Comment on attachment 411834 [details] [diff] [review]
patch v3

See previous comments for risk details.
Comment 15 Daniel Veditz [:dveditz] 2009-12-21 15:17:18 PST
Comment on attachment 411834 [details] [diff] [review]
patch v3

Approved for 1.9.1.8, a=dveditz for release-drivers
Comment 17 William J. Edney 2009-12-23 20:34:53 PST
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

Note You need to log in before you can comment on or make changes to this bug.