198270 - Trunk crash [@ COOKIE_Write][@ cookie.dll] (nsCookies.cpp line 1734)

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P1)

Description

[Jay Patel [:jay]]

A few people are seeing crashes with the latest Trunk builds started on 3/17. 
We don't have too much Talkback data:

15   COOKIE_Write   9 

 
 	Source File : c:/builds/seamonkey/mozilla/extensions/cookie/nsCookies.cpp line
: 1734
 
====================================================================================================
     Count   Offset    Real Signature
[ 4   COOKIE_Write 8a73ff5c - COOKIE_Write ]
[ 2   COOKIE_Write 63aa99a8 - COOKIE_Write ]
[ 2   COOKIE_Write 0f1634b0 - COOKIE_Write ]
[ 1   COOKIE_Write a04f5cc5 - COOKIE_Write ]
 
     Crash date range: 2003-03-17 to 2003-03-18
     Min/Max Seconds since last crash: 51 - 4491
     Min/Max Runtime: 51 - 4542
 
     Count   Platform List 
     7   Windows NT 5.0 build 2195
     2   Windows NT 5.1 build 2600
 
     Count   Build Id List 
     5   2003031714
     4   2003031808
 
     No of Unique Users         3
 
 Stack trace(Frame) 

	 COOKIE_Write	[c:/builds/seamonkey/mozilla/extensions/cookie/nsCookies.cpp 
line 1734] 
	 nsTimerImpl::Fire	[c:/builds/seamonkey/mozilla/xpcom/threads/nsTimerImpl.cpp 
line 383] 
	 nsTimerManager::FireNextIdleTimer
[c:/builds/seamonkey/mozilla/xpcom/threads/nsTimerImpl.cpp  line 616] 
	 nsAppShell::Run	[c:/builds/seamonkey/mozilla/widget/src/windows/nsAppShell.cpp
 line 176] 
	 nsAppShellService::Run
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp  line 480] 
	 main1	[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1284] 
	 main	[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1642] 
	 WinMain	[c:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp  line 1663] 
	 WinMainCRTStartup()  
	 KERNEL32.DLL + 0xd326 (0x77e7d326)   
 
     (18226059)	Comments: After input of Data in login fields the Mozilla
crashes.  All Builds from 17 and 18.


However, there were a few changes to nsCookies.cpp last week by dougt.  Making
this zt4newcrash for no...until we understand what caused this regression.

Comment 1

[Doug Turner (:dougt)]

dwitte, could this be your changes?  Let me know if you need any help or
additional data.

Comment 2

[[e-mail]]

dougt: hmm, looks like the crash is happening when you dereference 'strm'.
recommend you add a nullcheck around
http://lxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookies.cpp#1723:

if (NS_FAILED(rv)) return rv;

although i'm not sure why that's failing; shouldn't NS_NewLocalFileOutputStream
be one of those things that "hardly ever fails"?

Comment 3

[[e-mail]]

Attachment: sprinkle some checks around

adds NS_FAILED checks to Read and Write. it's sad we need these checks for
AppendNative, since it's such a simple op, but I think we do...

r/sr?

Comment 4

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 117775 [details] [diff] [review]
sprinkle some checks around

This code looks good.
Only I would like to know why we fail. That would mean that cookies are not
always saved. From the comment on TB, it looks like normal users, no kiosk or
anything.

(hmm, why the removals of empty lines?)

Comment 5

[[e-mail]]

Comment on attachment 117775 [details] [diff] [review]
sprinkle some checks around

thanks mvl!

-> darin for sr

Comment 6

[[e-mail]]

cc'ing darin (not sure if this is already done, since it's component=cookies)

darin: topcrash fix attached.

Comment 7

[Darin Fisher]

Comment on attachment 117775 [details] [diff] [review]
sprinkle some checks around

sr=darin

Comment 8

[Doug Turner (:dougt)]

over IRC dwitte mentioned that there are other patterns in the patch that was
landed that should be fixed:

rdf/datasource/src/nsFileSystemDataSource.cpp
security/manager/ssl/src/nsPKCS12Blob.cpp
widget/src/xpwidgets/nsTransferable.cpp
extensions/cookie/nsPermissions.cpp

Comment 9

[Darin Fisher]

ok, i landed dwitte's patch, but dougt says to keep it open so the other code
spots can be similarly fixed up.

-> dougt for that part.

Comment 10

[[e-mail]]

ok, this is confusing. as mentioned above, the reason for the crashes isn't
really known; creating a new output stream should never really fail. so I was
suspecting some profile hackery (i.e. profile path is strange) or some strange
permissions on the users' cookie file or something...

but apparently not. Neil can reproduce the crash on win32 and told me what his
setup is - standard profile with cookies.txt in existence, no read-only
permission set on the cookiefile (just archive), crash occurs randomly and
immediately when starting to load a site.

2 talkback reports submitted by him; TB18283807Y and TB18286137X.

so this really should be investigated, imo. under these circumstances, we should
never encounter a situation where opening an output stream fails. -> dataloss

it couldn't be something stupid like not closing the stream when _Write()
finishes? it's a COMPtr so it should destruct & automatically flush & close when
the consumer goes away right?

Comment 11

[Darin Fisher]

dwitte: yeah, something really screwing is going on.

Comment 12

[Doug Turner (:dougt)]

Attachment: more clean up.

Comment 13

[Darin Fisher]

Comment on attachment 118016 [details] [diff] [review]
more clean up.

sr=darin

Comment 14

[Dave T]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4a) Gecko/20030321

Not sure if above build would include patch above, so here is some more info:

if site tries to set cookie then
  if cookies=enabled then
    OK
  else if cookies=enabled or cookies=originating_site then
    if ask_before_setting_cookies=enabled then
      if answer is accept or deny cookie creation then
        crash.

talkback ids when loading various sites: www.amx.com, www.crestron.com,
www.ford.com.au, www.macromedia.com
TB18343527E TB18343522G TB18343493Q TB18343424E TB18343422M 
TB18343215H TB18343202E TB18343151Z TB18343126G

Comment 15

[Jan Varga [:janv]]

TB18343527E:

Permission_Save
[c:/builds/seamonkey/mozilla/extensions/cookie/nsPermissions.cpp, line 423]
Permission_AddHost
[c:/builds/seamonkey/mozilla/extensions/cookie/nsPermissions.cpp, line 345]
Permission_Check
[c:/builds/seamonkey/mozilla/extensions/cookie/nsPermissions.cpp, line 256]
cookie_SetCookieString
[c:/builds/seamonkey/mozilla/extensions/cookie/nsCookies.cpp, line 1329]
COOKIE_SetCookieStringFromHttp
[c:/builds/seamonkey/mozilla/extensions/cookie/nsCookies.cpp, line 1694]
nsCookieService::SetCookieStringFromHttp
[c:/builds/seamonkey/mozilla/extensions/cookie/nsCookieService.cpp, line 247]
nsCookieHTTPNotify::OnExamineResponse
[c:/builds/seamonkey/mozilla/extensions/cookie/nsCookieHTTPNotify.cpp, line 266]
XPTC_InvokeByIndex
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 102]
nsProxyObject::Post
[c:/builds/seamonkey/mozilla/xpcom/proxy/src/nsProxyEvent.cpp, line 479]
nsProxyEventObject::CallMethod
[c:/builds/seamonkey/mozilla/xpcom/proxy/src/nsProxyEventObject.cpp, line 547]
PrepareAndDispatch
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp,
line 119]
SharedStub
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp,
line 147]
nsHttpHandler::OnExamineResponse
[c:/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHttpHandler.cpp, line 543]
nsHttpChannel::ProcessResponse
[c:/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp, line 621]
nsHttpChannel::OnStartRequest
[c:/builds/seamonkey/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp, line 2876]
nsInputStreamPump::OnStateStart
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsInputStreamPump.cpp, line 354]
nsInputStreamPump::OnInputStreamReady
[c:/builds/seamonkey/mozilla/netwerk/base/src/nsInputStreamPump.cpp, line 328] 

Comment 16

[Michiel van Leeuwen (email: mvl+moz@)]

So the last crash was in nsPermissions. That part just had a rewrite, so it
should be fixed in new builds. If it is not, please open a new bug. It is not
related to this then.

Comment 17

[Dave T]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4a) Gecko/2003032208

in this build. setting cookies after query allow/deny is now OK :)

Comment 18

[Darin Fisher]

-> me (since dougt is out for the week)

Comment 19

[Darin Fisher]

Comment on attachment 118016 [details] [diff] [review]
more clean up.

cc'ing kaie for review of the changes to
security/manager/ssl/src/nsPKCS12Blob.cpp

ignore changes to nsPermissions.cpp

thx!

Comment 20

[Kai Engert [:KaiE:]]

Comment on attachment 118016 [details] [diff] [review]
more clean up.

r=kaie on security changes

Comment 21

[Darin Fisher]

fixed-on-trunk