1342442 - Crash in nsCacheService::DispatchToCacheIOThread

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

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

missing null check for gService, easy fix.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 2

[Michal Novotny [:michal]]

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.

Comment 3

[Honza Bambas (:mayhemer)]

I just did a mass replace :D  Will fix.  Thanks.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v2

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bef322dbe4c4
Add null-check over gService in nsCacheService. r=michal

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bef322dbe4c4

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta/ESR52 approval on this when you get a chance.

Comment 8

[Honza Bambas (:mayhemer)]

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

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8843958 [details] [diff] [review]
v2

Crash fix, Beta53+, Aurora54+

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7dfb1fe34ea5

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f1965c2814f

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8843958 [details] [diff] [review]
v2

See comment 8.

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8843958 [details] [diff] [review]
v2

shutdown crash fix for esr52

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/775b6f85ef81

Comment 15

[Andrei Vaida [:avaida]]

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).