Closed
Bug 1382555
Opened 7 years ago
Closed 7 years ago
Crash in mozilla::net::nsHttpConnectionMgr::nsConnectionEntry::~nsConnectionEntry
Categories
(Core :: Networking: HTTP, enhancement)
Tracking
()
RESOLVED
FIXED
People
(Reporter: dragana, Assigned: dragana)
Details
(Whiteboard: [necko-active])
Crash Data
Attachments
(3 files)
14.40 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
15.78 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
32.28 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
Looking at this diagnostic crash, I think it is wrong and it should not happen. nsConnectionEntry is destroyed (this mean it is removed from mCT as well (I change mCT into: nsRefPtrHashtable<nsCStringHashKey, nsConnectionEntry> mCT;)) When a connection is finished, e.g. closed, or a transaction finishes, ReclainConnection will be call. Not finished connections are in ActiveConn list. ReclaimConnection needs the connections's nsConnectionEntry and it will try to find it in mCT if it does not find it it will create a new one. There is a comment before creating the new nsConnectionEntry says: " if (!ent) { // this can happen if the connection is made outside of the // connection manager and is being "reclaimed" for use with // future transactions. HTTP/2 tunnels work like this." I think this is not the case here... I am not 100% sure... a nsConnectionEntry is removed from mCT in: 1) nsHttpConnectionMgr::ClearConnectionHistory() - here we check whether ent->mActiveConns.Length() == 0 2) nsHttpConnectionMgr::OnMsgShutdown - here I am not sure I would need to check this one. Maybe something holds reference to connection after conn->Close() is callled. 3) nsHttpConnectionMgr::OnMsgPruneDeadConnections - here we check whether ent->mActiveConns.Length() == 0 I do not know details of coalescing, very, very well, but I think coalesing does not influence my observation. Patrick, does this make any sense?
Flags: needinfo?(mcmanus)
Assignee | ||
Comment 2•7 years ago
|
||
Crash is in line: // nsHttpConnectionMgr::nsConnectionEntry nsHttpConnectionMgr::nsConnectionEntry::~nsConnectionEntry() { LOG(("nsConnectionEntry::~nsConnectionEntry this=%p", this)); MOZ_DIAGNOSTIC_ASSERT(!mIdleConns.Length()); MOZ_DIAGNOSTIC_ASSERT(!mActiveConns.Length()); <--------- MOZ_DIAGNOSTIC_ASSERT(!mHalfOpens.Length()); MOZ_DIAGNOSTIC_ASSERT(!mUrgentStartQ.Length()); MOZ_DIAGNOSTIC_ASSERT(!PendingQLength()); MOZ_DIAGNOSTIC_ASSERT(!mHalfOpenFastOpenBackups.Length()); MOZ_DIAGNOSTIC_ASSERT(!mDoNotDestroy); MOZ_COUNT_DTOR(nsConnectionEntry); } https://hg.mozilla.org/mozilla-central/annotate/1b065ffd8a53/netwerk/protocol/http/nsHttpConnectionMgr.cpp#l2840
Assignee | ||
Comment 3•7 years ago
|
||
This crash is happening when halfOpenSock is destroyed, so nsConnectionEntry is not removed in nsHttpConnectionMgr::OnMsgShutdown, because we have diagnostic assert just before that and all halfOpenSocks are abandon, i.e. in ::Abandon() the halfOpenSocket is removed as listener to OnSocketReady and it should never get that callback! code: // Close all half open tcp connections. for (int32_t i = int32_t(ent->mHalfOpens.Length()) - 1; i >= 0; i--) { ent->mHalfOpens[i]->Abandon(); } MOZ_DIAGNOSTIC_ASSERT(ent->mHalfOpenFastOpenBackups.Length() == 0 && !ent->mDoNotDestroy); ent->mHowItWasRemoved = nsConnectionEntry::CONN_ENTRY_REMOVED_SHUTDOWN; iter.Remove();
Assignee | ||
Comment 4•7 years ago
|
||
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=df604228a5785b36b580a1b461c48be33f6f98a8
Comment 6•7 years ago
|
||
This is the #6 Windows topcrash in Nightly 20170721030204. It has occurred 139 times across 86 installations in the past 7 days.
Updated•7 years ago
|
Attachment #8888833 -
Flags: review?(mcmanus) → review+
Pushed by dd.mozilla@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd090362de7 Add more diagnostic asserts to nsHtttpConnectionMgr. r=mcmanus
Assignee | ||
Comment 8•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bd090362de77a8bf247e3c4f97a9f76f9afc04d Bug 1382555 - Add more diagnostic asserts to nsHtttpConnectionMgr. r=mcmanus
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0bd090362de7
Updated•7 years ago
|
Whiteboard: [necko-active]
Comment 10•7 years ago
|
||
This fix also broght some improvements: == Change summary for alert #8215 (as of July 24 2017 14:26 UTC) == Improvements: 3% tp5o Private Bytes windows7-32 pgo e10s 153,898,506.63 -> 149,398,274.96 3% tp5o_webext Main_RSS linux64 pgo e10s 186,215,295.06 -> 181,287,319.14 2% tp5o_webext Main_RSS linux64 opt e10s 192,233,333.57 -> 188,069,587.86 2% tp5o Private Bytes windows7-32 opt e10s 152,538,413.13 -> 149,261,416.09 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8215
Assignee | ||
Updated•7 years ago
|
Crash Signature: [@ mozilla::net::nsHttpConnectionMgr::nsConnectionEntry::~nsConnectionEntry] → [@ mozilla::net::nsHttpConnectionMgr::nsConnectionEntry::~nsConnectionEntry]
[@ mozilla::net::nsHttpConnectionMgr::CheckConnEntryMustBeInmCT]
Assignee | ||
Comment 11•7 years ago
|
||
I needed to add some changes to diagnostic asserts because the last patch does not help in discovering the bug. I have also made some small changes, that were wrong, but I do not think they are the cause of this bug.
Attachment #8889960 -
Flags: review?(mcmanus)
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8450fe7832a9de4866bf26939657f9dbed62b9d3
Assignee | ||
Comment 13•7 years ago
|
||
This is one of the new crashes: https://crash-stats.mozilla.com/report/index/027c5603-0f58-4b38-aaca-44f990170725 and it crashes at: 5181 nsHttpConnectionMgr::CheckConnEntryMustBeInmCT(nsHttpConnectionInfo *ci) 5182 { 5183 nsConnectionEntry *ent = mCT.GetWeak(ci->HashKey()); 5184 MOZ_DIAGNOSTIC_ASSERT(ent); 5185 if (ent->mHowItWasRemoved == nsConnectionEntry::CONN_ENTRY_CLEAR_CONNECTION_HISTORY) { 5186 MOZ_DIAGNOSTIC_ASSERT(false); 5187 } else if (ent->mHowItWasRemoved == nsConnectionEntry::CONN_ENTRY_REMOVED_SHUTDOWN) { 5188 MOZ_DIAGNOSTIC_ASSERT(false); 5189 } 5190 } at line 5184.
Assignee | ||
Comment 14•7 years ago
|
||
Comment on attachment 8889960 [details] [diff] [review] bug_1382555_v2.patch Honza, can you review this. It only adding some assert change and 2 0ther changes that will ensure once again that callbacks are reset(this is only for double check)
Attachment #8889960 -
Flags: review?(mcmanus) → review?(honzab.moz)
Updated•7 years ago
|
Attachment #8889960 -
Flags: review?(honzab.moz) → review+
Comment 15•7 years ago
|
||
Pushed by dd.mozilla@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b4cd32bc7d14 Change some diagnostic asserts to find out why mEnt is nof in mCT. r=mcmanus
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b4cd32bc7d14
Assignee | ||
Comment 17•7 years ago
|
||
Flags: needinfo?(mcmanus)
Attachment #8892052 -
Flags: review?(mcmanus)
Assignee | ||
Comment 18•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10f58633e7e12ad5badd9fe77d9ec2b070121420
Updated•7 years ago
|
Attachment #8892052 -
Flags: review?(mcmanus) → review+
Comment 19•7 years ago
|
||
Pushed by dd.mozilla@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2bb82c799d1a Remove diagnostic asserts.r=mcmanus
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2bb82c799d1a
Assignee | ||
Comment 21•7 years ago
|
||
Fixed by bug 1363372
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 22•6 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•