Closed Bug 1342442 Opened 4 years ago Closed 4 years ago

Crash in nsCacheService::DispatchToCacheIOThread


(Core :: Networking: Cache, defect)

Not set



Tracking Status
firefox51 --- wontfix
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed


(Reporter: mayhemer, Assigned: mayhemer)


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

Crash Data


(1 file, 1 obsolete file)

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

missing null check for gService, easy fix.
Attached patch v1 (obsolete) — Splinter Review
Attachment #8840929 - Flags: review?(michal.novotny)
Comment on attachment 8840929 [details] [diff] [review]

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.
Attached patch v2Splinter Review
Attachment #8840929 - Attachment is obsolete: true
Attachment #8843958 - Flags: review?(michal.novotny)
Attachment #8843958 - Flags: review?(michal.novotny) → review+
Keywords: checkin-needed
Pushed by
Add null-check over gService in nsCacheService. r=michal
Keywords: checkin-needed
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Please request Aurora/Beta/ESR52 approval on this when you get a chance.
Flags: needinfo?(honzab.moz)
Comment on attachment 8843958 [details] [diff] [review]

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]

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 on attachment 8843958 [details] [diff] [review]

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

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.