Closed
Bug 1381005
Opened 7 years ago
Closed 7 years ago
Crash in libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus
Categories
(Core :: Networking: HTTP, defect)
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | --- | fixed |
People
(Reporter: dragana, Assigned: dragana)
References
Details
(Keywords: crash, Whiteboard: [necko-active])
Crash Data
Attachments
(1 file, 1 obsolete file)
48.86 KB,
patch
|
dragana
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
Assignee | ||
Comment 1•7 years ago
|
||
This is crash in MOZ_DIAGNOSTIC_ASSERT(mEnt) that I have added. Maybe I was wrong to expect that mEnt must not be 0. I look at that code for so long and I do not see how this is happening. Strangely code later in the same function has check for mEnt != nullptr.
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Dragana Damjanovic [:dragana] from comment #1) > This is crash in MOZ_DIAGNOSTIC_ASSERT(mEnt) that I have added. > > Maybe I was wrong to expect that mEnt must not be 0. I look at that code for > so long and I do not see how this is happening. Strangely code later in the > same function has check for mEnt != nullptr. gHttpHandler->CoalesceSpdy() can null mEnt (I think)
Assignee | ||
Comment 3•7 years ago
|
||
There is a user comment saying that it happened while watching a youtube video.
Assignee | ||
Updated•7 years ago
|
Crash Signature: [libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus] → [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ]
Assignee | ||
Comment 4•7 years ago
|
||
(In reply to Dragana Damjanovic [:dragana] from comment #3) > There is a user comment saying that it happened while watching a youtube > video. I am running nightly on linux playing anything on youtube for 6 hours still no crash :(
Assignee | ||
Updated•7 years ago
|
Crash Signature: [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ] → [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ]
[@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupStreams]
Assignee | ||
Updated•7 years ago
|
Crash Signature: [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ]
[@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupStreams] → [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ]
[@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupStreams]
[@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::OnOutputStreamReady ]
Assignee | ||
Updated•7 years ago
|
Crash Signature: [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus ]
[@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupStreams]
[@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::OnOutputStreamReady ] → [@libxul.so@0xc242b6 | mozilla::net::nsSocketTransport::SendStatus]
[@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetupStreams]
[@ mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::OnOutputStreamReady]
[@ mozilla::net::nsHttpConnectionMgr:…
Assignee | ||
Comment 5•7 years ago
|
||
This make nsConnectionEntry ref countable. And adds a lot of MOZ_DIAGNOSTIC_ASSERT to figure out why mEnt gets nulled. This is only temporary, probably it will run only for a day. On the other side maybe we can make nsConnectionEntry ref countably in any case, but that will be in a different patch.
Attachment #8887011 -
Flags: review?(mcmanus)
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=797ba97bfab60dc2249ed63b6834131116a8df87
Comment 7•7 years ago
|
||
Comment on attachment 8887011 [details] [diff] [review] bug_find_howw_ment_is_null.patch Review of attachment 8887011 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpConnection.cpp @@ +1926,5 @@ > } > if (!ci) { > ci = mConnInfo; > } > + MOZ_DIAGNOSTIC_ASSERT(ci); hope we don't see that one in crash stacks :) ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ +3956,5 @@ > mBackupStreamOut = nullptr; > } > > // Lose references to input stream (and backup). > + if (mStreamIn) { its possible this is helpful for more than diagnostics :) @@ +4315,5 @@ > // Restore callbacks. > mStreamOut->AsyncWait(this, 0, 0, nullptr); > mSocketTransport->SetEventSink(this, nullptr); > mSocketTransport->SetSecurityCallbacks(this); > + mStreamIn->AsyncWait(nullptr, 0, 0, nullptr); obv this should have been mstreamin the whole time, right? but should that first param be this? ::: netwerk/protocol/http/nsHttpConnectionMgr.h @@ +335,5 @@ > uint32_t maxCount = 0); > > // Remove the empty pendingQ in |mPendingTransactionTable|. > void RemoveEmptyPendingQ(); > + uint8_t mHowItWasRemoved; please make this an enum @@ +408,5 @@ > void FastOpenNotSupported() override; > void SetFastOpenStatus(uint8_t tfoStatus) override; > void CancelFastOpenConnection(); > + > + RefPtr<nsConnectionEntry> mEnt; I originally was going to ask you to keep this stuff private and create one or more assert methods on the class.. but that seems like a big ask. so how about you move these to their own private section with a big comment (instead of landing in the existing public one) and include the necessary friend references that can later be removed?
Attachment #8887011 -
Flags: review?(mcmanus) → review+
Pushed by dd.mozilla@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/59c86ba26101 Add some diagnostic assertions to figure out why mEnt is null. This is only temporary. r=mcmanus
Assignee | ||
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/59c86ba2610141e0c9fa82defdae58f919a5c6bd Bug 1381005 - Add some diagnostic assertions to figure out why mEnt is null. This is only temporary. r=mcmanus
Assignee | ||
Comment 10•7 years ago
|
||
Attachment #8887011 -
Attachment is obsolete: true
Attachment #8887241 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/59c86ba26101
Assignee | ||
Comment 12•7 years ago
|
||
This crash actually went away with this patch. The patch made ConnectioEntry ref counted.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Updated•7 years ago
|
status-firefox54:
--- → unaffected
status-firefox55:
--- → unaffected
status-firefox56:
--- → fixed
status-firefox-esr52:
--- → unaffected
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•