764186 - SpdySession3 closes its connection immediately when the first and only transaction is NullTransaction

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: 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]]

(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]

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]]

Attachment: 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)]

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]]

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]]

https://hg.mozilla.org/mozilla-central/rev/aaa6906f8e92

Comment 7

[Patrick McManus [:mcmanus]]

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]]

(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]]

(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]]

Comment on attachment 634521 [details] [diff] [review]
patch 0

Given that, approving for Aurora 15.

Comment 11

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df798374a729