Closed Bug 1213060 Opened 4 years ago Closed 4 years ago

Http2Session::OnWriteSegment does not allow PADDING

Categories

(Core :: Networking: HTTP, defect)

32 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: mcmanus, Assigned: u408661)

Details

(Whiteboard: [spdy])

Attachments

(1 file)

in bug 1208114 I put in an assert(false) at the end of Http2Session::OnWriteSegment() just before return ns_error_unexpected to see if any states were not being considered and that assert failed on https://www.youtube.com/watch?v=1visYpIREUM with mDownstreamState == DISCARDING_DATA_FRAME_PADDING

the code was relanded without the assert but we should be able to fix that up.

According to the log with the assert removed there is frequent padding used on google.com and youtube, but we don't seem to do anything bad based on the error_unexpected (like reset the stream).
This is likely the case when mDownstreamState == DISCARDING_DATA_FRAME, as well, though who knows if we ever hit that.

The fix is relatively simple, but I want to do a bit of investigation to see how we're getting into OnWriteSegment when the state is DISCARDING_DATA_FRAME_PADDING - so far as I can remember, that shouldn't ever happen. Best to make sure something really funky isn't going on that could bite us worse later on down the line.
OK, so I was wrong in comment 1 - this really does only happen in DISCARDING_DATA_FRAME_PADDING. Here's what happens - OnWriteSegment gets called, and we cross the boundary in the frame from real data into padding, so we change state to DISCARDING_DATA_FRAME_PADDING. The pipe further up the stack notices there's still a bit of data left, so it calls through the transaction into OnWriteSegment again without popping any higher back up the stack (such as into the session's WriteSegments). So this is a totally legit case, and we'll just handle it the way we do other state changes that may come in OnWriteSegment, namely by returning WOULD_BLOCK and letting the next iteration handle things.

I've also added the assertion back in for good measure.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a24412934bfe (ignore the massive amounts of infra failures on that push)
Attachment #8678231 - Flags: review?(mcmanus)
Comment on attachment 8678231 [details] [diff] [review]
Properly handle discarding padding in Http2Session::OnWriteSegment

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

r+, but can you push this in two parts (with the assert separate) so that if the assert finds something new and unrelated (the point of the assert afterall), we can disable it with r=backout
Attachment #8678231 - Flags: review?(mcmanus) → review+
https://hg.mozilla.org/mozilla-central/rev/66b3e8673774
https://hg.mozilla.org/mozilla-central/rev/c5279ea8d3c3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.