Closed
Bug 1342442
Opened 7 years ago
Closed 7 years ago
Crash in nsCacheService::DispatchToCacheIOThread
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: mayhemer, Assigned: mayhemer)
Details
(Keywords: crash, Whiteboard: [necko-active])
Crash Data
Attachments
(1 file, 1 obsolete file)
4.18 KB,
patch
|
michal
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-0e265816-9ea6-4c0f-8389-ecb4b2170224. ============================================================= missing null check for gService, easy fix.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8840929 -
Flags: review?(michal.novotny)
Comment 2•7 years ago
|
||
Comment on attachment 8840929 [details] [diff] [review] v1 Review of attachment 8840929 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/cache/nsCacheService.cpp @@ +1430,5 @@ > CACHE_LOG_DEBUG(("Dooming entry for session %p, key %s\n", > session, PromiseFlatCString(key).get())); > NS_ASSERTION(gService, "nsCacheService::gService is null."); > > + if (!gService || !gService->mInitialized) If we expect this can happen then we should remove the assertion. @@ +2030,5 @@ > NS_ASSERTION(gService, "nsCacheService::gService is null."); > if (result) > *result = nullptr; > > + if (!gService || !gService->mInitialized) Again, remove the assertion. @@ +2354,5 @@ > void > nsCacheService::OnProfileShutdown() > { > if (!gService) return; > + if (!gService || !gService->mInitialized) { gService is already checked above.
Attachment #8840929 -
Flags: review?(michal.novotny) → feedback+
Updated•7 years ago
|
Whiteboard: [necko-active]
Assignee | ||
Comment 3•7 years ago
|
||
I just did a mass replace :D Will fix. Thanks.
Updated•7 years ago
|
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
status-firefox-esr52:
--- → affected
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8840929 -
Attachment is obsolete: true
Attachment #8843958 -
Flags: review?(michal.novotny)
Updated•7 years ago
|
Attachment #8843958 -
Flags: review?(michal.novotny) → review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/bef322dbe4c4 Add null-check over gService in nsCacheService. r=michal
Keywords: checkin-needed
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bef322dbe4c4
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 7•7 years ago
|
||
Please request Aurora/Beta/ESR52 approval on this when you get a chance.
Flags: needinfo?(honzab.moz)
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8843958 [details] [diff] [review] v2 Approval Request Comment [Feature/Bug causing the regression]: the old http cache (since ever) [User impact if declined]: shutdown null deref crash [Is this code covered by automated tests?]: not the case when the pointer is null, but this code path is tested [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: no str, no testing [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: it's just a simple null check. callers handle error result well [String changes made/needed]: none
Flags: needinfo?(honzab.moz)
Attachment #8843958 -
Flags: approval-mozilla-beta?
Attachment #8843958 -
Flags: approval-mozilla-aurora?
Comment on attachment 8843958 [details] [diff] [review] v2 Crash fix, Beta53+, Aurora54+
Attachment #8843958 -
Flags: approval-mozilla-beta?
Attachment #8843958 -
Flags: approval-mozilla-beta+
Attachment #8843958 -
Flags: approval-mozilla-aurora?
Attachment #8843958 -
Flags: approval-mozilla-aurora+
Comment 10•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/7dfb1fe34ea5
Comment 11•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/0f1965c2814f
Comment 12•7 years ago
|
||
Comment on attachment 8843958 [details] [diff] [review] v2 See comment 8.
Attachment #8843958 -
Flags: approval-mozilla-esr52?
Comment 13•7 years ago
|
||
Comment on attachment 8843958 [details] [diff] [review] v2 shutdown crash fix for esr52
Attachment #8843958 -
Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Comment 14•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/775b6f85ef81
Comment 15•7 years ago
|
||
Setting qe-verify- based on Honza's assessment on manual testing needs and the fact that this fix is, in some measure, covered by automated tests (see Comment 8).
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•