Initialize HttpStreamRequest::alternate_protocol_ to avoid Valgrind false rep...

Initialize HttpStreamRequest::alternate_protocol_ to avoid Valgrind false reports
BUG=64918
TEST=linux_valgrind trybot

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=68456

[Timur Iskhodzhanov, 2010-12-06 16:40:22]

Hi Mike, Will,

( this patch should fix http://code.google.com/p/chromium/issues/detail?id=64918
)

The "bool want_spdy_over_npn = alternate_protocol_mode_ ==
kUsingAlternateProtocol && alternate_protocol_ ==
HttpAlternateProtocols::NPN_SPDY_2;"
is over-optimized by gcc -O1 so it actually reads both variables at once as a
64-bit value.
This is a correct optimization but it makes Valgrind bark on the
"alternate_protocol_" read since it isn't initialized.

There are basically three options to overcome this report:
1) disable -O1 for Valgrind (will slow down everything by 2x or something)
2) disable this particular optimization (I couldn't find the corresponding flag)
3) initialize the "alternate_protocol_".

I think the easiest way is (3), so I'd like to initialize it with an
impossible/incorrect value (BROKEN).

Can you please take a look at this change?

Thanks,
Timur Iskhodzhanov
Google Russia

[willchan no longer on Chromium, 2010-12-06 17:52:04]

http://codereview.chromium.org/5599004/diff/1/net/http/http_stream_request.cc
File net/http/http_stream_request.cc (right):

http://codereview.chromium.org/5599004/diff/1/net/http/http_stream_request.cc...
net/http/http_stream_request.cc:66:
alternate_protocol_(HttpAlternateProtocols::BROKEN),
I'd prefer you added a new enum value to represent the uninitialized state. 
INVALID or something.  Up to you.

[Mike Belshe, 2010-12-06 18:03:21]

LGTM - thanks for tracking this down!

[Timur Iskhodzhanov, 2010-12-06 18:39:41]

I don't see much difference between BROKEN and INVALID names :-) that said I
think this adds some ambiguity.

The comment in the header file says:
  // Only valid if |alternate_protocol_mode_| == kUsingAlternateProtocol.
  HttpAlternateProtocols::Protocol alternate_protocol_;

I think using "BROKEN" in case "alternamte_protocol_mode_ !=
kUsingAlternateProtocol" is fine
(after all, this value is undefined as of the HEAD revision).
Probably doesn't worth adding a new enum?

I'm not familiar with the code so I leave the decision to you.

On 2010/12/06 17:52:04, willchan wrote:
> http://codereview.chromium.org/5599004/diff/1/net/http/http_stream_request.cc
> File net/http/http_stream_request.cc (right):
> 
>
http://codereview.chromium.org/5599004/diff/1/net/http/http_stream_request.cc...
> net/http/http_stream_request.cc:66:
> alternate_protocol_(HttpAlternateProtocols::BROKEN),
> I'd prefer you added a new enum value to represent the uninitialized state. 
> INVALID or something.  Up to you.

[willchan no longer on Chromium, 2010-12-06 20:19:16]

On 2010/12/06 18:39:41, Timur Iskhodzhanov wrote:
> I don't see much difference between BROKEN and INVALID names :-) that said I
> think this adds some ambiguity.

BROKEN means we tried the protocol and it's known to be broken.  Initializing to
this state would be deceiving.  INVALID may not be the clearest name.  Maybe
UNINITIALIZED is better, if more verbose.  Let's do UNINITIALIZED.

> 
> The comment in the header file says:
>   // Only valid if |alternate_protocol_mode_| == kUsingAlternateProtocol.
>   HttpAlternateProtocols::Protocol alternate_protocol_;
> 
> I think using "BROKEN" in case "alternamte_protocol_mode_ !=
> kUsingAlternateProtocol" is fine
> (after all, this value is undefined as of the HEAD revision).
> Probably doesn't worth adding a new enum?
> 
> I'm not familiar with the code so I leave the decision to you.
> 
> On 2010/12/06 17:52:04, willchan wrote:
> >
http://codereview.chromium.org/5599004/diff/1/net/http/http_stream_request.cc
> > File net/http/http_stream_request.cc (right):
> > 
> >
>
http://codereview.chromium.org/5599004/diff/1/net/http/http_stream_request.cc...
> > net/http/http_stream_request.cc:66:
> > alternate_protocol_(HttpAlternateProtocols::BROKEN),
> > I'd prefer you added a new enum value to represent the uninitialized state. 
> > INVALID or something.  Up to you.

[Timur Iskhodzhanov, 2010-12-06 21:10:45]

UNINITIALIZED sounds much better to me.
Done.

On 2010/12/06 20:19:16, willchan wrote:
> On 2010/12/06 18:39:41, Timur Iskhodzhanov wrote:
> > I don't see much difference between BROKEN and INVALID names :-) that said I
> > think this adds some ambiguity.
> 
> BROKEN means we tried the protocol and it's known to be broken.  Initializing
to
> this state would be deceiving.  INVALID may not be the clearest name.  Maybe
> UNINITIALIZED is better, if more verbose.  Let's do UNINITIALIZED.
> 
> > 
> > The comment in the header file says:
> >   // Only valid if |alternate_protocol_mode_| == kUsingAlternateProtocol.
> >   HttpAlternateProtocols::Protocol alternate_protocol_;
> > 
> > I think using "BROKEN" in case "alternamte_protocol_mode_ !=
> > kUsingAlternateProtocol" is fine
> > (after all, this value is undefined as of the HEAD revision).
> > Probably doesn't worth adding a new enum?
> > 
> > I'm not familiar with the code so I leave the decision to you.
> > 
> > On 2010/12/06 17:52:04, willchan wrote:
> > >
> http://codereview.chromium.org/5599004/diff/1/net/http/http_stream_request.cc
> > > File net/http/http_stream_request.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/5599004/diff/1/net/http/http_stream_request.cc...
> > > net/http/http_stream_request.cc:66:
> > > alternate_protocol_(HttpAlternateProtocols::BROKEN),
> > > I'd prefer you added a new enum value to represent the uninitialized
state. 
> > > INVALID or something.  Up to you.

[willchan no longer on Chromium, 2010-12-06 21:16:42]

LGTM