Twitter's favicon slow loading due to SPDY v3

RESOLVED FIXED in Firefox 16



Networking: HTTP
5 years ago
5 years ago


(Reporter: Fanolian, Assigned: mcmanus)


16 Branch
Windows 7

Firefox Tracking Flags

(firefox14 unaffected, firefox15 affected, firefox16 fixed)


(Whiteboard: [spdy])


(1 attachment)



5 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1
Build ID: 20120607025755

Steps to reproduce:

1. Make sure network.http.spdy.enabled.v3 is set to true. Restart the browser.
2. Load or directly

Actual results:

The favicon takes a long time to load, but it appears on the tab eventually after 20 seconds to 2 minutes.
If network.http.spdy.enabled.v3 is set to false, the favicon always loads instantly.

Twitter changes its logo today. I do not know if this is related.


5 years ago
Assignee: nobody → mcmanus
Whiteboard: [spdy]

Comment 1

5 years ago
this is my bug in v3 - thanks for the report.

That url actually returned a 302 redirect with a 0 content length and a fin on the syn-reply (i.e. response header) frame. Response header handling was changed in v2 -> v3 and for this case I failed to tell the http transaction that it was complete when this happened (thus the hang, because the redirect was incomplete)

Comment 2

5 years ago
Created attachment 631035 [details] [diff] [review]
patch 0
Attachment #631035 - Flags: review?(honzab.moz)


5 years ago
status-firefox14: --- → unaffected
status-firefox15: --- → affected
status-firefox16: --- → affected
Ever confirmed: true
Duplicate of this bug: 762943
Patrick, can you please add (a bug comment may be sufficient) a more detailed explanation of the changes?  I don't much follow the added comments.


Comment 5

5 years ago
(In reply to Honza Bambas (:mayhemer) from comment #4)
> Patrick, can you please add (a bug comment may be sufficient) a more
> detailed explanation of the changes?  I don't much follow the added comments.

The code on m-c concerns the PROCESSING_COMPLETE_HEADERS state. Normally this state is pushed onto the stack from the PROCESSING_DATA_FRAME state when the first data frame for a stream arrives (because at that point we know there will be no more headers). That is signaled by the mDataPending flag - if that's set we pop DATA_FRAME back onto the stack after all the response headers have been given to the stream.

The bug was that in the case we hadn't come to COMPLETE_HEADERS out of DATA_FRAME.. in that case we did ResetDownstreamState() unconditionally (to prepare for a new packet)... specifically when we are processing the headers because of a SYN_REPLY with a FIN flag on it. (where we also know there will be no more headers of course!). This is what is happening with twitter's favicon - it is a 0 byte 3xx redirection response.

The fix leaves us in the COMPLETE_HEADERS state so this same OnWriteSegment() will be called again.. the next time through the bit about 10 lines above that checks for the fin bit and all the headers having been written out will return BASE_STREAM_CLOSED to tell the stream it is complete. We can't do that synchronously where the fix is applied because it needs to return >0 in countWritten on that execution.

Comment 6

5 years ago
Comment on attachment 631035 [details] [diff] [review]
patch 0

crud, this fails one of my hand tests.
Attachment #631035 - Flags: review?(honzab.moz)

Comment 7

5 years ago
Comment on attachment 631035 [details] [diff] [review]
patch 0

I believe this patch is fine and review should continue.

The issue I was seeing that made me drop the r? flag turns out to be unrelated to spdy.. (bug 766159)
Attachment #631035 - Flags: review?(honzab.moz)
Comment on attachment 631035 [details] [diff] [review]
patch 0

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

This r+ is based mostly on trust the patch is correct.

Please write an automated test for this.

::: netwerk/protocol/http/SpdySession3.cpp
@@ +2093,5 @@
>          mDataPending = false;
>          ChangeDownstreamState(PROCESSING_DATA_FRAME);
>        }
> +      else if (!mInputFrameDataLast) {
> +        // If more frames are expected in ths stream, then reset the state so they can be

Attachment #631035 - Flags: review?(honzab.moz) → review+

Comment 9

5 years ago
status-firefox16: affected → fixed
Target Milestone: --- → mozilla16

Comment 10

5 years ago
I'm not planning to nom this for aurora, but if anyone reading this feels differently please weigh in.

The broken v3 code is indeed on FF15 (aurora), but v3 is preffed off there so it has no impact for normal users. It basically exists on that branch for early adopters to play with, and now they can move to FF16.
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.