Closed Bug 755167 Opened 9 years ago Closed 9 years ago

spdysession::mConnection should not be nsHttpPipeline

Categories

(Core :: Networking: HTTP, defect)

x86_64
Windows 7
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: mcmanus, Assigned: mcmanus)

Details

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

Attachments

(1 file)

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.
Attached patch patch 0Splinter Review
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
Attachment #623918 - Flags: review?(honzab.moz)
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 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!
Attachment #623918 - Flags: review?(honzab.moz) → review+
[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
https://hg.mozilla.org/mozilla-central/rev/ba73cb5eab6c
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [spdy] → [spdy][http-conn]
You need to log in before you can comment on or make changes to this bug.