Closed Bug 1021396 Opened 5 years ago Closed 5 years ago

duplicate spdy syn_stream can hang necko thread

Categories

(Core :: Networking: HTTP, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox29 --- wontfix
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: mcmanus, Assigned: mcmanus)

Details

(Whiteboard: [spdy])

Attachments

(2 files)

I'm testing with spdyproxy (spdy/3) and have seen the necko thread hang a couple times. I have one log with a lot of my personal profile in it. I'm working on getting one with a fresh profile to share.

The root cause is that the session sees 2 syn_reply frames for the same stream id. This happens to be a CONNECT stream - I'm not sure if that's relevant.

the spdy spec says "If an endpoint receives multiple SYN_REPLY frames for the same active stream ID, it MUST issue a stream error (Section 2.4.2) with the error code STREAM_IN_USE."

since that's a stream error (which does not close the session) the headers need to be uncompressed. The code uncompresses it first and then checks to see if this is a double reply.

The bug is that the decompression code will get stuck in a loop if called to process stream headers twice, as it frees the buffer after the first time. (and it can't make progress with 0 bytes of buffer). So it never completes the uncompression and it doesn't get to check if this is a double open.

this applies to spdy/3 and spdy/3.1 but not http/2. It doesn't apply to http/2 for a few reasons - chiefly multiple headers are simply legal for http/2 so the same assumption isn't made and the decompressor can actually allocate buffer as needed in http/2.

I'm going to turn the spdy stream error into a session error. That's just a protocol violation.
actually, as much as I want to make it a session error - spdy seems to explicit say this is OK but has to return a stream error.
Attached file log.bz2
nspr log showing double syn_reply for stream 0x67 on same session

line 582147 syn_reply 0x67
4 data frames for 0x67.. about 3kb of data
line 607557 syn_reply 0x67
Whiteboard: [spdy]
Attachment #8435954 - Flags: review?(hurley) → review+
  https://hg.mozilla.org/integration/mozilla-inbound/rev/c86cd310bee7

will want to backport this after it gets to m-c
https://hg.mozilla.org/mozilla-central/rev/c86cd310bee7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment on attachment 8435954 [details] [diff] [review]
0001-bug-1021396-duplicate-spdy-syn_stream-can-hang-necko.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): original spdy support bug (ff13?)
User impact if declined: a server bug can lead to a complete networking hang in firefox.
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): live with the risk - requires a server bug to trigger which is not known to widely exist on the internet
String or IDL/UUID changes made by this patch: none

more info: If a server sends a double spdy syn_reply for the same stream it can result in the networking thread getting into an infinite loop. There is only one networking thread, so the result is that firefox networking stops and requires a session restart to fix.

This trigger is a server bug - so we don't see it from the big spdy properties on the Internet. I've seen the node.js based spdyproxy do this however (which is how I found this bug). I have only seen that in conjuction with spdy proxying (introduced in firefox-32 (now aurora)) but I cannot say with confidence whether or not the bug can only happen in that scenario - so its best imo to backport this to 31 as well.

http/2 is not impacted - just spdy/3 and spdy/3.1
Attachment #8435954 - Flags: approval-mozilla-beta?
Attachment #8435954 - Flags: approval-mozilla-aurora?
Comment on attachment 8435954 [details] [diff] [review]
0001-bug-1021396-duplicate-spdy-syn_stream-can-hang-necko.patch

Since we have this bug for a while, I won't take this change for beta.
Attachment #8435954 - Flags: approval-mozilla-beta?
Attachment #8435954 - Flags: approval-mozilla-beta-
Attachment #8435954 - Flags: approval-mozilla-aurora?
Attachment #8435954 - Flags: approval-mozilla-aurora+
Assignee: nobody → mcmanus
You need to log in before you can comment on or make changes to this bug.