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

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P2)

Description

[Carey Evans]

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

Comment 1

[Jesse Ruderman]

This is topcrash #34 according to 
http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.0b4

Comment 2

[Carey Evans]

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.

Comment 3

[(not currently active) Ted Mielczarek]

This needs to be fixed or disabled. See also bug 422978.

Comment 4

[Damon Sicore (:damons)]

Who owns this?  It's a P2 blocker.  Need an owner ASAP.

Comment 5

[[e-mail]]

i won't have cycles to look at this for a while.

Comment 6

[Damon Sicore (:damons)]

dwitte, if not you, then who would you recommend?

Comment 7

[Mike Schroepfer]

Michal, Honza, or DCamp?

Comment 8

[Damon Sicore (:damons)]

Asked dave to take a look.

Comment 9

[Damon Sicore (:damons)]

Re-assigning to dcamp.

Comment 10

[Honza Bambas (:mayhemer)]

I am already investigating this, although I am unable to reproduce this.

Comment 11

[Honza Bambas (:mayhemer)]

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.

Comment 12

[Honza Bambas (:mayhemer)]

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.

Comment 13

[Carey Evans]

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.

Comment 14

[Honza Bambas (:mayhemer)]

(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.

Comment 15

[Honza Bambas (:mayhemer)]

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

Comment 16

[Honza Bambas (:mayhemer)]

(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.

Comment 17

[Honza Bambas (:mayhemer)]

Attachment: 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).

Comment 18

[Honza Bambas (:mayhemer)]

Attachment: 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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I can't review this code: I don't really know it.  Please try Christian instead.

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

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.

Comment 21

[Honza Bambas (:mayhemer)]

Attachment: 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.

Comment 22

[Reed Loden [:reed]]

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

Comment 23

[Carey Evans]

I've confirmed that I can no longer reproduce this bug with the latest nightly build. Thanks Honza!