Issue 3033012: Fixed memory leak in SpdyHttpStreamTest. Removed suppressions.

Issue 3033012: Fixed memory leak in SpdyHttpStreamTest. Removed suppressions. (Closed)

Created:
10 years, 5 months ago by erikchen

Modified:
9 years, 7 months ago

Reviewers:
Mike Belshe

CC:
chromium-reviews, cbentzel+watch_chromium.org, Timur Iskhodzhanov, Alexander Potapenko, stuartmorgan, pam+watch_chromium.org, Paweł Hajdan Jr., darin-cc_chromium.org

Visibility:
Public.

Description

Fixed memory leak in SpdyHttpStreamTest. Removed suppressions. Also cleaned up spdy unit tests a bit. TEST=SpdyHttpStreamTest should have no memleaks in valgrind. BUG=48865, 47950, 46886 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53200

Patch Set 1 #

Patch Set 2 : merge with trunk #

Total comments: 9

Patch Set 3 : nits. #

Patch Set 4 : merge with trunk #

Created: 10 years, 5 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+131 lines, -341 lines)			Patch
M	net/spdy/spdy_framer.h	View		2 chunks	+2 lines, -0 lines	0 comments	Download
M	net/spdy/spdy_http_stream_unittest.cc	View	1 2	2 chunks	+37 lines, -97 lines	0 comments	Download
M	net/spdy/spdy_network_transaction_unittest.cc	View	1	15 chunks	+29 lines, -85 lines	0 comments	Download
M	net/spdy/spdy_session_unittest.cc	View		6 chunks	+6 lines, -46 lines	0 comments	Download
M	net/spdy/spdy_stream_unittest.cc	View		3 chunks	+2 lines, -56 lines	0 comments	Download
M	net/spdy/spdy_test_util.h	View	1 2	2 chunks	+55 lines, -0 lines	0 comments	Download
M	tools/valgrind/memcheck/suppressions.txt	View	1 2 3	3 chunks	+0 lines, -57 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

Mike Belshe

lgtm http://codereview.chromium.org/3033012/diff/2001/3002 File net/spdy/spdy_http_stream_unittest.cc (right): http://codereview.chromium.org/3033012/diff/2001/3002#newcode25 net/spdy/spdy_http_stream_unittest.cc:25: // Needs fixing, see http://crbug.com/28622 nit: can you ...

10 years, 5 months ago (2010-07-20 21:23:19 UTC) #2

erikchen

10 years, 5 months ago (2010-07-20 21:46:48 UTC) #3

http://codereview.chromium.org/3033012/diff/2001/3002
File net/spdy/spdy_http_stream_unittest.cc (right):

http://codereview.chromium.org/3033012/diff/2001/3002#newcode25
net/spdy/spdy_http_stream_unittest.cc:25: // Needs fixing, see
http://crbug.com/28622
On 2010/07/20 21:23:19, Mike Belshe wrote:
> nit:  can you remove the comment in line 25?

Done.

http://codereview.chromium.org/3033012/diff/2001/3002#newcode31
net/spdy/spdy_http_stream_unittest.cc:31:
session_deps.host_resolver->set_synchronous_mode(true);
On 2010/07/20 21:23:19, Mike Belshe wrote:
> line 31: redundant?

Done.

http://codereview.chromium.org/3033012/diff/2001/3002#newcode52
net/spdy/spdy_http_stream_unittest.cc:52: scoped_refptr<TCPSocketParams>
tcp_params =
On 2010/07/20 21:23:19, Mike Belshe wrote:
> Why did you need to manually connect?
This is the source of the leak. http_stream->SendRequest queues up a frame to be
sent. It expects session to be CONNECTED. If state is IDLE, frame remains queued
until session connects, which is never.

http://codereview.chromium.org/3033012/diff/2001/3006
File net/spdy/spdy_test_util.h (right):

http://codereview.chromium.org/3033012/diff/2001/3006#newcode215
net/spdy/spdy_test_util.h:215: // Note: The CancelledTransaction test does
cleanup by running all
On 2010/07/20 21:23:19, Mike Belshe wrote:
> style: spacing on these comments needs 2 more chars

Done.

Expand Messages | Collapse Messages