710310 - SPDY: Google Docs Save Large File intermittent failure

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

STR:: 
1 enable spdy
2 goto google docs
3 make a new word processor doc
4 paste in > 40KB of text (all at once, this is impt)
5 - save or let it auto save

at least 1 in 3 times the save will fail.

Two issues identified:
 * counting uploaded bytes when frames bodies and headers are coalesced
 * refusing to consume data read from request stream in the hopes of creating a full chunk creates send loop

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch v0

This impacts only code in spdystream.cpp and fixes a problem testers are likely to hit so I'd like to get it into the tree asap under the existing spdy.enabled=false review policy.

Thanks

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: patch v1

Add a null check.. not actually needed the way the code flows right now, but the interface allows that attribute to be null.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 581355 [details] [diff] [review]
patch v1

Review of attachment 581355 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab

Sorry for the delay, let's get it in.

Comment 5

[Patrick McManus [:mcmanus]]

thanks honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/a28ebec62c3a

Comment 6

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/a28ebec62c3a

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 581355 [details] [diff] [review]
patch v1

Review of attachment 581355 [details] [diff] [review]:
-----------------------------------------------------------------

r=honzab for spdy.enabled=true

::: netwerk/protocol/http/SpdyStream.cpp
@@ +486,5 @@
>      LOG3(("Coalesce Transmit"));
>      memcpy (mTxInlineFrame + mTxInlineFrameSize,
>              buf, mTxStreamFrameSize);
> +    if (countUsed)
> +      *countUsed += mTxStreamFrameSize;

Interesting you are checking on 'countUsed'.  According assertion above and the fact you are using 'buf' in memcpy, it should never be null here.  

I also don't see a check on 'buf' being non-null to perform this memcpy at all.

But as I understand the code, buf is non-null when mTxStreamFrameSize is > 0, right?  It's so hard to track...

So, probably no need for the check and just add a comment explaining why not needed?

And also maybe assertion about (!buf ^ mTxStreamFrameSize) ?