The default bug view has changed. See this FAQ.

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.