Closed Bug 1363034 Opened 4 years ago Closed 4 years ago

Crashes in NullHttpTransaction

Categories

(Core :: Networking: HTTP, defect)

55 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: dragana, Assigned: dragana)

References

Details

(Whiteboard: [necko-active])

Crash Data

Attachments

(1 file)

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.
Blocks: 1188435
Whiteboard: [necko-active]
Crash Signature: [@ nsCOMPtr<T>::nsCOMPtr<T> | mozilla::net::NullHttpTransaction::GetSecurityCallbacks] [@ RefPtr<T>::RefPtr<T> | mozilla::net::NullHttpTransaction::GetSecurityCallbacks] [@ mozilla::net::NullHttpTransaction::~NullHttpTransaction]
Attachment #8865526 - Flags: review?(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?
Attachment #8865526 - Flags: review?(mcmanus) → review+
(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.
Pushed by dd.mozilla@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb9ec98024db
Do not make new transactions for backupSocketTransport, behave as nsHttpConnectionMgr used to behave. r=mcmanus
Crash Signature: [@ nsCOMPtr<T>::nsCOMPtr<T> | mozilla::net::NullHttpTransaction::GetSecurityCallbacks] [@ RefPtr<T>::RefPtr<T> | mozilla::net::NullHttpTransaction::GetSecurityCallbacks] [@ mozilla::net::NullHttpTransaction::~NullHttpTransaction] → [@ nsCOMPtr<T>::nsCOMPtr<T> | mozilla::net::NullHttpTransaction::GetSecurityCallbacks] [@ RefPtr<T>::RefPtr<T> | mozilla::net::NullHttpTransaction::GetSecurityCallbacks] [@ mozilla::net::NullHttpTransaction::~NullHttpTransaction] [@ RefPtr<T>::assign_a…
https://hg.mozilla.org/mozilla-central/rev/fb9ec98024db
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Crash Signature: [@ nsCOMPtr<T>::nsCOMPtr<T> | mozilla::net::NullHttpTransaction::GetSecurityCallbacks] [@ RefPtr<T>::RefPtr<T> | mozilla::net::NullHttpTransaction::GetSecurityCallbacks] [@ mozilla::net::NullHttpTransaction::~NullHttpTransaction] [@ RefPtr<T>::assign_a… → [@ nsCOMPtr_base::assign_assuming_AddRef | mozilla::net::NullHttpTransaction::~NullHttpTransaction]
probably resolved by bug 1382555, which added some ref counting that was not there since this is really old code.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.