755167 - spdysession::mConnection should not be nsHttpPipeline

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Patrick McManus [:mcmanus]]

pipelines set the connection member of their owned transactions to be itself. When pipelines are converted into spdysessions via TakeSubTransactions() that needs to be undone because the spdysession inherits its connection member based on the transaction passed to its ctor.

While this is a bug, I haven't yet been able to see anything bad come of it. With only minor exceptions the nsAHttpConnection methods that spdysession calls (by accident) on nsHttpPipeline just forward through to the original connection object. It does mean that a reference keeps the pipeline object alive far longer than it should be (it goes away when spdysession does), and that can't be good.

Comment 1

[Patrick McManus [:mcmanus]]

Attachment: patch 0

Comment 2

[Honza Bambas (:mayhemer)]

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

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

::: netwerk/protocol/http/nsHttpPipeline.cpp
@@ +485,5 @@
>      for (i = 0; i < count; ++i) {
>          nsAHttpTransaction *trans = Request(i);
> +        // set the transaction conneciton object back to the underlying
> +        // nsHttpConnectionHandle
> +        trans->SetConnection(mConnection);

Hmm.. looking at [1] I don't think this makes any sense.  You call here [2] at SpdySession::AddStream() aHttpTransaction->SetConnection(this);



[1] http://hg.mozilla.org/mozilla-central/annotate/00c7a320165b/netwerk/protocol/http/nsHttpConnection.cpp#l226
[2] http://hg.mozilla.org/mozilla-central/annotate/62910fbc21f1/netwerk/protocol/http/SpdySession.cpp#l366

Comment 3

[Honza Bambas (:mayhemer)]

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

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

> spdysession
> inherits its connection member based on the transaction passed to its ctor.

Ah!

Comment 4

[Patrick McManus [:mcmanus]]

[moot now - but I typed them so I'm saving the comment]

the issue is that the session gets its mConnection ptr from the transaction in the session ctor:

http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/SpdySession.cpp#106

only later on does it give that transaction to a stream and set the connection ptr in it back to the session:

http://hg.mozilla.org/mozilla-central/annotate/62910fbc21f1/netwerk/protocol/http/SpdySession.cpp#l366

Comment 5

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba73cb5eab6c

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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