Last Comment Bug 481775 - Crash in [@ nsCookieService::RemoveCookieFromList(nsListIter&)]
: Crash in [@ nsCookieService::RemoveCookieFromList(nsListIter&)]
: crash, testcase, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9.3a1
Assigned To:
: Patrick McManus [:mcmanus]
Depends on:
  Show dependency treegraph
Reported: 2009-03-05 18:15 PST by Henrik Skupin (:whimboo)
Modified: 2011-06-13 10:01 PDT (History)
10 users (show)
mbeltzner: blocking1.9.2-
mbeltzner: wanted1.9.2+
hskupin: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Simplified Mozmill test (1.84 KB, patch)
2009-09-15 01:04 PDT, Henrik Skupin (:whimboo)
no flags Details | Diff | Splinter Review
GDB stack (14.01 KB, text/plain)
2009-09-15 03:19 PDT, Henrik Skupin (:whimboo)
no flags Details
patch v1 (7.95 KB, patch)
2009-10-12 22:33 PDT,
no flags Details | Diff | Splinter Review
patch v2 (37.82 KB, patch)
2009-10-13 15:58 PDT,
sdwilsh: review+
mbeltzner: approval1.9.2+
Details | Diff | Splinter Review
additional test (4.55 KB, patch)
2009-10-19 10:53 PDT,
sdwilsh: review+
Details | Diff | Splinter Review
rolled-up patch (landed on 1.9.2) (42.60 KB, patch)
2009-10-30 10:09 PDT,
dveditz: approval1.9.1.6+
Details | Diff | Splinter Review
1.9.1 branch patch (29.63 KB, patch)
2009-11-06 14:53 PST,
no flags Details | Diff | Splinter Review
Mozmill crash test (1.81 KB, patch)
2009-11-09 04:55 PST, Henrik Skupin (:whimboo)
mozaakash: review-
Details | Diff | Splinter Review
Mozmill Patch v2 (3.37 KB, patch)
2009-11-09 09:06 PST, Henrik Skupin (:whimboo)
mozaakash: review+
Details | Diff | Splinter Review

Description User image Henrik Skupin (:whimboo) 2009-03-05 18:15:13 PST
I don't really know when this happened but I believe it was while setting a SSL exception.

Crash stack: bp-1d527d25-dd0c-4adb-ad01-36fa12090305

Source line:   if (!aIter.current->IsSession() && mStmtDelete) {

0  	xul.dll  	nsCookieService::RemoveCookieFromList  	netwerk/cookie/src/nsCookieService.cpp:2180
1 	xul.dll 	xul.dll@0x2d35e9 	
2 	xul.dll 	nsCookieService::SetCookieInternal 	netwerk/cookie/src/nsCookieService.cpp:1416
3 	xul.dll 	nsCookieService::SetCookieStringInternal 	netwerk/cookie/src/nsCookieService.cpp:771
4 	xul.dll 	nsCookieService::SetCookieStringFromHttp 	netwerk/cookie/src/nsCookieService.cpp:726
5 	xul.dll 	nsHttpChannel::SetCookie 	netwerk/protocol/http/src/nsHttpChannel.cpp:4619
6 	xul.dll 	nsHttpChannel::ProcessResponse 	netwerk/protocol/http/src/nsHttpChannel.cpp:853
7 	xul.dll 	nsHttpChannel::OnStartRequest 	netwerk/protocol/http/src/nsHttpChannel.cpp:4739
8 	xul.dll 	nsInputStreamPump::OnStateStart 	netwerk/base/src/nsInputStreamPump.cpp:439
9 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:395
10 	xul.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:111
11 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:510
12 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:170
13 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:192
14 	nspr4.dll 	PR_GetEnv 	
15 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:87
16 	firefox.exe 	firefox.exe@0x2197 	
17 	kernel32.dll 	BaseProcessStart 	

The following is a crash on OS X which was reported a while back with FF3.1b2:

Crash report: bp-54d8488a-c92f-4067-87af-6d9732090301

0  	XUL  	nsCookieService::RemoveCookieFromList  	netwerk/cookie/src/nsCookieService.cpp:2180
1 	XUL 	nsCookieService::AddInternal 	netwerk/cookie/src/nsCookieService.cpp:1494
2 	XUL 	nsCookieService::SetCookieInternal 	netwerk/cookie/src/nsCookieService.cpp:1416
3 	XUL 	nsCookieService::SetCookieStringInternal 	netwerk/cookie/src/nsCookieService.cpp:771
4 	XUL 	nsCookieService::SetCookieString 	netwerk/cookie/src/nsCookieService.cpp:715
5 	XUL 	nsHTMLDocument::SetCookie 	content/html/document/src/nsHTMLDocument.cpp:1770
Comment 1 User image timeless 2009-09-15 00:15:53 PDT
bug 516012 comment 5 hit this
Comment 2 User image Henrik Skupin (:whimboo) 2009-09-15 00:31:55 PDT
I can reproduce the crash with the Mozmill testcase Cris gave us. I will investigate and create a minimized testcase.
Comment 3 User image Henrik Skupin (:whimboo) 2009-09-15 01:04:08 PDT
Created attachment 400704 [details] [diff] [review]
Simplified Mozmill test

This simplified test demonstrates the crash. It depends on the Private Browsing mode. Without it the crash does not occur when deleting the history from the CRH dialog afterward.
Comment 4 User image Henrik Skupin (:whimboo) 2009-09-15 01:05:53 PDT
When we don't have a chance for an automated test we could add this one as crash test to our Mozmill test repository.
Comment 5 User image Henrik Skupin (:whimboo) 2009-09-15 03:19:38 PDT
Created attachment 400727 [details]
GDB stack

This is a full stack given by gdb. We crash in RemoveCookieFromList because nsCookie::IsSession gets a null pointer as session in data.iter.current:

(gdb) l
1507	      if (mCookieCount >= mMaxNumberOfCookies) {
1508	        // find the position of the oldest cookie, and remove it
1509	        data.oldestTime = LL_MAXINT;
1510	        FindOldestCookie(data);
1511	        oldCookie = data.iter.current;
1512	        RemoveCookieFromList(data.iter);
1513	      }
1514	    }
1516	    // if we deleted an old cookie, notify consumers
(gdb) p data.iter
$5 = {
  entry = 0x0, 
  prev = 0x0, 
  current = 0x0

The easiest way to get the crash into the debugger is to place a 'controller.window.alert("");' before the first statement in testTabRestoration. That will give you as much time as you need to attach the debugger to the Firefox process. You definitely have to run the test from the command line. The crash doesn't occur in the IDE.

1. Install Mozmill (
2. Clone the test repository (
3. Apply the patch
4. Run the test with Mozmill
Comment 6 User image Henrik Skupin (:whimboo) 2009-09-15 03:23:55 PDT
Dan, could you help?
Comment 7 User image 2009-09-15 09:42:16 PDT
Thanks for the awesome investigation, Henrik. I'm flat out right now but I'll take a look later this week...
Comment 8 User image Henrik Skupin (:whimboo) 2009-09-29 11:19:32 PDT
Dan, would you have spare time to look at this crash in the next couple of days?
Comment 9 User image 2009-10-12 22:33:34 PDT
Created attachment 405979 [details] [diff] [review]
patch v1

OK, finally got around to this. Patch should fix it - I haven't tested though. Henrik, do you have a build env handy to test? If not, I'll see what I can do.
Comment 10 User image Henrik Skupin (:whimboo) 2009-10-13 01:29:07 PDT
Thanks Dan! I have submitted your patch to the tryserver. Once the builds are ready I will have a test-run with Mozmill to check if the problem is fixed.
Comment 11 User image Henrik Skupin (:whimboo) 2009-10-13 06:23:10 PDT
Tryserver builds can be found here:
Comment 12 User image Henrik Skupin (:whimboo) 2009-10-13 06:33:31 PDT
This tryserver build is broken and already crash on startup. Seems like I started the build process at the wrong time. I will build locally.
Comment 13 User image Henrik Skupin (:whimboo) 2009-10-13 07:58:47 PDT
Dan, the fix looks good. No crash anymore when running the Mozmill test. You can go ahead. Thanks!
Comment 14 User image 2009-10-13 10:33:00 PDT
Comment 15 User image 2009-10-13 15:58:10 PDT
Created attachment 406118 [details] [diff] [review]
patch v2

This makes the cookieservice a bit more stateful with its DB's. With private browsing, we want to have two states -- default and private -- that we switch between, and that encapsulates both the in-memory hash and the on-disk DB. (The on-disk DB connection for private browsing will always be null, by design.)

With this approach, there's much less chance that things can get out of sync.

This also tweaks some other stuff to be a bit more robust, and makes it so that we don't actually close the on-disk DB when entering PB -- we just switch states, and keep it open. This makes it faster to switch back, but means we have to decide what it means to do things like close/load an on-disk DB while in PB. In most cases, we want to use the DB state that we're currently in, but in a few places we force or assert the default state.
Comment 16 User image Shawn Wilsher :sdwilsh 2009-10-16 11:26:53 PDT
Comment on attachment 406118 [details] [diff] [review]
patch v2

Comment 18 User image 2009-10-16 14:37:25 PDT
Comment on attachment 406118 [details] [diff] [review]
patch v2

We want this on 1.9.2, and probably 1.9.1 as well. crash-stats says 298 people have hit this in the wild -- not exactly a topcrasher, but if this patch bakes okay, we should fix it.
Comment 19 User image Henrik Skupin (:whimboo) 2009-10-18 05:30:55 PDT
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091016 Minefield/3.7a1pre ID:20091016035239

Once it has been fixed on trunk I will also checkin the Mozmill crash test. Dan or how complicated would it to convert it to a Mochitest?
Comment 20 User image 2009-10-19 09:37:51 PDT
It's hard to test the general case of whether the cookie count is in sync with the host table, but I can certainly add an xpcshell crashtest for this bug -- good idea. I'll roll a patch.
Comment 21 User image 2009-10-19 10:53:33 PDT
Created attachment 407054 [details] [diff] [review]
additional test

Adds an xpcshell test to make sure purging doesn't crash when going into and out of PB. (Also adds a couple assertions to that effect, and tweaks the pref-sanitizing code to not allow zero cookie counts - that's bad too.)
Comment 22 User image Shawn Wilsher :sdwilsh 2009-10-20 09:53:21 PDT
Comment on attachment 407054 [details] [diff] [review]
additional test

Comment 23 User image 2009-10-22 13:56:22 PDT
Comment on attachment 407054 [details] [diff] [review]
additional test
Comment 24 User image Mike Beltzner [:beltzner, not reading bugmail] 2009-10-29 08:41:18 PDT
Comment on attachment 406118 [details] [diff] [review]
patch v2

a192=beltzner, please also land the additional test
Comment 25 User image 2009-10-30 10:09:58 PDT
Created attachment 409355 [details] [diff] [review]
rolled-up patch (landed on 1.9.2)

Includes attachment 406118 [details] [diff] [review] and attachment 407054 [details] [diff] [review].
Comment 26 User image 2009-10-30 10:11:03 PDT
Comment on attachment 409355 [details] [diff] [review]
rolled-up patch (landed on 1.9.2)
Comment 27 User image Henrik Skupin (:whimboo) 2009-11-01 14:41:32 PST
Verified fixed with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2b2pre) Gecko/20091030 Namoroka/3.6b2pre ID:20091030043032
Comment 28 User image Daniel Veditz [:dveditz] 2009-11-04 16:59:38 PST
Comment on attachment 409355 [details] [diff] [review]
rolled-up patch (landed on 1.9.2)

Approved for, a=dveditz for release-drivers
Comment 30 User image Henrik Skupin (:whimboo) 2009-11-09 04:53:19 PST
Verified on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv: Gecko/20091108 Shiretoko/3.5.6pre ID:20091108030959.
Comment 31 User image Henrik Skupin (:whimboo) 2009-11-09 04:55:28 PST
Created attachment 411172 [details] [diff] [review]
Mozmill crash test

I would like to see this crash test with Mozmill in our repository. It will not be executed with the other tests. Users have to explicitly select this folder.
Comment 32 User image Aakash Desai [:aakashd] 2009-11-09 08:51:28 PST
Comment on attachment 411172 [details] [diff] [review]
Mozmill crash test

Runtime check works fine on OSX and XP on Namoroka.

I'm minus'ing this because I think the coding style of how this is implemented is fine, but warrants some chatter about if this is how it should be done in the future. For example, we have a testClearFormHistory testcase in the testFormManager folder. Also, the name of this testscript is testClearRecentHistory, but it incorporates Private Browsing mode as well. I like your implementation of cutting them it out between different test modules, but the name should probably be changed to reflect the use of private browsing. 

A couple other things:

- there needs to be a comment at the top of each test module and on top of things done within the test modules per the coding style guidelines we've outlined before.

- I don't see the UtilsAPI module used in this testscript, please remove it.

- testTabRestoration function has a "{" on the same line as the test module call. Please move that to the next line, so it's in sync with the rest of the testscript.
Comment 33 User image Henrik Skupin (:whimboo) 2009-11-09 09:06:30 PST
Created attachment 411201 [details] [diff] [review]
Mozmill Patch v2

I simply forgot a hg qrefresh. Now the patch is updated. I have fixed the small nits and merged the two test functions into one which makes it easier to read.
Comment 34 User image Aakash Desai [:aakashd] 2009-11-09 09:43:55 PST
Comment on attachment 411201 [details] [diff] [review]
Mozmill Patch v2

Looks good and passes after a quick test run.
Comment 35 User image Henrik Skupin (:whimboo) 2009-11-09 10:24:45 PST
Mozmill test has been landed:

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