Last Comment Bug 762505 - Twitter's favicon slow loading due to SPDY v3
: Twitter's favicon slow loading due to SPDY v3
Status: RESOLVED FIXED
[spdy]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: 16 Branch
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: mozilla16
Assigned To: Patrick McManus [:mcmanus]
:
: Patrick McManus [:mcmanus]
Mentors:
: 762943 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-07 08:20 PDT by Fanolian
Modified: 2012-06-21 04:06 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
affected
fixed


Attachments
patch 0 (1.55 KB, patch)
2012-06-07 10:34 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
Details | Diff | Splinter Review

Description Fanolian 2012-06-07 08:20:46 PDT
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.
Comment 1 Patrick McManus [:mcmanus] 2012-06-07 10:30:26 PDT
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 Patrick McManus [:mcmanus] 2012-06-07 10:34:52 PDT
Created attachment 631035 [details] [diff] [review]
patch 0
Comment 3 Matthias Versen [:Matti] 2012-06-08 09:09:50 PDT
*** Bug 762943 has been marked as a duplicate of this bug. ***
Comment 4 Honza Bambas (:mayhemer) 2012-06-15 05:22:58 PDT
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.
Comment 5 Patrick McManus [:mcmanus] 2012-06-18 06:42:16 PDT
(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 Patrick McManus [:mcmanus] 2012-06-18 08:18:50 PDT
Comment on attachment 631035 [details] [diff] [review]
patch 0

crud, this fails one of my hand tests.
Comment 7 Patrick McManus [:mcmanus] 2012-06-19 09:28:55 PDT
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)
Comment 8 Honza Bambas (:mayhemer) 2012-06-20 08:06:14 PDT
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/
Comment 9 Patrick McManus [:mcmanus] 2012-06-20 10:21:04 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/febe76e0a075
Comment 10 Patrick McManus [:mcmanus] 2012-06-20 10:24:07 PDT
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.
Comment 11 Ed Morley [:emorley] 2012-06-21 04:06:40 PDT
https://hg.mozilla.org/mozilla-central/rev/febe76e0a075

Note You need to log in before you can comment on or make changes to this bug.