Crash in nsTHashtable<T>::GetEntry | nsClassHashtable<T>::Get | mozilla::net::nsHttpTransaction::ShouldStopReading

RESOLVED FIXED in Firefox 55

Status

()

--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mats, Assigned: mayhemer)

Tracking

({crash})

Trunk
mozilla56
Unspecified
Windows 7
crash
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 fixed, firefox56 fixed)

Details

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

Attachments

(1 attachment)

(Reporter)

Description

a year ago
This bug was filed from the Socorro interface and is 
report bp-55094a05-4bf5-4ef1-b7ba-a386d0170614.
=============================================================

Looks like a recent regression, first reported crash is for 20170611030208.
Possibly a regression from bug 1369496?

nsTHashtable<mozilla::dom::WebIDLNameTableEntry>::GetEntry(mozilla::dom::WebIDLNameTableKey const&)
nsClassHashtable<nsUint64HashKey, nsTArray<RefPtr<mozilla::net::nsHttpConnectionMgr::PendingTransactionInfo> > >::Get(unsigned __int64 const&)
mozilla::net::nsHttpTransaction::ShouldStopReading()
mozilla::net::nsHttpTransaction::WriteSegments(mozilla::net::nsAHttpSegmentWriter*, unsigned int, unsigned int*)
mozilla::net::nsHttpConnection::OnSocketReadable()
mozilla::net::nsHttpConnection::OnInputStreamReady(nsIAsyncInputStream*)
mozilla::net::nsSocketInputStream::OnSocketReady(nsresult)
mozilla::net::nsSocketTransport::OnSocketDetached(PRFileDesc*)
mozilla::net::nsSocketTransportService::DetachSocket(mozilla::net::nsSocketTransportService::SocketContext*, mozilla::net::nsSocketTransportService::SocketContext*)
mozilla::net::nsSocketTransportService::DoPollIteration(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>*)
mozilla::net::nsSocketTransportService::Run()
[...]
Flags: needinfo?(honzab.moz)
(Reporter)

Updated

a year ago
Crash Signature: [@ nsTHashtable<T>::GetEntry | nsClassHashtable<T>::Get | mozilla::net::nsHttpTransaction::ShouldStopReading] → [@ nsTHashtable<T>::GetEntry | nsClassHashtable<T>::Get | mozilla::net::nsHttpTransaction::ShouldStopReading] [@ PLDHashTable::Search | mozilla::net::nsHttpTransaction::ShouldStopReading ] [@ PLDHashTable::Search | nsClassHashtable<T>::Get | mozilla::ne…
(Assignee)

Comment 1

a year ago
Looks like we are missing some null checks.
Flags: needinfo?(honzab.moz)
QA Contact: honzab.moz
Whiteboard: [necko-active]
(Assignee)

Comment 2

a year ago
Created attachment 8878044 [details] [diff] [review]
v1 (add a nullcheck)

When can this happen?  Can there be no entry when there is a transaction with the key?  Is it something with coalescing?
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Attachment #8878044 - Flags: review?(mcmanus)
(Assignee)

Updated

a year ago
QA Contact: honzab.moz
Comment on attachment 8878044 [details] [diff] [review]
v1 (add a nullcheck)

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

if this is before the getorcreateentry() call this could happen.. also entries can be removed from ct if they go idle.
Attachment #8878044 - Flags: review?(mcmanus) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Blocks: 1369496
(Assignee)

Comment 4

a year ago
Comment on attachment 8878044 [details] [diff] [review]
v1 (add a nullcheck)

Approval Request Comment
[Feature/Bug causing the regression]: bug 1369496
[User impact if declined]: crash (null crash)
[Is this code covered by automated tests?]: yes, but not for the crashing case apparently...
[Has the fix been verified in Nightly?]: not even landed
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: instead of null-deref we early return false, the caller is safe with it
[String changes made/needed]: none
Attachment #8878044 - Flags: approval-mozilla-beta?
(Assignee)

Updated

a year ago
status-firefox54: --- → unaffected
status-firefox55: --- → affected
status-firefox56: --- → affected
status-firefox-esr52: --- → unaffected

Comment 5

a year ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf586f9f5eeb
Add null-check for a connection entry to nsHttpConnectionMgr::IsConnEntryUnderPressure. r=mcmanus
Keywords: checkin-needed

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf586f9f5eeb
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox56: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment on attachment 8878044 [details] [diff] [review]
v1 (add a nullcheck)

add null check to fix crash, beta55+
Attachment #8878044 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 8

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/e2638104f016
status-firefox55: affected → fixed
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.