The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.9.3a1

Status

()

Core
Networking: Cookies
--
critical
VERIFIED FIXED
8 years ago
6 years ago

People

(Reporter: whimboo, Assigned: dwitte@gmail.com)

Tracking

(4 keywords)

Trunk
mozilla1.9.3a1
crash, testcase, verified1.9.1, verified1.9.2
Points:
---
Bug Flags:
blocking1.9.2 -
wanted1.9.2 +
in-testsuite +

Firefox Tracking Flags

(status1.9.2 beta2-fixed, status1.9.1 .6-fixed)

Details

(Whiteboard: [mozmill], crash signature)

Attachments

(4 attachments, 5 obsolete attachments)

(Reporter)

Description

8 years ago
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
Flags: blocking1.9.1?
(Reporter)

Updated

8 years ago
Flags: blocking1.9.1?
OS: Mac OS X → All
Hardware: x86 → All

Comment 1

8 years ago
bug 516012 comment 5 hit this
(Reporter)

Comment 2

8 years ago
I can reproduce the crash with the Mozmill testcase Cris gave us. I will investigate and create a minimized testcase.
(Reporter)

Comment 3

8 years ago
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.
(Reporter)

Comment 4

8 years ago
When we don't have a chance for an automated test we could add this one as crash test to our Mozmill test repository.
Keywords: testcase
(Reporter)

Comment 5

8 years ago
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	    }
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
(Reporter)

Comment 6

8 years ago
Dan, could you help?
(Assignee)

Comment 7

8 years ago
Thanks for the awesome investigation, Henrik. I'm flat out right now but I'll take a look later this week...
(Reporter)

Comment 8

8 years ago
Dan, would you have spare time to look at this crash in the next couple of days?
(Assignee)

Comment 9

8 years ago
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.
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
(Reporter)

Comment 10

8 years ago
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.
(Reporter)

Comment 11

8 years ago
Tryserver builds can be found here:
https://build.mozilla.org/tryserver-builds/hskupin@mozilla.com-RemoveCookie/
(Reporter)

Comment 12

8 years ago
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.
(Reporter)

Comment 13

8 years ago
Dan, the fix looks good. No crash anymore when running the Mozmill test. You can go ahead. Thanks!
(Assignee)

Comment 14

8 years ago
Awesome!
(Assignee)

Comment 15

8 years ago
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.
Attachment #405979 - Attachment is obsolete: true
Attachment #406118 - Flags: review?(sdwilsh)
Comment on attachment 406118 [details] [diff] [review]
patch v2

r=sdwilsh
Attachment #406118 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 17

8 years ago
http://hg.mozilla.org/mozilla-central/rev/fbe123a48e7b
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 18

8 years ago
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.
Attachment #406118 - Flags: approval1.9.2?
Attachment #406118 - Flags: approval1.9.1.5?
(Reporter)

Comment 19

8 years ago
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?
Status: RESOLVED → VERIFIED
status1.9.1: --- → ?
Flags: blocking1.9.2?
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Comment 20

8 years ago
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.
(Assignee)

Comment 21

8 years ago
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.)
Attachment #407054 - Flags: review?(sdwilsh)

Updated

8 years ago
Attachment #407054 - Flags: review?(sdwilsh) → review+
Comment on attachment 407054 [details] [diff] [review]
additional test

r=sdwilsh
(Assignee)

Comment 23

8 years ago
Comment on attachment 407054 [details] [diff] [review]
additional test

http://hg.mozilla.org/mozilla-central/rev/8d79c4a770ea
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Attachment #406118 - Flags: approval1.9.2? → approval1.9.2+
Comment on attachment 406118 [details] [diff] [review]
patch v2

a192=beltzner, please also land the additional test
(Assignee)

Comment 25

8 years ago
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].
Attachment #406118 - Attachment is obsolete: true
Attachment #407054 - Attachment is obsolete: true
Attachment #409355 - Flags: approval1.9.1.5?
Attachment #406118 - Flags: approval1.9.1.5?
(Assignee)

Comment 26

8 years ago
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
(Assignee)

Updated

8 years ago
status1.9.2: --- → final-fixed
(Reporter)

Comment 27

8 years ago
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
Flags: in-testsuite+
Keywords: verified1.9.2
Attachment #409355 - Flags: approval1.9.1.6? → approval1.9.1.6+
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
(Assignee)

Comment 29

8 years ago
Created attachment 410878 [details] [diff] [review]
1.9.1 branch patch

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

Updated

8 years ago
status1.9.1: ? → .6-fixed
(Reporter)

Comment 30

8 years ago
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.
Keywords: verified1.9.1
(Reporter)

Comment 31

8 years ago
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.
Attachment #400704 - Attachment is obsolete: true
Attachment #411172 - Flags: review?(adesai)
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.
Attachment #411172 - Flags: review?(adesai) → review-
(Reporter)

Comment 33

8 years ago
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.
Attachment #411172 - Attachment is obsolete: true
Attachment #411201 - Flags: review?(adesai)
Comment on attachment 411201 [details] [diff] [review]
Mozmill Patch v2

Looks good and passes after a quick test run.
Attachment #411201 - Flags: review?(adesai) → review+
(Reporter)

Comment 35

8 years ago
Mozmill test has been landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/52451228aaf3
http://hg.mozilla.org/qa/mozmill-tests/rev/3b39a3549872
(Reporter)

Updated

7 years ago
Whiteboard: [mozmill]
Crash Signature: [@ nsCookieService::RemoveCookieFromList(nsListIter&)]
You need to log in before you can comment on or make changes to this bug.