Closed Bug 762505 Opened 12 years ago Closed 12 years ago

Twitter's favicon slow loading due to SPDY v3

Categories

(Core :: Networking: HTTP, defect)

16 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16
Tracking Status
firefox14 --- unaffected
firefox15 --- affected
firefox16 --- fixed

People

(Reporter: Fanolian+BMO, Assigned: mcmanus)

References

Details

(Whiteboard: [spdy])

Attachments

(1 file)

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 https://twitter.com or directly https://twitter.com/phoenix/favicon.ico.


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.
Assignee: nobody → mcmanus
Whiteboard: [spdy]
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)
Attached patch patch 0Splinter Review
Attachment #631035 - Flags: review?(honzab.moz)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.

Thanks.
(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 on attachment 631035 [details] [diff] [review]
patch 0

crud, this fails one of my hand tests.
Attachment #631035 - Flags: review?(honzab.moz)
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

s/ths/this/
Attachment #631035 - Flags: review?(honzab.moz) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/febe76e0a075
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: