Do not drop data on the floor in SpdySession::DoReadLoop.

Do not drop data on the floor in SpdySession::DoReadLoop.

Patch Set 1 adds regression test, watch it specatularily fail in trybot logs.
Patch Set 2 fixes the issue.

BUG=531570
Committed: https://crrev.com/125cde33c45c37472639c94e4af73760042bdcb0
Cr-Commit-Position: refs/heads/master@{#348726}

[Bence, 2015-09-14 17:25:12]

bnc@chromium.org changed reviewers:
+ mef@chromium.org

[Bence, 2015-09-14 17:25:12]

Misha: PTAL.  Thanks.

[mef, 2015-09-14 19:08:01]

On 2015/09/14 17:25:12, Bence wrote:
> Misha: PTAL.  Thanks.

The fix looks good, and makes sense, but I don't quite understand how does test
provide 'slow' read. 
It seems to fail on one trybot without the fix, but how does it guarantee
consistenc

[Bence, 2015-09-14 19:27:23]

On 2015/09/14 19:08:01, mef wrote:
> On 2015/09/14 17:25:12, Bence wrote:
> > Misha: PTAL.  Thanks.
> 
> The fix looks good, and makes sense, but I don't quite understand how does
test
> provide 'slow' read. 
> It seems to fail on one trybot without the fix, but how does it guarantee
> consistenc

SlowReads() is a mock time function that I introduced in
https://crrev.com/1214833006.  It ensures that enough time seems to pass between
subsequent calls of |time_func_| in SpdySession::DoReadLoop() to trigger
"yielding" in lines 1401--1409.  |session_deps_.time_func = SlowRead| is the
major difference between the new test TestYieldingSlowSynchronousReads and the
pre-existing test ReadDataWithoutYielding that I used as a template.

My impression is that eight bots see the test fail on three different platforms:
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulato...
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_...
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...

[mef, 2015-09-14 19:36:54]

lgtm

On 2015/09/14 19:27:23, Bence wrote:
> On 2015/09/14 19:08:01, mef wrote:
> > On 2015/09/14 17:25:12, Bence wrote:
> > > Misha: PTAL.  Thanks.
> > 
> > The fix looks good, and makes sense, but I don't quite understand how does
> test
> > provide 'slow' read. 
> > It seems to fail on one trybot without the fix, but how does it guarantee
> > consistenc
> 
> SlowReads() is a mock time function that I introduced in
> https://crrev.com/1214833006.  It ensures that enough time seems to pass
between
> subsequent calls of |time_func_| in SpdySession::DoReadLoop() to trigger
> "yielding" in lines 1401--1409.  |session_deps_.time_func = SlowRead| is the
> major difference between the new test TestYieldingSlowSynchronousReads and the
> pre-existing test ReadDataWithoutYielding that I used as a template.
Perfect, thanks for the explanation!


> My impression is that eight bots see the test fail on three different
platforms:
>
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel...
>
https://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64...
>
https://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulato...
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_...
>
https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
Hrm, for some reason when I clicked on them before I saw seemingly unrelated
compile issues.

[Bence, 2015-09-14 20:21:45]

The CQ bit was checked by bnc@chromium.org

[commit-bot: I haz the power, 2015-09-14 20:22:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1345453005/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1345453005/20001

[commit-bot: I haz the power, 2015-09-14 21:39:43]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-09-14 21:40:36]

Patchset 2 (id:??) landed as
https://crrev.com/125cde33c45c37472639c94e4af73760042bdcb0
Cr-Commit-Position: refs/heads/master@{#348726}

[commit-bot: I haz the power, 2015-09-23 12:37:32]

Patchset 2 (id:??) landed as
https://crrev.com/125cde33c45c37472639c94e4af73760042bdcb0
Cr-Commit-Position: refs/heads/master@{#348726}