Note: There are a few cases of duplicates in user autocompletion which are being worked on.

spdysession::mConnection should not be nsHttpPipeline

RESOLVED FIXED in mozilla15

Status

()

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

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

unspecified
mozilla15
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment)

(Assignee)

Description

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

Comment 1

5 years ago
Created attachment 623918 [details] [diff] [review]
patch 0
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+
(Assignee)

Comment 4

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

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba73cb5eab6c
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/ba73cb5eab6c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.