Last Comment Bug 746731 - Uninitialised value use in nsHttpConnection::CanReuse
: Uninitialised value use in nsHttpConnection::CanReuse
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla15
Assigned To: Patrick McManus [:mcmanus]
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-18 14:13 PDT by Julian Seward [:jseward]
Modified: 2012-04-25 18:26 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed


Attachments
patch 0 (1.78 KB, patch)
2012-04-23 09:19 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Julian Seward [:jseward] 2012-04-18 14:13:17 PDT
/tests/toolkit/components/passwordmgr/test/test_bug_627616.html


(DISPLAY=:3.0 make -C ff-opt mochitest-plain TEST_PATH=toolkit/components/passwordmgr/test/test_bug_627616.html EXTRA_TEST_ARGS="--close-when-done --debugger=/home/sewardj/VgTRUNK/tchain/Inst/bin/valgrind --debugger-args='--smc-check=all-non-file --suppressions=/home/sewardj/MOZ/SUPPS/mochitest-mc.supp --error-limit=no --stats=no --trace-children=yes --child-silent-after-fork=yes "--trace-children-skip=/usr/bin/hg,/bin/rm,*/bin/certutil,*/bin/pk12util,*/bin/ssltunnel,*/bin/uname,*/bin/which,*/bin/ps,*/bin/grep" --tool=memcheck --track-origins=yes'") 2>&1 | tee spew-mc-627616-1


Conditional jump or move depends on uninitialised value(s)
   at 0x6315097: nsHttpConnection::CanReuse() (nsHttpConnection.cpp:571)
   by 0x631B839: nsHttpConnectionMgr::OnMsgReclaimConnection(int, void*) (nsHttpConnectionMgr.cpp:1895)
   by 0x6315DB7: nsHttpConnectionMgr::nsConnEvent::Run() (nsHttpConnectionMgr.h:518)
   by 0x6FF0E39: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:656)
   by 0x6FB95A9: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:245)
   by 0x62C303B: nsSocketTransportService::Run() (nsSocketTransportService2.cpp:652)
   by 0x6FF0E39: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:656)
   by 0x6FB95A9: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:245)
   by 0x6FF0B64: nsThread::ThreadFunc(void*) (nsThread.cpp:289)
   by 0x41A3EB2: _pt_root (ptthread.c:187)
   by 0x4A289C9: start_thread (pthread_create.c:300)
   by 0x53B470C: clone (clone.S:112)
 Uninitialised value was created by a heap allocation
   at 0x4029B9A: malloc (vg_replace_malloc.c:263)
   by 0x403CF6B: moz_xmalloc (mozalloc.cpp:87)
   by 0x631BB14: nsHttpConnectionMgr::nsHalfOpenSocket::OnOutputStreamReady(nsIAsyncOutputStream*) (mozalloc.h:229)
   by 0x62BE644: nsSocketOutputStream::OnSocketReady(unsigned int) (nsSocketTransport2.cpp:525)
   by 0x62C062C: nsSocketTransport::OnSocketReady(PRFileDesc*, short) (nsSocketTransport2.cpp:1568)
   by 0x62C2DB2: nsSocketTransportService::DoPollIteration(bool) (nsSocketTransportService2.cpp:762)
   by 0x62C3027: nsSocketTransportService::Run() (nsSocketTransportService2.cpp:645)
   by 0x6FF0E39: nsThread::ProcessNextEvent(bool, bool*) (nsThread.cpp:656)
   by 0x6FB95A9: NS_ProcessNextEvent_P(nsIThread*, bool) (nsThreadUtils.cpp:245)
   by 0x6FF0B64: nsThread::ThreadFunc(void*) (nsThread.cpp:289)
   by 0x41A3EB2: _pt_root (ptthread.c:187)
   by 0x4A289C9: start_thread (pthread_create.c:300)
Comment 1 Patrick McManus [:mcmanus] 2012-04-23 07:47:21 PDT
Thanks Julian, somehow we lost the initializer when making that PRInterval based.. it matters in this case where you have a transaction that did not finish parsing the headers.

I also noted a problem that the default value that is used when parsing the headers is always the spdy one instead of the http/1 version in this case.

my bug.
Comment 2 Patrick McManus [:mcmanus] 2012-04-23 09:19:57 PDT
Created attachment 617502 [details] [diff] [review]
patch 0

Initialize mIdleTimeout to 0. This is the way it was before the printerval change - which means that any connection that does not make it to onheadersvailable will not be reused unless the timeout is set explicitly such as by connection manager when it has leftover connections from syn-retry. I considered making it the default http KA value instead of 0, but that should be a separate patch if we do that.

Also fix up one case where the default http/1 KA value should have been used but the spdy value was used instead.
Comment 3 Patrick McManus [:mcmanus] 2012-04-23 09:51:04 PDT
Comment on attachment 617502 [details] [diff] [review]
patch 0

[Approval Request Comment]
Please see https://wiki.mozilla.org/Tree_Rules for information on mozilla-central landings that do not require approval.

Possible risk to Fennec Native 14 (if any):

This fixes a regression in HTTP timeout handling that was introduced on FF14.
Comment 4 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-24 11:20:40 PDT
Comment on attachment 617502 [details] [diff] [review]
patch 0

[Triage Comment]
Merge of 14 to Aurora has completed.  Please renominate for mozilla-aurora.
Comment 5 Patrick McManus [:mcmanus] 2012-04-24 12:17:01 PDT
This is FF15
https://hg.mozilla.org/integration/mozilla-inbound/rev/946995f07a3f
Comment 6 Patrick McManus [:mcmanus] 2012-04-24 12:18:20 PDT
Comment on attachment 617502 [details] [diff] [review]
patch 0

[Approval Request Comment]


This fixes a regression in HTTP timeout handling that was introduced on FF14.
Comment 8 Alex Keybl [:akeybl] 2012-04-25 12:41:27 PDT
Comment on attachment 617502 [details] [diff] [review]
patch 0

[Triage Comment]
Regression in 14, approved for Aurora 14.
Comment 9 Patrick McManus [:mcmanus] 2012-04-25 18:26:35 PDT
ff14
https://hg.mozilla.org/releases/mozilla-aurora/rev/00d9852452e5

Note You need to log in before you can comment on or make changes to this bug.