I am confused about why asyncOpenCacheEntry is not always asynchronous. In particular, it isn't asynchronous when called from any thread except the main thread. I suggest that we make asyncOpenCacheEntry always asynchronous to make it easier to understand. This will make the patch for bug 660749 easier to understand, so I would like to get reviews for it right away if possible. Thanks! Note that this patch makes the listener argument for asyncOpenCacheEntry* mandatory in order to enable this simplification. In practice, this won't be an issue, AFAICT.

Comment on attachment 609382 [details] [diff] [review] Make asyncOpenCacheEntry easier to understand Review of attachment 609382 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good, and doesn't seem to be a bad idea. I've got a couple comments below, but I'll go with whatever Michal says in terms of r+ or r-, so I'm just clearing my r? ::: netwerk/cache/nsCacheService.cpp @@ -1735,5 @@ > > nsCOMPtr<nsIRunnable> ev = > new nsCacheListenerEvent(listener, descriptor, > accessGranted, status); > - if (!ev) { We still need to handle this condition, just in case, yes? @@ +1856,5 @@ > CACHE_LOG_DEBUG(("Created request %p\n", request)); > > // Process the request on the background thread if we are on the main thread > // and the the request is asynchronous > + if (listener) { I think we still want "&& gService->mCacheIOThread" here, since it doesn't make sense to even try dispatching asynchronously if we know we don't have a thread to dispatch it to. I'm not sure why that would ever happen, but it is a possibility. @@ +1872,3 @@ > > // delete requests that have completed > + if (!(blockingMode && Given the above, I think we still need "listener &&" here.

(In reply to Nick Hurley [:hurley] from comment #1) > ::: netwerk/cache/nsCacheService.cpp > @@ -1735,5 @@ > > > > nsCOMPtr<nsIRunnable> ev = > > new nsCacheListenerEvent(listener, descriptor, > > accessGranted, status); > > - if (!ev) { > > We still need to handle this condition, just in case, yes? operator new cannot return null in Gecko. See https://developer.mozilla.org/en/Infallible_memory_allocation > @@ +1856,5 @@ > > CACHE_LOG_DEBUG(("Created request %p\n", request)); > > > > // Process the request on the background thread if we are on the main thread > > // and the the request is asynchronous > > + if (listener) { > > I think we still want "&& gService->mCacheIOThread" here, since it doesn't > make sense to even try dispatching asynchronously if we know we don't have a > thread to dispatch it to. I'm not sure why that would ever happen, but it is > a possibility. DispatchToCacheIOThread already does a check: nsresult nsCacheService::DispatchToCacheIOThread(nsIRunnable* event) { if (!gService->mCacheIOThread) return NS_ERROR_NOT_AVAILABLE; return gService->mCacheIOThread->Dispatch(event, NS_DISPATCH_NORMAL); }

(In reply to Brian Smith (:bsmith) from comment #2) > DispatchToCacheIOThread already does a check: > > nsresult > nsCacheService::DispatchToCacheIOThread(nsIRunnable* event) > { > if (!gService->mCacheIOThread) return NS_ERROR_NOT_AVAILABLE; > return gService->mCacheIOThread->Dispatch(event, NS_DISPATCH_NORMAL); > } And, returning NS_ERROR_NOT_AVAILABLE from *OpenCacheEntry* is the right thing to do, because if the cache IO thread isn't running, then that means you're accessing the cache when you're not supposed to be (too early at startup or too late at shutdown).

OK, that all sounds fine to me. We just need to figure out if this will do something funky to wyciwyg channels (which are already doing cache ops on the cache thread, which could be why we force "async" opens to actually be synchronous when they're not on the main thread).

Comment on attachment 609382 [details] [diff] [review] Make asyncOpenCacheEntry easier to understand Looks good. AFAIK, as of now, we don't call AsyncOpenCacheEntry() on cache IO thread or any other background thread. In wyciwyg channel we call only synchronous OpenCacheEntry() on the cache IO thread. It would be good to land this together with bug #649194 because then we'll be sure that the cache IO thread is always available.

This patch is missing the NS_ENSURE_ARG_POINTER(listener) at the start of AsyncOpenCacheEntry. Nick, in your patch, you will need to put NS_ENSURE_ARG_POINTER(listener) in the DoAsyncOpen. I will comment in the other bug.

Add the null pointer check I mentioned in the previous comment.

Merged to Nick's patch in bug 712914.

Based on Michal's feedback over the phone, I decided not to do this. See also bug 722034 comment 21.