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

RESOLVED FIXED in Firefox 15

Status

()

Core
Networking: HTTP
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mayhemer, Assigned: mcmanus)

Tracking

({perf})

Trunk
mozilla16
Points:
---

Firefox Tracking Flags

(firefox14 unaffected, firefox15- fixed, firefox16 fixed)

Details

(Whiteboard: [spdy][http-conn])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
(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.
Whiteboard: [spdy]
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.
tracking-firefox15: ? → -
(Assignee)

Updated

5 years ago
status-firefox14: --- → unaffected
status-firefox15: --- → affected
(Assignee)

Comment 3

5 years ago
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.
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #634521 - Flags: review?(honzab.moz)
(Reporter)

Updated

5 years ago
Whiteboard: [spdy] → [spdy][http-conn]
(Reporter)

Comment 4

5 years ago
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.
Attachment #634521 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaa6906f8e92

I plan to let it bake for a few days then nom for aurora.
status-firefox16: affected → fixed
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/aaa6906f8e92
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 7

5 years ago
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.
Attachment #634521 - Flags: approval-mozilla-aurora?

Comment 8

5 years ago
(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?
(Assignee)

Comment 9

5 years ago
(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 on attachment 634521 [details] [diff] [review]
patch 0

Given that, approving for Aurora 15.
Attachment #634521 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/df798374a729
status-firefox15: affected → fixed
You need to log in before you can comment on or make changes to this bug.