Crash [@ nsHttpHeaderArray::PeekHeader] with proxied https only when network.http.pipelining.ssl is true

RESOLVED FIXED in mozilla1.9

Status

()

Core
Networking: HTTP
P2
critical
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Carey Evans, Assigned: mayhemer)

Tracking

({crash, topcrash})

Trunk
mozilla1.9
x86
Windows XP
crash, topcrash
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

10 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4) Gecko/2008030714 Firefox/3.0b4

After updating to Firefox 3.0b4 from Firefox 3.0b3, many moderately complicated https sites crash the browser, almost every time they're accessed. This occurs even in safe mode on a new profile. I accessed these web pages using a proxy with NTLM authentication.

Pages that crash the browser include https://addons.mozilla.org/, https://bugzilla.mozilla.org/ and https://www.microsoft.com/.

Changing the preference network.http.pipelining.ssl to false seems to have stopped the crashes.

Reproducible: Always

Steps to Reproduce:
Load a moderately complicated https page from behind an NTLM authenticated proxy, either following a link or typing in the URL.
Actual Results:  
The browser crashes before the page has finished loading.

Expected Results:  
The browser shouldn't crash.

It seems to help if some images or other resources on the page are cached. I haven't tested this with other kinds of proxies.

Bug #416175 looks suspicious, and reading it is what gave me the idea to turn off the SSL pipelining preference.

Crash report IDs include ea3a794f-ef16-11dc-881f-001a4bd43ed6, 351c2f28-ef16-11dc-8763-001a4bd43e5c and 63285c61-ef15-11dc-916d-001a4bd43e5c.  The crash is an EXCEPTION_ACCESS_VIOLATION at various addresses, and the stack trace always seems to be that below:

nsHttpHeaderArray::PeekHeader(nsHttpAtom)
nsHttpConnection::SetupSSLProxyConnect()
xul.dll@0x286c2e
nsHttpConnectionMgr::DispatchTransaction(nsHttpConnectionMgr::nsConnectionEntry*, nsAHttpTransaction*, unsigned char, nsHttpConnection*)
xul.dll@0x29a2d0
nsHttpConnectionMgr::OnMsgProcessPendingQ(int, void*)
nsHttpConnectionMgr::OnMsgReclaimConnection(int, void*)
nsHttpConnectionMgr::nsConnEvent::Run()
nsThread::ProcessNextEvent(int, int*)
PR_Lock
Component: General → Networking: HTTP
Product: Firefox → Core
QA Contact: general → networking.http
Version: unspecified → Trunk
Flags: blocking1.9?
Keywords: crash

Comment 1

10 years ago
This is topcrash #34 according to 
http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.0b4
Keywords: topcrash
Summary: Crash with proxied https only when network.http.pipelining.ssl is true → Crash [@ nsHttpHeaderArray::PeekHeader] with proxied https only when network.http.pipelining.ssl is true
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 2

10 years ago
Sorry, it looks like bug #416175 has nothing to do with the crash. All the nightlies since https pipelining was turned on in bug #414477 crash for me.

Updated

10 years ago
Blocks: 414477
This needs to be fixed or disabled. See also bug 422978.

Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Who owns this?  It's a P2 blocker.  Need an owner ASAP.

Comment 5

10 years ago
i won't have cycles to look at this for a while.
dwitte, if not you, then who would you recommend?

Comment 7

10 years ago
Michal, Honza, or DCamp?
Asked dave to take a look.
Re-assigning to dcamp.
Assignee: nobody → dcamp
(Assignee)

Comment 10

10 years ago
I am already investigating this, although I am unable to reproduce this.
Assignee: dcamp → honzab
(Assignee)

Comment 11

10 years ago
I spent some time trying to reproduce the crash (unsuccessfully). then I was analyzing this and have some theory, but not much certain:

nsHttpConnectionMgr::DispatchTransaction calls nsHttpConnection::Activate that calls SetupSSLProxyConnect only and only if !mCompletedSSLConnect (and of course using SSL+proxy)

The cause of the crash could be call of nsHttpConnection::Activate on connetion with mTransaction containing nsHttpPipeline instead of nsHttpTransaction and with flag mCompletedSSLConnect being false.

Does anyone believe I am on the right path? I have to spend some other time to figure out why this could happen.
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 12

10 years ago
Carey Evans: what kind of proxy are you exactly using and what is exactly your net config? I was testing with Squid (lnx) and CCProxy (win) both NTLM capable w/o success to reproduce.
(Reporter)

Comment 13

10 years ago
The proxy software is WebMarshal, running on Windows Server, chained to Squid on some version of Red Hat. I've only reproduced this from Windows XP for the client. We don't yet use Active Directory, so there is definitely no Kerberos involved, just NTLM authentication. (Using nsAuthSSPI.cpp?) I'm never prompted for the proxy username and password.
(Assignee)

Comment 14

10 years ago
(In reply to comment #13)
> The proxy software is WebMarshal, running on Windows Server, chained to Squid
> on some version of Red Hat. I've only reproduced this from Windows XP for the
> client. We don't yet use Active Directory, so there is definitely no Kerberos
> involved, just NTLM authentication. (Using nsAuthSSPI.cpp?) I'm never prompted
> for the proxy username and password.
> 

Thanks for this info, I installed web marshal trial on my local host, imported local NT users and I can reproduce the crash.
(Assignee)

Comment 15

10 years ago
Comment 11 is wrong, mConnInfo and mRequestHead in mTransaction (of correct nsHttpTransaction type) are deleted.
(Assignee)

Comment 16

10 years ago
(In reply to comment #15)
> Comment 11 is wrong, mConnInfo and mRequestHead in mTransaction (of correct
> nsHttpTransaction type) are deleted.
> 

Ops, overlook in my debugger! Comment 11 *IS* correct. Sorry.
(Assignee)

Comment 17

10 years ago
Created attachment 312095 [details]
nsHttp:5 crash log, grep for network thread only

This is log of connection to https://addons.mozilla.org through WebMarshal proxy configured to accept only local NT users (NTLM authentication).

Cause of the crash:
Crash is caused by static cast on nsHttpConnection::mTransaction in nsHttpConnection::SetupSSLProxyConnect where nsHttpTransaction object is expected (as seen at the comment).

Because proxy has several times answered with 407 and HTTP/1.1 connection switched to accept pipelining (nsHttpConnection::OnHeadersAvailable, call to SupportsPipelining).

When the same connection is reused after the proxy connection failure (to have chance to negotiate again (?)) there is already pending more then one transaction. Because the top level transaction allows pipelining and also the connection is capable of it then instead of nsHttpTransaction object of nsHttpPipeline is built and passed to the connection (nsHttpConnectionMgr::DispatchTransaction, if (conn->SupportsPipelining()..., call to conn->Activate => CRASH).
(Assignee)

Comment 18

10 years ago
Created attachment 312103 [details] [diff] [review]
Possible fix

As I understand, HTTP proxies that supports HTTP/1.1 are capable to provide (emulate) pipelining even the target server doesn't. But in case of using HTTPS tunneling (connection to an HTTPS server using an HTTP proxy) doesn't make sense to provide pipelining from the very start of proxy negotiation/connection. Only after we connect the real server through the SSL tunnel (untouched by the proxy server) we may decide if pipelining is supported by that server.

This patch fixes Firefox behavior and also fixes the crash. However, as I was writing this comment it seemed to me we should crash the same way also for normal (non SSL) proxy connections when pipelining is turned on. But testing shows I am not able to reproduce such crash so, we are probably ok.
Attachment #312103 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #312103 - Flags: review? → review?(bzbarsky)
I can't review this code: I don't really know it.  Please try Christian instead.

Updated

10 years ago
Attachment #312103 - Flags: review?(bzbarsky) → review?(cbiesinger)
Comment on attachment 312103 [details] [diff] [review]
Possible fix

+            // SSL tunnel though an HTTP proxy. Pipelining support

s/SSL/an SSL/?

+            // target server in this case. This is also fix for crash in 
+            // SetupSSLProxyConnect method where nsHttpTransaction
+            // is expected but nsHttpPipeline is present in mTransaction

so, this makes it sound like this is a workaround for a bug in SetupSSLProxyConnect. But that's not the case. The bug was just here.

I think you should just remove this sentence and the next one.
Attachment #312103 - Flags: review?(cbiesinger) → review+
(Assignee)

Comment 21

10 years ago
Created attachment 312127 [details] [diff] [review]
Fix2

changed just the comment as Christian suggested.

Personally I also don't like static casts in nsHttpConnection. First I wanted to fix that rather then do this magic with flags.
Attachment #312103 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
Checking in netwerk/protocol/http/src/nsHttpConnection.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpConnection.cpp,v  <--  nsHttpConnection.cpp
new revision: 1.63; previous revision: 1.62
done
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9
(Reporter)

Comment 23

10 years ago
I've confirmed that I can no longer reproduce this bug with the latest nightly build. Thanks Honza!
Crash Signature: [@ nsHttpHeaderArray::PeekHeader]
You need to log in before you can comment on or make changes to this bug.