1363034 - Crashes in NullHttpTransaction

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Dragana Damjanovic [:dragana]]

There are couple of crashes in NullHttpTransaction, I am sure they cause by bug 1188435. I think I know what is the problem, I will submit a patch soon.

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: bug_1363034_v1.patch

Comment 2

[Patrick McManus [:mcmanus]]

Comment on attachment 8865526 [details] [diff] [review]
bug_1363034_v1.patch

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

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ -3820,5 @@
>          }
>      }
>      if (aFastOpen) {
> -        // If it is fast open create a new tranaction for backup stream.
> -        mTransaction = new NullHttpTransaction(mEnt->mConnInfo,

that will fix the problem.. but aren't we losing part of the feature?

Comment 3

[Dragana Damjanovic [:dragana]]

(In reply to Patrick McManus [:mcmanus] from comment #2)
> Comment on attachment 8865526 [details] [diff] [review]
> bug_1363034_v1.patch
> 
> Review of attachment 8865526 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
> @@ -3820,5 @@
> >          }
> >      }
> >      if (aFastOpen) {
> > -        // If it is fast open create a new tranaction for backup stream.
> > -        mTransaction = new NullHttpTransaction(mEnt->mConnInfo,
> 
> that will fix the problem.. but aren't we losing part of the feature?

If the primary socketTransoort takes too long its transaction will be given to the  backupSocetTransport and it will be put into pendingQueue so that it can be found in the pendinQueue and the transaction will be dispatched on backupSocetTransport connection. If the primary socketTransoort does not take too long, the backupSocetTransport connection will not find the mTransaction in pending queue and this backup connection will be reclaimed.
So this NullHttpTransaction is useless.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb9ec98024db
Do not make new transactions for backupSocketTransport, behave as nsHttpConnectionMgr used to behave. r=mcmanus

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 6

[Calixte Denizet (:calixte)]

There is 1 crash in nightly 55 with buildid 20170511030301 with signature "RefPtr<T>::RefPtr<T> | mozilla::net::NullHttpTransaction::GetSecurityCallbacks" (see [1]).

[1] https://crash-stats.mozilla.com/report/index/86eead2b-d7c2-447a-bc8a-793ec0170511

Comment 7

[Dragana Damjanovic [:dragana]]

probably resolved by bug 1382555, which added some ref counting that was not there since this is really old code.