Closed Bug 507571 Opened 10 years ago Closed 3 years ago

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

Categories

(Core :: Networking, defect, critical)

1.9.1 Branch
x86
Windows XP
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: timeless, Assigned: dragana)

References

(Blocks 1 open bug)

Details

(Keywords: crash, dev-doc-complete, Whiteboard: [rare][necko-active])

Crash Data

Attachments

(1 file, 4 obsolete files)

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
Crash Signature: [@ PL_strncasecmp - LocateHttpStart - nsHttpTransaction::ParseHead]
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
Crash Signature: [@ PL_strncasecmp - LocateHttpStart - nsHttpTransaction::ParseHead] → [@ PL_strncasecmp - LocateHttpStart - nsHttpTransaction::ParseHead] [@ PL_strncasecmp | nsHttpTransaction::LocateHttpStart(char*, unsigned int, bool) ]
Whiteboard: [rare]
Crash Signature: [@ PL_strncasecmp - LocateHttpStart - nsHttpTransaction::ParseHead] [@ PL_strncasecmp | nsHttpTransaction::LocateHttpStart(char*, unsigned int, bool) ] → [@ PL_strncasecmp - LocateHttpStart - nsHttpTransaction::ParseHead] [@ PL_strncasecmp | nsHttpTransaction::LocateHttpStart(char*, unsigned int, bool) ] [@ PL_strncasecmp | nsHttpTransaction::LocateHttpStart ]
Whiteboard: [rare] → [rare][necko-backlog]
Assignee: nobody → dd.mozilla
Summary: crash [@ PL_strncasecmp - LocateHttpStart - nsHttpTransaction::ParseHead] → crash [@ PL_strncasecmp - LocateHttpStart - nsHttpTransaction::ParseHead] lock response headers
Whiteboard: [rare][necko-backlog] → [rare][necko-active]
Attached patch bug_507571_v1.patch (obsolete) — Splinter Review
Attachment #8752156 - Flags: review?(mcmanus)
Comment on attachment 8752156 [details] [diff] [review]
bug_507571_v1.patch

need to fix a test :)
Attachment #8752156 - Flags: review?(mcmanus)
Attached patch bug_507571_v1.patch (obsolete) — Splinter Review
Attachment #8752156 - Attachment is obsolete: true
Attachment #8752357 - Flags: review?(mcmanus)
Comment on attachment 8752357 [details] [diff] [review]
bug_507571_v1.patch

I will do an update.
Attachment #8752357 - Flags: review?(mcmanus)
Attached patch bug_507571_v1.patch (obsolete) — Splinter Review
Attachment #8752357 - Attachment is obsolete: true
Attachment #8755300 - Flags: review?(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
Attachment #8755300 - Flags: review?(mcmanus) → review+
Attached patch bug_507571_v1.patch (obsolete) — Splinter Review
Attachment #8755300 - Attachment is obsolete: true
Attachment #8755559 - Flags: review+
Keywords: checkin-needed
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
Flags: needinfo?(dd.mozilla)
Keywords: checkin-needed
Rebased.
Attachment #8755559 - Attachment is obsolete: true
Flags: needinfo?(dd.mozilla)
Attachment #8755850 - Flags: review+
Keywords: checkin-needed
For dev-doc-needed: I have only change that setHeaders will return an error if called during VisitHeaders, VisitOriginalHeader and GetOriginalHeader.
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/866b3a1aa9b5
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
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
Flags: needinfo?(dd.mozilla)
(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.
Flags: needinfo?(dd.mozilla)
(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
You need to log in before you can comment on or make changes to this bug.