765665 - nsCacheService::GetCacheIOTarget takes cache service lock

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Stop acquiring the cache service lock in nsICacheService::GetCacheIOTarget

In Part 5 of the patch series for bug 722034, I changed nsHttpChannel to do its cache queries by dispatching an event to the cache service thread. In the original patch, I used nsCacheService::DispatchToCacheIOThread and everything worked fine. However, we decided to change the patch to call nsICacheService::GetCacheIOTarget()->Dispatch() in later versions of the patch. Unfortunately, I failed to notice that nsICacheService::GetCacheIOTarget() takes the cache service lock whereas nsCacheService::DispatchToCacheIOThread does not, and I didn't rerun my performance tests after I made that change. :(

The whole point of that patch series is to avoid taking the cache service lock on the main thread during cache queries, so obviously it is not a good idea to start off by immediately acquiring the lock.

We have two choices: either we can change the patch to call nsCacheService::DispatchToCacheIOThread directly again, or we can remove the lock acquisition from nsICacheService::GetCacheIOTarget(). I have chosen to do the latter.

Technically, this bug "affects" all versions of Firefox, but it particularly affects v15 and v16 because those are the versions that have the patches from bug 722034.

I am not sure that this change is sufficient to turn the patches bug 722034 into a clear win, but it is likely necessary. Let's try this change first before we try backing out bug 722034.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Stop acquiring the cache service lock in nsICacheService::GetCacheIOTarget

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 633967 [details] [diff] [review]
Stop acquiring the cache service lock in nsICacheService::GetCacheIOTarget

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

I got a little agressive with the "only allowed on the main thread" checks. But, I think the important parts of the patch are OK.

::: netwerk/cache/nsCacheService.cpp
@@ +792,5 @@
>  
>  nsresult
>  nsCacheService::DispatchToCacheIOThread(nsIRunnable* event)
>  {
> +    if (!NS_IsMainThread()) {

Based on tryserver results, I see that this check is not valid as there are places within the cache service where DispatchToCacheIOThread is called off the main thread. But, this doesn't affect the important parts of the patch.

@@ +805,5 @@
>  nsresult
>  nsCacheService::SyncWithCacheIOThread()
>  {
>      gService->mLock.AssertCurrentThreadOwns();
> +    if (!NS_IsMainThread()) {

Ditto. I imagine this isn't right, and it isn't necessary either.

@@ +1104,5 @@
>  
>  nsCacheService::~nsCacheService()
>  {
> +    if (mInitialized) { // Shutdown hasn't been called yet.
> +        if (!NS_IsMainThread()) {

This check is redundant with the check I added to Shutdown().

Comment 3

[Michal Novotny [:michal]]

Comment on attachment 633967 [details] [diff] [review]
Stop acquiring the cache service lock in nsICacheService::GetCacheIOTarget

> >  nsresult
> >  nsCacheService::DispatchToCacheIOThread(nsIRunnable* event)
> >  {
> > +    if (!NS_IsMainThread()) {
> 
> Based on tryserver results, I see that this check is not valid as there are
> places within the cache service where DispatchToCacheIOThread is called off
> the main thread.

Right. Remove this check.


> >  nsCacheService::SyncWithCacheIOThread()
> >  {
> >      gService->mLock.AssertCurrentThreadOwns();
> > +    if (!NS_IsMainThread()) {
> 
> Ditto. I imagine this isn't right, and it isn't necessary either.

This method could be called on any thread except the cache IO thread. So we have a potential deadlock in our code because following call stack is in theory possible on the cache IO thread:

nsCacheService::SyncWithCacheIOThread
nsDiskCacheDevice::Shutdown_Private
nsDiskCacheDevice::ClearDiskCache
nsDiskCacheDevice::EvictEntries
nsCacheService::EvictEntriesForClient
nsCacheService::EvictEntriesForSession
nsCacheSession::EvictEntries


> +    // nsCacheService::Init must be called on the main thread because
> +    // mCacheIOThread must only be created and destroyed on the main thread
> +    if (!NS_IsMainThread()) {
> +        MOZ_NOT_REACHED("nsCacheService::Init called off the main thread");
> +        return NS_ERROR_NOT_SAME_THREAD;
> +    }

The MOZ_NOT_REACHED shouldn't be there. This could really happen e.g. when some addon would try to initialize the cache service on some background thread...


>  NS_IMETHODIMP nsCacheService::GetCacheIOTarget(nsIEventTarget * *aCacheIOTarget)
>  {
> -    nsCacheServiceAutoLock lock;
> +    NS_ENSURE_ARG_POINTER(aCacheIOTarget);
> +    if (!NS_IsMainThread()) {
> +      MOZ_NOT_REACHED("nsCacheService::GetCacheIOTarget called off the main thread");
> +      *aCacheIOTarget = nsnull;
> +      return NS_ERROR_NOT_SAME_THREAD;
> +    }

What about grabbing the lock only in case we are not on the main thread?

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Stop acquiring the cache service lock in nsICacheService::GetCacheIOTarget when on the main thread

Michal, this implements your suggestion.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b40a155d4c

Comment 6

[Ed Morley [:emorley]]

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

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

[Triage Comment]

If this is something that should be uplifted to Beta (Fx 15), please nominate for approval.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

The main reason we wanted to do this was to follow up on bug 722034, but since all that work got backed out of Aurora 15 before the beta merge, there's nothing to do for 15 now.