Last Comment Bug 526789 - UMR in nsCookieService::CountCookiesFromHostInternal, GetCookiesFromHost, and GetCookieInternal when given empty host string
: UMR in nsCookieService::CountCookiesFromHostInternal, GetCookiesFromHost, and...
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To:
: Patrick McManus [:mcmanus]
: 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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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

Description User image 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 User image 2009-11-09 10:43:06 PST
*** Bug 527437 has been marked as a duplicate of this bug. ***
Comment 2 User image 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 User image 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 User image 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 User image 2009-11-09 16:28:31 PST
Created attachment 411316 [details] [diff] [review]
patch v2

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 User image Phil Ringnalda (:philor) 2009-11-09 19:50:05 PST
Backed you out on the trunk, for failures like on very nearly every everythingelse run after your push.
Comment 7 User image 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
> on very nearly every everythingelse run after your push.

You also caused a Txul regression. ;)
Comment 8 User image Daniel Veditz [:dveditz] 2009-11-10 17:24:01 PST
Comment on attachment 411316 [details] [diff] [review]
patch v2

not going to make -- you can try again for the next release after trunk/1.9.2 landing and testing.
Comment 9 User image 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 User image 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 User image 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 User image 2009-11-30 16:28:40 PST
Comment on attachment 411834 [details] [diff] [review]
patch v3

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

Approved for, a=dveditz for release-drivers
Comment 17 User image William J. Edney 2009-12-23 20:34:53 PST
All -

I just filed this regression for FF 3.6b5:

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.


- Bill

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