507571 - crash [@ PL_strncasecmp - LocateHttpStart - nsHttpTransaction::ParseHead] lock response headers

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[timeless]

Signature	PL_strncasecmp
UUID	45a65c78-35a5-4719-a1f2-946b52090725
Time 	2009-07-25 11:47:20.533418
Uptime	171
Last Crash	173 seconds before submission
Product	Firefox
Version	3.5.1
Build ID	20090715094852
Branch	1.9.1
OS	Windows NT
OS Version	5.1.2600 Service Pack 2
CPU	x86
CPU Info	GenuineIntel family 15 model 4 stepping 9
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x8
User Comments	
Processor Notes 	
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	plc4.dll 	PL_strncasecmp 	nsprpub/lib/libc/src/strcase.c:99
1 	xul.dll 	LocateHttpStart 	netwerk/protocol/http/src/nsHttpTransaction.cpp:93
2 	xul.dll 	nsHttpTransaction::ParseHead 	netwerk/protocol/http/src/nsHttpTransaction.cpp:774
3 	xul.dll 	nsHttpTransaction::ProcessData 	netwerk/protocol/http/src/nsHttpTransaction.cpp:1006
4 	xul.dll 	nsHttpTransaction::WritePipeSegment 	netwerk/protocol/http/src/nsHttpTransaction.cpp:511
5 	xul.dll 	nsPipeOutputStream::WriteSegments 	xpcom/io/nsPipe3.cpp:1137
6 	xul.dll 	nsHttpTransaction::WriteSegments 	netwerk/protocol/http/src/nsHttpTransaction.cpp:529
7 	xul.dll 	nsHttpConnection::OnSocketReadable 	netwerk/protocol/http/src/nsHttpConnection.cpp:664
8 	xul.dll 	nsHttpConnection::OnInputStreamReady 	netwerk/protocol/http/src/nsHttpConnection.cpp:762
9 	xul.dll 	nsSocketInputStream::OnSocketReady 	netwerk/base/src/nsSocketTransport2.cpp:256
10 	xul.dll 	nsSocketTransport::OnSocketReady 	netwerk/base/src/nsSocketTransport2.cpp:1519
11 	xul.dll 	nsSocketTransportService::DoPollIteration 	netwerk/base/src/nsSocketTransportService2.cpp:674
12 	nspr4.dll 	PR_GetThreadPrivate 	nsprpub/pr/src/threads/prtpd.c:232
13 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:497
14 	winmm.dll 	timeGetTime 	
15 	xul.dll 	nsSocketTransportService::Run 	netwerk/base/src/nsSocketTransportService2.cpp:575

Comment 1

[Wayne Mery (:wsmwk)]

rare, but still occurs as PL_strncasecmp | nsHttpTransaction::LocateHttpStart(char*, unsigned int, bool) 
bp-d25308ea-4382-45be-9066-1aaff2130106
0	plc4.dll	PL_strncasecmp	nsprpub/lib/libc/src/strcase.c:73
1	xul.dll	nsHttpTransaction::LocateHttpStart	netwerk/protocol/http/nsHttpTransaction.cpp:946
2	xul.dll	nsHttpTransaction::ParseHead	netwerk/protocol/http/nsHttpTransaction.cpp:1081
3	xul.dll	nsHttpTransaction::ProcessData	netwerk/protocol/http/nsHttpTransaction.cpp:1408
4	xul.dll	nsPipeOutputStream::WriteSegments	xpcom/io/nsPipe3.cpp:1106
5	xul.dll	nsHttpTransaction::WriteSegments	netwerk/protocol/http/nsHttpTransaction.cpp:616
6	xul.dll	nsHttpConnection::OnInputStreamReady	netwerk/protocol/http/nsHttpConnection.cpp:1508
7	xul.dll	nsSocketTransport::OnSocketReady	netwerk/base/src/nsSocketTransport2.cpp:1549

Comment 2

[Dragana Damjanovic [:dragana]]

Attachment: bug_507571_v1.patch

Comment 3

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c81c64db4766

Comment 4

[Dragana Damjanovic [:dragana]]

Comment on attachment 8752156 [details] [diff] [review]
bug_507571_v1.patch

need to fix a test :)

Comment 5

[Dragana Damjanovic [:dragana]]

Attachment: bug_507571_v1.patch

Comment 6

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ece17ac94aa5&selectedJob=20818077

Comment 7

[Dragana Damjanovic [:dragana]]

Comment on attachment 8752357 [details] [diff] [review]
bug_507571_v1.patch

I will do an update.

Comment 8

[Dragana Damjanovic [:dragana]]

Attachment: bug_507571_v1.patch

Comment 9

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dace70182c63

Comment 10

[Patrick McManus [:mcmanus]]

Comment on attachment 8755300 [details] [diff] [review]
bug_507571_v1.patch

Review of attachment 8755300 [details] [diff] [review]:
-----------------------------------------------------------------

I would change the _internal calls to be _locked - clearer and matches an existing idiom

this patch isn't quite a scary as it seemed at first (though it is big) and its obvious from reading it where things could be problematic without the locks

::: netwerk/protocol/http/nsHttpResponseHead.cpp
@@ +97,5 @@
> +void
> +nsHttpResponseHead::ContentType(nsACString &aContentType)
> +{
> +    ReentrantMonitorAutoEnter monitor(mReentrantMonitor);
> +    aContentType = mContentType; 

whitespace nit

@@ +104,5 @@
> +void
> +nsHttpResponseHead::ContentCharset(nsACString &aContentCharset)
> +{
> +    ReentrantMonitorAutoEnter monitor(mReentrantMonitor);
> +    aContentCharset = mContentCharset; 

whitespace nit

@@ +205,5 @@
> +    return mHeaders.HasHeader(h);
> +}
> +
> +void
> +nsHttpResponseHead::SetContentType(const nsACString &s)    

whitespace nit

@@ +212,5 @@
> +    mContentType = s;
> +}
> +
> +void
> +nsHttpResponseHead::SetContentCharset(const nsACString &s) 

whitespace nit

::: netwerk/protocol/http/nsHttpResponseHead.h
@@ +10,5 @@
>  #include "nsHttp.h"
>  #include "nsString.h"
> +#include "mozilla/ReentrantMonitor.h"
> +
> +class nsIHttpHeaderVisitor; 

whitespace nit

Comment 11

[Dragana Damjanovic [:dragana]]

Attachment: bug_507571_v1.patch

Comment 12

[Carsten Book [:Tomcat]]

failed to apply:

applying bug_507571_v1.patch
patching file netwerk/protocol/http/nsHttpConnection.cpp
Hunk #3 FAILED at 963
1 out of 4 hunks FAILED -- saving rejects to file netwerk/protocol/http/nsHttpConnection.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug_507571_v1.patch

Comment 13

[Dragana Damjanovic [:dragana]]

Attachment: bug_507571_v1.patch

Rebased.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/866b3a1aa9b5

Comment 15

[Dragana Damjanovic [:dragana]]

For dev-doc-needed: I have only change that setHeaders will return an error if called during VisitHeaders, VisitOriginalHeader and GetOriginalHeader.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/866b3a1aa9b5

Comment 17

[Sebastian Zartner [:sebo]]

Documented here:
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHttpChannel

Dragana, again, please review my changes and tell me whether they're ok.

Sebastian

Comment 18

[Dragana Damjanovic [:dragana]]

(In reply to Sebastian Zartner [:sebo] from comment #17)
> Documented here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIHttpChannel
> 
> Dragana, again, please review my changes and tell me whether they're ok.
> 
> Sebastian

Thanks.
Can you also extend warning which is part of VisitResponseHeader and VisitOriginalResponseHeader descriptions: 
"Warning: Calling setRequestHeader() while visiting request headers has undefined behavior. Don't do it!"
Add comment that the call of this function will return error NS_ERROR_FAILURE  starting with Gecko49.

Thanks.

Comment 19

[Sebastian Zartner [:sebo]]

(In reply to Dragana Damjanovic [:dragana] from comment #18)
> (In reply to Sebastian Zartner [:sebo] from comment #17)
> > Documented here:
> > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> > Interface/nsIHttpChannel
> > 
> > Dragana, again, please review my changes and tell me whether they're ok.
> > 
> > Sebastian
> 
> Thanks.
> Can you also extend warning which is part of VisitResponseHeader and
> VisitOriginalResponseHeader descriptions: 
> "Warning: Calling setRequestHeader() while visiting request headers has
> undefined behavior. Don't do it!"
> Add comment that the call of this function will return error
> NS_ERROR_FAILURE  starting with Gecko49.

Good point. Fixed that now.

Sebastian