Crashes in NullHttpTransaction

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dragana, Assigned: dragana)

Tracking

55 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox-esr45 unaffected, firefox-esr52 unaffected, firefox53 unaffected, firefox54 unaffected, firefox55 fixed)

Details

(Whiteboard: [necko-active], crash signature)

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Updated

a year ago
Blocks: 1188435
Whiteboard: [necko-active]
(Assignee)

Updated

a year ago
Crash Signature: [@ nsCOMPtr<T>::nsCOMPtr<T> | mozilla::net::NullHttpTransaction::GetSecurityCallbacks] [@ RefPtr<T>::RefPtr<T> | mozilla::net::NullHttpTransaction::GetSecurityCallbacks] [@ mozilla::net::NullHttpTransaction::~NullHttpTransaction]
(Assignee)

Updated

a year ago
status-firefox55: --- → affected
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox-esr45: --- → unaffected
status-firefox-esr52: --- → unaffected
(Assignee)

Comment 1

a year ago
Created attachment 8865526 [details] [diff] [review]
bug_1363034_v1.patch
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+
(Assignee)

Comment 3

a year ago
(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

a year ago
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
(Assignee)

Updated

a year ago
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…

Comment 5

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fb9ec98024db
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
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 → ---
(Assignee)

Updated

a year ago
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]
(Assignee)

Comment 7

a year ago
probably resolved by bug 1382555, which added some ref counting that was not there since this is really old code.
Status: REOPENED → RESOLVED
Last Resolved: a year agoa year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.