Last Comment Bug 764186 - SpdySession3 closes its connection immediately when the first and only transaction is NullTransaction
: SpdySession3 closes its connection immediately when the first and only transa...
Status: RESOLVED FIXED
[spdy][http-conn]
: perf
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla16
Assigned To: Patrick McManus [:mcmanus]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-12 15:30 PDT by Honza Bambas (:mayhemer)
Modified: 2012-06-28 17:18 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
-
fixed
fixed


Attachments
log (1.94 MB, text/plain)
2012-06-12 15:30 PDT, Honza Bambas (:mayhemer)
no flags Details
patch 0 (3.79 KB, patch)
2012-06-19 11:50 PDT, Patrick McManus [:mcmanus]
honzab.moz: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Honza Bambas (:mayhemer) 2012-06-12 15:30:31 PDT
Created attachment 632445 [details]
log

Current m-c build.  Figured out through the event tracer.

rv = stream->ReadSegments(this, count, countRead); returns NS_BASE_STREAM_CLOSED that is considered as a fatal error.  That leads to call of CloseTransaction by nsHttpConnection and also DontReuse(); that throws the spdy session away because of mStreamTransactionHash.Count() == 0.

Search the log for "SpdySession3::ReadSegments fb4be18 returning FAIL code 80470002".

This will be fixed with bug 715905, but I suspect this could be an architectural bug in spdy implementation.  But I cannot say for sure.

Hard to say what all versions are affected with this bug since this doesn't seem to be limited to spdy3.
Comment 1 Patrick McManus [:mcmanus] 2012-06-12 17:52:48 PDT
(In reply to Honza Bambas (:mayhemer) from comment #0)

> Hard to say what all versions are affected with this bug since this doesn't
> seem to be limited to spdy3.

thanks honza. 15 is the only question.. nulltransactions aren't in 14.

I don't favor tracking - this is a fairly minor under the covers perf issue - no user visible impact. definitely worth fixing tho.
Comment 2 Lukas Blakk [:lsblakk] use ?needinfo 2012-06-18 17:23:51 PDT
I agree with Patrick, we don't need to track this one, nominate and make the case for uplift if you fix and 15 is known to be affected.
Comment 3 Patrick McManus [:mcmanus] 2012-06-19 11:50:33 PDT
Created attachment 634521 [details] [diff] [review]
patch 0

This is a good find. I was seeing around a dozen connections being tossed away due to this when getting setup with google plus on m-c and aurora.

I considered a number of ways to address this within the abstraction.. but most of them boiled down to variations on some change of NullTransaction::mDone/IsDone() like in https://bug760608.bugzilla.mozilla.org/attachment.cgi?id=629832 .. we avoided doing that last time around.

So I decided that it was better to just put a crack in the abstraction being used here and add nsAHttpTransaction::QueryNullTranascation().. so it can be dealt with explicitly in spdy rather than implicitly through a bunch of subtle state mechanations. It can be removed with 715905. Its not great, but its not forever and is quite straight forward.
Comment 4 Honza Bambas (:mayhemer) 2012-06-21 15:28:38 PDT
Comment on attachment 634521 [details] [diff] [review]
patch 0

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

::: netwerk/protocol/http/nsAHttpTransaction.h
@@ +108,5 @@
> +
> +    // equivalent to !!dynamic_cast<NullHttpTransaction *>(this)
> +    // A null transaction is expected to return BASE_STREAM_CLOSED on all of
> +    // its IO functions all the time.
> +    virtual bool QueryNullTransaction() { return false; }

Maybe just call this IsNullTransaction when the result is bool.
Comment 5 Patrick McManus [:mcmanus] 2012-06-21 18:16:42 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa6906f8e92

I plan to let it bake for a few days then nom for aurora.
Comment 6 Ed Morley [:emorley] 2012-06-22 03:43:39 PDT
https://hg.mozilla.org/mozilla-central/rev/aaa6906f8e92
Comment 7 Patrick McManus [:mcmanus] 2012-06-26 07:15:28 PDT
Comment on attachment 634521 [details] [diff] [review]
patch 0

[Approval Request Comment]
Bug caused by (feature/regressing bug #): feature 729133 (speculative connectinos)
User impact if declined: lag getting started with gmail/g+/twitter
Testing completed (on m-c, etc.):  on m-c for 5 days
Risk to taking this patch (and alternatives if risky): modest risk.. alternative would be to do nothing and live with the connection storm.
String or UUID changes made by this patch: none

beta is not impacted.
Comment 8 Alex Keybl [:akeybl] 2012-06-26 10:07:34 PDT
(In reply to Patrick McManus [:mcmanus] from comment #7)
> Risk to taking this patch (and alternatives if risky): modest risk..
> alternative would be to do nothing and live with the connection storm.
> String or UUID changes made by this patch: none

In bug 763352 you mentioned expecting to quickly hear about any negative feedback once landed. Does the same apply here?
Comment 9 Patrick McManus [:mcmanus] 2012-06-26 10:45:57 PDT
(In reply to Alex Keybl [:akeybl] from comment #8)
> (In reply to Patrick McManus [:mcmanus] from comment #7)
> > Risk to taking this patch (and alternatives if risky): modest risk..
> > alternative would be to do nothing and live with the connection storm.
> > String or UUID changes made by this patch: none
> 
> In bug 763352 you mentioned expecting to quickly hear about any negative
> feedback once landed. Does the same apply here?

Yes, certainly anything important. The path is definitely well exercised.
Comment 10 Alex Keybl [:akeybl] 2012-06-27 15:38:01 PDT
Comment on attachment 634521 [details] [diff] [review]
patch 0

Given that, approving for Aurora 15.
Comment 11 Patrick McManus [:mcmanus] 2012-06-28 17:18:01 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/df798374a729

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