Last Comment Bug 710310 - SPDY: Google Docs Save Large File intermittent failure
: SPDY: Google Docs Save Large File intermittent failure
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 11 Branch
: x86_64 Linux
-- normal (vote)
: mozilla11
Assigned To: Patrick McManus [:mcmanus]
: Patrick McManus [:mcmanus]
: 711384 (view as bug list)
Depends on:
  Show dependency treegraph
Reported: 2011-12-13 11:37 PST by Patrick McManus [:mcmanus]
Modified: 2012-02-10 10:13 PST (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v0 (3.29 KB, patch)
2011-12-13 11:45 PST, Patrick McManus [:mcmanus]
no flags Details | Diff | Splinter Review
patch v1 (3.31 KB, patch)
2011-12-13 11:58 PST, Patrick McManus [:mcmanus]
honzab.moz: review+
honzab.moz: review+
Details | Diff | Splinter Review

Description User image Patrick McManus [:mcmanus] 2011-12-13 11:37:41 PST
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 User image Patrick McManus [:mcmanus] 2011-12-13 11:45:59 PST
Created attachment 581348 [details] [diff] [review]
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.

Comment 2 User image Patrick McManus [:mcmanus] 2011-12-13 11:58:08 PST
Created attachment 581355 [details] [diff] [review]
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 3 User image Patrick McManus [:mcmanus] 2011-12-16 15:54:09 PST
*** Bug 711384 has been marked as a duplicate of this bug. ***
Comment 4 User image Honza Bambas (:mayhemer) 2011-12-17 06:53:54 PST
Comment on attachment 581355 [details] [diff] [review]
patch v1

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


Sorry for the delay, let's get it in.
Comment 5 User image Patrick McManus [:mcmanus] 2011-12-17 17:32:13 PST
thanks honza
Comment 6 User image Matt Brubeck (:mbrubeck) 2011-12-18 15:34:13 PST
Comment 7 User image Honza Bambas (:mayhemer) 2012-02-10 10:13:48 PST
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) ?

Note You need to log in before you can comment on or make changes to this bug.