Last Comment Bug 755167 - spdysession::mConnection should not be nsHttpPipeline
: spdysession::mConnection should not be nsHttpPipeline
Status: RESOLVED FIXED
[spdy][http-conn]
:
Product: Core
Classification: Components
Component: Networking: HTTP (show other bugs)
: unspecified
: x86_64 Windows 7
: -- minor (vote)
: mozilla15
Assigned To: Patrick McManus [:mcmanus] PTO until Sep 6
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-14 19:56 PDT by Patrick McManus [:mcmanus] PTO until Sep 6
Modified: 2012-06-21 06:41 PDT (History)
4 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 0 (1.02 KB, patch)
2012-05-14 20:03 PDT, Patrick McManus [:mcmanus] PTO until Sep 6
honzab.moz: review+
Details | Diff | Splinter Review

Description Patrick McManus [:mcmanus] PTO until Sep 6 2012-05-14 19:56:24 PDT
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] PTO until Sep 6 2012-05-14 20:03:55 PDT
Created attachment 623918 [details] [diff] [review]
patch 0
Comment 2 Honza Bambas (:mayhemer) 2012-05-15 13:20:16 PDT
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) 2012-05-15 13:26:45 PDT
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] PTO until Sep 6 2012-05-15 13:29:37 PDT
[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] PTO until Sep 6 2012-05-16 06:29:59 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba73cb5eab6c
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-05-16 19:46:19 PDT
https://hg.mozilla.org/mozilla-central/rev/ba73cb5eab6c

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