Fix race in HttpNetworkTransactionSpdy2Test.BasicAuthSpdyProxy

Fix race in HttpNetworkTransactionSpdy2Test.BasicAuthSpdyProxy and
re-enable it on TSAN/Dr. Memory.  Apply same fix to the corresponding
Spdy 3 test.

Issue was that the tests relied on never reading MockReadData could
not be safely read not being read.  The memory bots slow things down
enough that it was being read, and causing a crash.  Fixed the bad
data, and corrected the ordering of events.

BUG=130874
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=145959

[mmenke, 2012-06-29 16:11:11]

The win_drmemory buildbot looks broken-ish, but was able to reproduce the
failure locally, and this CL does fix it.

The rules for OrderedSocketData step ordering seem to be as follows:

1)  Connect is the only thing that happens on step 0.
2)  Can only either read or write on a step, can't do both.
3)  Trying to do an async read that will end up waiting for a write that has not
yet occurred will also take its own step.

The step numbers in the code now match up with the actual step numbers when
running test.  Renumbering wasn't strictly necessary, but the final read has to
be at step 11.  Earlier, and would happen before some of the writes, since when
there are ordering conflicts, reads tend to jump ahead of writes.  Later, and
we'd end up getting two ERR_IO_PENDING events, the second one for an
OnReadComplete call, which isn't allowed.

In my opinion, we should have tests fail when the ordering doesn't exactly mess
up to prevent problems like this, but probably take a lot of effort to switch
over to that behavior.  Using QuitNow instead of Quit may also help, but a bunch
of tests currently fail if we do that.

[Ryan Hamilton, 2012-06-29 16:27:27]

Thanks for doing this, but I'm confused.  I suspect you have solved the problem
but I'm missing something.  You said:

  Renumbering wasn't strictly necessary, but the final read has to be at step
11.  

I'm not sure how to square the first half of that sentence with the second half?
 Why does the final read need to be at step 11?

(Man, I hate OrderedSocketData.  I'm tempted to propose that we switch
everything that uses it over to DeterministicSocketData...)

[mmenke, 2012-06-29 16:41:27]

On 2012/06/29 16:27:27, Ryan Hamilton wrote:
> Thanks for doing this, but I'm confused.  I suspect you have solved the
problem
> but I'm missing something.  You said:
> 
>   Renumbering wasn't strictly necessary, but the final read has to be at step
> 11.  
> 
> I'm not sure how to square the first half of that sentence with the second
half?
>  Why does the final read need to be at step 11?
> 
> (Man, I hate OrderedSocketData.  I'm tempted to propose that we switch
> everything that uses it over to DeterministicSocketData...)

With the old code, the final reads would occur before the final write (Which is
just weird), so if it was before step 11, we'd get the read 0 event before the
write as well, and then either treat it as an EOF or try and read again.  If it
was the latter, we'd run off the end of our reads, in the former case, the last
write would fail.

If it were after step 11, then we'd return an IO_PENDING event because of the
write in front of it.  Then the write would post a task to actually start
reading.  Since we weren't in a far enough along step to start reading yet, we'd
post another IO_PENDING event.  Since the test code expects the read result
after an IO_PENDING not to be another IO_PENDING, the test would then fail.

Fixing the order of the other events as well probably eliminates the necessity
of making it exactly the event at step 11.

[mmenke, 2012-07-10 18:57:27]

Ping?  So how do you want to proceed with this?

On 2012/06/29 16:41:27, Matt Menke wrote:
> On 2012/06/29 16:27:27, Ryan Hamilton wrote:
> > Thanks for doing this, but I'm confused.  I suspect you have solved the
> problem
> > but I'm missing something.  You said:
> > 
> >   Renumbering wasn't strictly necessary, but the final read has to be at
step
> > 11.  
> > 
> > I'm not sure how to square the first half of that sentence with the second
> half?
> >  Why does the final read need to be at step 11?
> > 
> > (Man, I hate OrderedSocketData.  I'm tempted to propose that we switch
> > everything that uses it over to DeterministicSocketData...)
> 
> With the old code, the final reads would occur before the final write (Which
is
> just weird), so if it was before step 11, we'd get the read 0 event before the
> write as well, and then either treat it as an EOF or try and read again.  If
it
> was the latter, we'd run off the end of our reads, in the former case, the
last
> write would fail.
> 
> If it were after step 11, then we'd return an IO_PENDING event because of the
> write in front of it.  Then the write would post a task to actually start
> reading.  Since we weren't in a far enough along step to start reading yet,
we'd
> post another IO_PENDING event.  Since the test code expects the read result
> after an IO_PENDING not to be another IO_PENDING, the test would then fail.
> 
> Fixing the order of the other events as well probably eliminates the necessity
> of making it exactly the event at step 11.

[mmenke, 2012-07-10 18:58:43]

rch:  Sorry, forgot to clearly address my ping...  That ping was for you.  :)

On 2012/07/10 18:57:27, Matt Menke wrote:
> Ping?  So how do you want to proceed with this?
> 
> On 2012/06/29 16:41:27, Matt Menke wrote:
> > On 2012/06/29 16:27:27, Ryan Hamilton wrote:
> > > Thanks for doing this, but I'm confused.  I suspect you have solved the
> > problem
> > > but I'm missing something.  You said:
> > > 
> > >   Renumbering wasn't strictly necessary, but the final read has to be at
> step
> > > 11.  
> > > 
> > > I'm not sure how to square the first half of that sentence with the second
> > half?
> > >  Why does the final read need to be at step 11?
> > > 
> > > (Man, I hate OrderedSocketData.  I'm tempted to propose that we switch
> > > everything that uses it over to DeterministicSocketData...)
> > 
> > With the old code, the final reads would occur before the final write (Which
> is
> > just weird), so if it was before step 11, we'd get the read 0 event before
the
> > write as well, and then either treat it as an EOF or try and read again.  If
> it
> > was the latter, we'd run off the end of our reads, in the former case, the
> last
> > write would fail.
> > 
> > If it were after step 11, then we'd return an IO_PENDING event because of
the
> > write in front of it.  Then the write would post a task to actually start
> > reading.  Since we weren't in a far enough along step to start reading yet,
> we'd
> > post another IO_PENDING event.  Since the test code expects the read result
> > after an IO_PENDING not to be another IO_PENDING, the test would then fail.
> > 
> > Fixing the order of the other events as well probably eliminates the
necessity
> > of making it exactly the event at step 11.

[Ryan Hamilton, 2012-07-10 19:17:20]

On 2012/07/10 18:58:43, Matt Menke wrote:
> rch:  Sorry, forgot to clearly address my ping...  That ping was for you.  :)

Sorry for the delay.  LGTM.  At some point, I think we're going to have to
declare "OrderedSocketData Considered Harmful", but that's for another time :>

[mmenke, 2012-07-10 19:23:57]

On 2012/07/10 19:17:20, Ryan Hamilton wrote:
> On 2012/07/10 18:58:43, Matt Menke wrote:
> > rch:  Sorry, forgot to clearly address my ping...  That ping was for you. 
:)
> 
> Sorry for the delay.  LGTM.  At some point, I think we're going to have to
> declare "OrderedSocketData Considered Harmful", but that's for another time :>

Either that or make it strongly enforce ordering, and smarter.  I was kinda
shocked how badly out of order events were happening.

[commit-bot: I haz the power, 2012-07-10 19:24:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mmenke@chromium.org/10704042/4002

[Ryan Hamilton, 2012-07-10 19:46:14]

On 2012/07/10 19:23:57, Matt Menke wrote:
> On 2012/07/10 19:17:20, Ryan Hamilton wrote:
> > On 2012/07/10 18:58:43, Matt Menke wrote:
> > > rch:  Sorry, forgot to clearly address my ping...  That ping was for you. 
> :)
> > 
> > Sorry for the delay.  LGTM.  At some point, I think we're going to have to
> > declare "OrderedSocketData Considered Harmful", but that's for another time
:>
> 
> Either that or make it strongly enforce ordering, and smarter.  I was kinda
> shocked how badly out of order events were happening.

Right, DeterministicSocketData does precisely that.  I pretty much think we
should use it instead of OrderedSocketData.  Bah!  SO much time, so many
warts...

[commit-bot: I haz the power, 2012-07-10 20:52:16]

Try job failure for 10704042-4002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...