Crash in nsCacheService::DispatchToCacheIOThread

RESOLVED FIXED in Firefox -esr52

Status

()

Core
Networking: Cache
--
critical
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

({crash})

unspecified
mozilla55
crash
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox51 wontfix, firefox52 wontfix, firefox-esr52 fixed, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

This bug was filed from the Socorro interface and is 
report bp-0e265816-9ea6-4c0f-8389-ecb4b2170224.
=============================================================

missing null check for gService, easy fix.
Created attachment 8840929 [details] [diff] [review]
v1
Attachment #8840929 - Flags: review?(michal.novotny)
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+
Whiteboard: [necko-active]
I just did a mass replace :D  Will fix.  Thanks.

Updated

a year ago
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
Created attachment 8843958 [details] [diff] [review]
v2
Attachment #8840929 - Attachment is obsolete: true
Attachment #8843958 - Flags: review?(michal.novotny)
Attachment #8843958 - Flags: review?(michal.novotny) → review+
Keywords: checkin-needed

Comment 5

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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bef322dbe4c4
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please request Aurora/Beta/ESR52 approval on this when you get a chance.
status-firefox51: affected → wontfix
status-firefox52: affected → wontfix
Flags: needinfo?(honzab.moz)
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

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/7dfb1fe34ea5
status-firefox54: affected → fixed

Comment 11

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/0f1965c2814f
status-firefox53: affected → fixed
Comment on attachment 8843958 [details] [diff] [review]
v2

See comment 8.
Attachment #8843958 - Flags: approval-mozilla-esr52?
Comment on attachment 8843958 [details] [diff] [review]
v2

shutdown crash fix for esr52
Attachment #8843958 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
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.