481775 - Crash in [@ nsCookieService::RemoveCookieFromList(nsListIter&)]

Status: VERIFIED FIXED

(Core :: Networking: Cookies, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[timeless]

bug 516012 comment 5 hit this

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I can reproduce the crash with the Mozmill testcase Cris gave us. I will investigate and create a minimized testcase.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: 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	    }
1515	
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.

Steps:
1. Install Mozmill (https://addons.mozilla.org/de/firefox/addon/9018)
2. Clone the test repository (http://hg.mozilla.org/qa/mozmill-tests/)
3. Apply the patch
4. Run the test with Mozmill

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dan, could you help?

Comment 7

[[e-mail]]

Thanks for the awesome investigation, Henrik. I'm flat out right now but I'll take a look later this week...

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dan, would you have spare time to look at this crash in the next couple of days?

Comment 9

[[e-mail]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Tryserver builds can be found here:
https://build.mozilla.org/tryserver-builds/[e-mail]-RemoveCookie/

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dan, the fix looks good. No crash anymore when running the Mozmill test. You can go ahead. Thanks!

Comment 14

[[e-mail]]

Awesome!

Comment 15

[[e-mail]]

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

[Shawn Wilsher :sdwilsh]

Comment on attachment 406118 [details] [diff] [review]
patch v2

r=sdwilsh

Comment 17

[[e-mail]]

http://hg.mozilla.org/mozilla-central/rev/fbe123a48e7b

Comment 18

[[e-mail]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[[e-mail]]

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

[[e-mail]]

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

[Shawn Wilsher :sdwilsh]

Comment on attachment 407054 [details] [diff] [review]
additional test

r=sdwilsh

Comment 23

[[e-mail]]

Comment on attachment 407054 [details] [diff] [review]
additional test

http://hg.mozilla.org/mozilla-central/rev/8d79c4a770ea

Comment 24

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 406118 [details] [diff] [review]
patch v2

a192=beltzner, please also land the additional test

Comment 25

[[e-mail]]

Attachment: rolled-up patch (landed on 1.9.2)

Includes attachment 406118 [details] [diff] [review] and attachment 407054 [details] [diff] [review].

Comment 26

[[e-mail]]

Comment on attachment 409355 [details] [diff] [review]
rolled-up patch (landed on 1.9.2)

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/1e6c3f773674

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Daniel Veditz [:dveditz]]

Comment on attachment 409355 [details] [diff] [review]
rolled-up patch (landed on 1.9.2)

Approved for 1.9.1.6, a=dveditz for release-drivers

Comment 29

[[e-mail]]

Attachment: 1.9.1 branch patch

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ff0bc8ee6e31

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified on 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.6pre) Gecko/20091108 Shiretoko/3.5.6pre ID:20091108030959.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Aakash Desai [:aakashd]]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

[Aakash Desai [:aakashd]]

Comment on attachment 411201 [details] [diff] [review]
Mozmill Patch v2

Looks good and passes after a quick test run.

Comment 35

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Mozmill test has been landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/52451228aaf3
http://hg.mozilla.org/qa/mozmill-tests/rev/3b39a3549872