950164 - crash in nsCOMPtr_base::assign_assuming_AddRef(nsISupports*) | mozilla::net::CacheFileIOManager::OnProfile()

Status: VERIFIED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

This bug was filed from the Socorro interface and is 
report bp-62b39577-a61e-44cd-9890-60c702131211.
=============================================================

Crash Address 	0x10

Missing gInstance check?

michal@156755
966 if (directory) {
michal@156755
> 967 rv = directory->Clone(getter_AddRefs(gInstance->mCacheDirectory));
michal@156755
968 NS_ENSURE_SUCCESS(rv, rv);

Comment 2

[Honza Bambas (:mayhemer)]

Should be fixed before the temp enable.

Comment 3

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 4

[Michal Novotny [:michal]]

Comment on attachment 8372672 [details] [diff] [review]
v1

Review of attachment 8372672 [details] [diff] [review]:
-----------------------------------------------------------------

CacheFileIOManager::OnProfile() is called only from CacheObserver::Observe() and we call CacheFileIOManager::Init() just before we call OnProfile(). Maybe a better fix would be to call OnProfile() only when Init() succeeds or fails with NS_ERROR_ALREADY_INITIALIZED, i.e. something like:

  rv = CacheFileIOManager::Init();
  if (NS_SUCCEEDED(rv) || rv == NS_ERROR_ALREADY_INITIALIZED) {
    CacheFileIOManager::OnProfile();
  }

I would slightly prefer this fix over your patch, but it is up to you. Anyway, the question here is why initialization of CacheFileIOManager failed?

Comment 5

[Honza Bambas (:mayhemer)]

I was thinking of this and it's similar to discussion in bug 971975.  Doing non-null checks in a method is safer than doing state checks out of a method.  So I'll rather leave it, since this is a crash fix.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 8372672 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/4da945c0084c

Comment 7

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #4)
> Anyway, the question here is why initialization of CacheFileIOManager failed?

"probably could not create the IO thread" as the comment in the patch says.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4da945c0084c

Comment 9

[Honza Bambas (:mayhemer)]

Verified after the cache2 trial (bug 967693).