SPDY: Google Docs Save Large File intermittent failure

RESOLVED FIXED in mozilla11

Status

()

Core
Networking: HTTP
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

11 Branch
mozilla11
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
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.

Thanks
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #581348 - Flags: review?(honzab.moz)
(Assignee)

Comment 2

5 years ago
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.
Attachment #581348 - Attachment is obsolete: true
Attachment #581348 - Flags: review?(honzab.moz)
Attachment #581355 - Flags: review?(honzab.moz)
(Assignee)

Updated

5 years ago
Duplicate of this bug: 711384
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.
Attachment #581355 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 5

5 years ago
thanks honza
https://hg.mozilla.org/integration/mozilla-inbound/rev/a28ebec62c3a
https://hg.mozilla.org/mozilla-central/rev/a28ebec62c3a
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
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) ?
Attachment #581355 - Flags: review+
You need to log in before you can comment on or make changes to this bug.