Closed Bug 1342442 Opened 3 years ago Closed 3 years ago

Crash in nsCacheService::DispatchToCacheIOThread

Categories

(Core :: Networking: Cache, defect, critical)

defect
Not set
critical

Tracking

()

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

People

(Reporter: mayhemer, Assigned: mayhemer)

Details

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

Crash Data

Attachments

(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]
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.
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 ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bef322dbe4c4
Add null-check over gService in nsCacheService. r=michal
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bef322dbe4c4
Status: ASSIGNED → RESOLVED
Closed: 3 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]
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 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.