722033 - Remove calls to synchronous openCacheEntry in nsHttpChannel

Status: RESOLVED FIXED

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

Description

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

+++ This bug was initially created as a clone of Bug #695399 +++

Synchronous openCacheEntry takes the cache service lock, and that lock can block on disk I/O, so we cannot call it from the main thread, including anywhere in nsHttpChannel.

Comment 1

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

This is blocking P1 work in PSM and it is blocking bugs that likely need to be fixed to keep the disk cache enabled on mobile. I am going to take a stab at it.

Comment 2

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

Michal, do you have anything that I start testing here? I would like to build my patches on top of this one, even if the changes to the tests are not ready.

Comment 3

[Michal Novotny [:michal]]

(In reply to Brian Smith (:bsmith) from comment #2)
> Michal, do you have anything that I start testing here? I would like to
> build my patches on top of this one, even if the changes to the tests are
> not ready.

What exactly do you need for testing? BTW changing tests is not a scope of this bug and will be covered by bug #695399.

So far I have eliminated openCacheEntry() that is needed due to LOAD_BYPASS_LOCAL_CACHE_IF_BUSY flag, but the patch break some xpc-tests and I'm now trying to find out what's wrong.

Comment 4

[Michal Novotny [:michal]]

Attachment: patch 1

- adds blocking argument to nsICacheSession::asyncOpenCacheEntry()
- removes OpenCacheEntry() needed due to LOAD_BYPASS_LOCAL_CACHE_IF_BUSY flag

Comment 5

[Michal Novotny [:michal]]

Attachment: patch 2 - removes OpenCacheEntry() in OpenOfflineCacheEntryForWriting()

This patch should be applied on the top of patch #601097.

Comment 6

[Honza Bambas (:mayhemer)]

I'll publish the reviews probably tomorrow.

Comment 7

[Honza Bambas (:mayhemer)]

Has this been pushed to try?

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 601097 [details] [diff] [review]
patch 1

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

r=honzab

I think this is correct.

I'd love to find some way to completely avoid duplication of code at http://hg.mozilla.org/mozilla-central/annotate/343ec916dfd5/netwerk/protocol/http/nsHttpChannel.cpp#l249.

::: netwerk/cache/nsCacheService.cpp
@@ +1021,5 @@
>                                                        nsnull);
>  
>          // Don't delete the request if it was queued
> +        if (!(mRequest->IsBlocking() &&
> +            rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION))

May be more readable as:

!mRequest-IsBlocking() ||
rv != NS_ERROR_CACHE_WAIT_FOR_VALIDATION

@@ +1669,5 @@
> +                if (request->IsBlocking()) {
> +                    // async exits - validate, doom, or close will resume
> +                    return rv;
> +                }
> +            } else if (request->IsBlocking()) {

As I understand, a non-blocking request is intended to give up when the cache entry can't be opened (from whatever reason) and to cancel the |open| operation, right?  

However blocking ones are intended to always try to open the cache entry even on WAIT_FOR_VALIDATION (either synchronously through WaitForValidation() or asynchronously via callback), is that so?


I think for simplicity the condition may just be:
if (blocking) {
  if (listener) 
    return rv;

  WaitForValidation();
}

@@ +1777,5 @@
>          rv = gService->ProcessRequest(request, true, result);
>  
>          // delete requests that have completed
> +        if (!(listener && blockingMode &&
> +            (rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION)))

Perhaps the same here (! || ! || !)...

Non-blocking requests are done here according my previous comment, right?


Please also check whether to delete requests in nsCacheService::ProcessPendingRequests also when the request is non-blocking.  I think you should since non-blocking async requests may return NS_ERROR_CACHE_WAIT_FOR_VALIDATION.  I actually think the condition should be the same as are now here.

::: netwerk/cache/nsICacheSession.idl
@@ +81,5 @@
>       * available.
>       */
>      void asyncOpenCacheEntry(in ACString          key,
>                               in nsCacheAccessMode accessRequested,
> +                             in boolean           blockingMode,

Also update the documentation.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2255,5 @@
> +        accessRequested,
> +        !(mLoadFlags & LOAD_BYPASS_LOCAL_CACHE_IF_BUSY),
> +        this);
> +
> +    if (rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION) {

Can you ever get this error here regardless blocking or non-blocking request?  I think the state needed to determine this error result is processed only asynchronously on the cache thread, right?

Comment 9

[Honza Bambas (:mayhemer)]

Comment on attachment 601099 [details] [diff] [review]
patch 2 - removes OpenCacheEntry() in OpenOfflineCacheEntryForWriting()

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

If there were anything wrong with the changes then tests should catch it.  I don't want to stare in to the patch too long, it proved to be useless.

r=honzab


However, both reviews are conditioned by an as-much-as-possible-green try run (I'm going to push it right now).

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2258,5 @@
>          LOG(("bypassing local cache since it is busy\n"));
>          rv = NS_ERROR_NOT_AVAILABLE;
>      }
>  
> +    if (NS_SUCCEEDED(rv)) return NS_OK;

return on a new line please.

@@ +2349,5 @@
>          // access to the cache entry has been denied (because the cache entry
>          // is probably in use by another channel).  Either the cache is being
>          // read from (we're offline) or it's being updated elsewhere.
>          return NS_OK;
>      }

Can we ever get this result from AsyncOpenCacheEntry ?

@@ +2354,5 @@
>  
> +    if (NS_SUCCEEDED(rv))
> +        return NS_OK;
> +
> +    return rv;

Just return rv?

@@ +4890,5 @@
> +                return NS_OK;
> +            }
> +
> +            if (NS_FAILED(rv))
> +                return rv;

Could we potentially do this in parallel?  (In a follow-up, though a helper class probably)

Comment 10

[Honza Bambas (:mayhemer)]

https://tbpl.mozilla.org/?tree=Try&rev=f8097830c2c0

Comment 11

[Honza Bambas (:mayhemer)]

Comment on attachment 601099 [details] [diff] [review]
patch 2 - removes OpenCacheEntry() in OpenOfflineCacheEntryForWriting()

There is a consistent M(oth) tests failure.

Comment 12

[Honza Bambas (:mayhemer)]

https://tbpl.mozilla.org/php/getParsedLog.php?id=9799939&tree=Try

Comment 13

[Michal Novotny [:michal]]

Attachment: part1 v2

(In reply to Honza Bambas (:mayhemer) from comment #8)
> I'd love to find some way to completely avoid duplication of code at
> http://hg.mozilla.org/mozilla-central/annotate/343ec916dfd5/netwerk/protocol/
> http/nsHttpChannel.cpp#l249.

I agree, but I would prefer to do this in a follow-up bug.


> >          // Don't delete the request if it was queued
> > +        if (!(mRequest->IsBlocking() &&
> > +            rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION))
> 
> May be more readable as:
> 
> !mRequest-IsBlocking() ||
> rv != NS_ERROR_CACHE_WAIT_FOR_VALIDATION

I think that negation of conjunction is easier to understand than disjunction of negation. But if others have the same opinion as you, I can change it.


> As I understand, a non-blocking request is intended to give up when the
> cache entry can't be opened (from whatever reason) and to cancel the |open|
> operation, right?  
> 
> However blocking ones are intended to always try to open the cache entry
> even on WAIT_FOR_VALIDATION (either synchronously through
> WaitForValidation() or asynchronously via callback), is that so?

Correct.


> I think for simplicity the condition may just be:
> if (blocking) {
>   if (listener) 
>     return rv;
> 
>   WaitForValidation();
> }

Right, this is easier to read. Fixed.


> @@ +1777,5 @@
> >          rv = gService->ProcessRequest(request, true, result);
> >  
> >          // delete requests that have completed
> > +        if (!(listener && blockingMode &&
> > +            (rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION)))
> 
> Perhaps the same here (! || ! || !)...

The same as above...

 
> Non-blocking requests are done here according my previous comment, right?
> 
> 
> Please also check whether to delete requests in
> nsCacheService::ProcessPendingRequests also when the request is
> non-blocking.  I think you should since non-blocking async requests may
> return NS_ERROR_CACHE_WAIT_FOR_VALIDATION.  I actually think the condition
> should be the same as are now here.

I don't think so. We should never end up processing a non-blocking request in nsCacheService::ProcessPendingRequests() since it is never queued.


> ::: netwerk/cache/nsICacheSession.idl
> @@ +81,5 @@
> >       * available.
> >       */
> >      void asyncOpenCacheEntry(in ACString          key,
> >                               in nsCacheAccessMode accessRequested,
> > +                             in boolean           blockingMode,
> 
> Also update the documentation.

I've updated the description in idl and I'll update MDN after landing this patch.


> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +2255,5 @@
> > +        accessRequested,
> > +        !(mLoadFlags & LOAD_BYPASS_LOCAL_CACHE_IF_BUSY),
> > +        this);
> > +
> > +    if (rv == NS_ERROR_CACHE_WAIT_FOR_VALIDATION) {
> 
> Can you ever get this error here regardless blocking or non-blocking
> request?  I think the state needed to determine this error result is
> processed only asynchronously on the cache thread, right?

Right, AsyncOpenCacheEntry() never returns NS_ERROR_CACHE_WAIT_FOR_VALIDATION. Fixed.


> There is a consistent M(oth) tests failure.

Fixed.

Comment 14

[Michal Novotny [:michal]]

Attachment: part2 v2 - fixed according to comment #9

(In reply to Honza Bambas (:mayhemer) from comment #9)
> @@ +4890,5 @@
> > +                return NS_OK;
> > +            }
> > +
> > +            if (NS_FAILED(rv))
> > +                return rv;
> 
> Could we potentially do this in parallel?  (In a follow-up, though a helper
> class probably)

Do you mean to do OpenOfflineCacheEntryForWriting() and OpenNormalCacheEntry() in parallel? It should be possible, but it is definitely out of scope of this bug.

Comment 15

[Michal Novotny [:michal]]

pushed to try server

https://tbpl.mozilla.org/?tree=Try&rev=861d85480e00

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #13)
> I agree, but I would prefer to do this in a follow-up bug.

Definitely.  Can you please file it?

> I think that negation of conjunction is easier to understand than
> disjunction of negation. But if others have the same opinion as you, I can
> change it.

I perceive simpler to see that when just one condition doesn't apply and the code executes.  But up to you.

> I don't think so. We should never end up processing a non-blocking request
> in nsCacheService::ProcessPendingRequests() since it is never queued.

Then it's OK.  Maybe worth a comment?

(In reply to Michal Novotny (:michal) from comment #14)
> Do you mean to do OpenOfflineCacheEntryForWriting() and
> OpenNormalCacheEntry() in parallel? It should be possible, but it is
> definitely out of scope of this bug.

I don't think this is worth to fix, actually...  It may just have a minor affect on background offline cache loads.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 603215 [details] [diff] [review]
part1 v2

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

As I understand today discussion during the meeting, you will update the IDL ones more, right?  This patch looks good, but I think I should review the final one ones more.

So, preliminary r+, I'll take a quick look at the final patch.

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 603216 [details] [diff] [review]
part2 v2 - fixed according to comment #9

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

Looks good, r+, but the same as the previous patch.

Comment 19

[Michal Novotny [:michal]]

Attachment: part1 v3 - the new argument of the asyncOpenCacheEntry() is now optional

Comment 20

[Michal Novotny [:michal]]

Attachment: part2 v3

Comment 21

[Michal Novotny [:michal]]

Attachment: part3

This patch removes the last openCacheEntry() in nsHttpChannel that is used to find and invalidate an entry.

Comment 22

[Michal Novotny [:michal]]

pushed to tryserver: https://tbpl.mozilla.org/?tree=Try&rev=db932be8e043

Comment 23

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> (In reply to Michal Novotny (:michal) from comment #13)
> > I agree, but I would prefer to do this in a follow-up bug.
> 
> Definitely.  Can you please file it?

Bug #735538

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 605603 [details] [diff] [review]
part1 v3 - the new argument of the asyncOpenCacheEntry() is now optional

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

r=honzab

::: netwerk/cache/nsICacheSession.idl
@@ +77,5 @@
>  
>      /**
> +     * Asynchronous cache access. Does not block the calling thread. Instead,
> +     * the listener will be notified when the descriptor is available. If
> +     * 'noWait' is set to true, the listener will be notified immediately with

s/will be/is/ I think..

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 605604 [details] [diff] [review]
part2 v3

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

r=honzab

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 605605 [details] [diff] [review]
part3

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

r=honzab with comments addressed and after this passes try.

::: netwerk/cache/nsCacheService.cpp
@@ +1053,5 @@
> +        return NS_OK;
> +    }
> +
> +private:
> +    nsICacheListener *mListener;

nsCOMPtr, see more bellow.

@@ +1094,5 @@
> +        }
> +
> +        if (entry) {
> +            status = NS_OK;
> +            nsCacheService::gService->DoomEntry_Internal(entry, foundActive);

Can this fail?

@@ +1099,5 @@
> +        }
> +
> +        if (mListener) {
> +            mThread->Dispatch(new nsNotifyDoomListener(mListener, status),
> +                              NS_DISPATCH_NORMAL);

nsRefPtr for nsNotifyDoomListener object please.

@@ +1110,5 @@
> +
> +private:
> +    nsCString             mKey;
> +    nsCacheStoragePolicy  mStoragePolicy;
> +    nsICacheListener     *mListener;

We leak the listener when dispatch of this event fails.  This has to be nsCOMPtr.  You can swap it to nsNotifyDoomListener event with dont_AddRef and forget() this one.

@@ +1111,5 @@
> +private:
> +    nsCString             mKey;
> +    nsCacheStoragePolicy  mStoragePolicy;
> +    nsICacheListener     *mListener;
> +    nsCOMPtr<nsIThread>   mThread;

Would nsIEventTarget be enough here?

@@ +1442,5 @@
> +
> +    if (!gService->mInitialized)
> +        return NS_ERROR_NOT_INITIALIZED;
> +
> +    return DispatchToCacheIOThread(new nsDoomEvent(session, key, listener));

Please preserve this style of dispatching:

http://hg.mozilla.org/mozilla-central/annotate/78e56fd22f2a/netwerk/cache/nsCacheService.cpp#l1762

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5213,5 @@
>  
>      if (NS_FAILED(rv))
>          return;
>  
> +    session->DoomEntry(key, nsnull);

Shouldn't DoInvalidateCacheEntry be renamed to DoomCacheEntry or so?

::: netwerk/test/unit/test_doomentry.js
@@ +18,5 @@
> +  return _CSvc = Cc["@mozilla.org/network/cache-service;1"].
> +                 getService(Ci.nsICacheService);
> +}
> +
> +function get_ostream(key, asFile, append, callback)

Hint: objects should be UpperCamelCase to be easily recognized from functions.

@@ +156,5 @@
> +
> +function check_doom4(status)
> +{
> +  do_check_eq(status, Cr.NS_ERROR_NOT_AVAILABLE);
> +  do_test_finished();

Should you rather clear the cache here?

Comment 27

[Michal Novotny [:michal]]

Attachment: part3 v2

(In reply to Honza Bambas (:mayhemer) from comment #26)
> ::: netwerk/cache/nsCacheService.cpp
> > +private:
> > +    nsICacheListener *mListener;
> 
> nsCOMPtr, see more bellow.
> 
>
> > +    nsICacheListener     *mListener;
> 
> We leak the listener when dispatch of this event fails.  This has to be
> nsCOMPtr.  You can swap it to nsNotifyDoomListener event with dont_AddRef
> and forget() this one.

The leak in case of failure is intentional since we need to release the listener on the callers thread. If we can't post the event to the correct thread how we could release the reference on the same thread? BTW we do the same in nsCacheService::NotifyListener().


> @@ +1111,5 @@
> > +private:
> > +    nsCString             mKey;
> > +    nsCacheStoragePolicy  mStoragePolicy;
> > +    nsICacheListener     *mListener;
> > +    nsCOMPtr<nsIThread>   mThread;
> 
> Would nsIEventTarget be enough here?

It would, but what's the benefit? do_GetCurrentThread returns already_AddRefed<nsIThread>.


> > +            mThread->Dispatch(new nsNotifyDoomListener(mListener, status),
> > +                              NS_DISPATCH_NORMAL);
> 
> nsRefPtr for nsNotifyDoomListener object please.
> 
>
> > +    return DispatchToCacheIOThread(new nsDoomEvent(session, key, listener));
> 
> Please preserve this style of dispatching:
> 
> http://hg.mozilla.org/mozilla-central/annotate/78e56fd22f2a/netwerk/cache/
> nsCacheService.cpp#l1762

It's actually a relict from times when new could fail. And this style is also present in the cache code, e.g. http://hg.mozilla.org/mozilla-central/annotate/78e56fd22f2a/netwerk/cache/nsDiskCacheDevice.cpp#l1141.


> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +5213,5 @@
> >  
> >      if (NS_FAILED(rv))
> >          return;
> >  
> > +    session->DoomEntry(key, nsnull);
> 
> Shouldn't DoInvalidateCacheEntry be renamed to DoomCacheEntry or so?

I don't think it's necessary. Both dooming the entry as well as setting expiration time to 0 do invalidate the entry.


> ::: netwerk/test/unit/test_doomentry.js
> @@ +18,5 @@
> > +  return _CSvc = Cc["@mozilla.org/network/cache-service;1"].
> > +                 getService(Ci.nsICacheService);
> > +}
> > +
> > +function get_ostream(key, asFile, append, callback)
> 
> Hint: objects should be UpperCamelCase to be easily recognized from
> functions.

Renamed.


> @@ +156,5 @@
> > +
> > +function check_doom4(status)
> > +{
> > +  do_check_eq(status, Cr.NS_ERROR_NOT_AVAILABLE);
> > +  do_test_finished();
> 
> Should you rather clear the cache here?

A common rule is to clean the cache before running the test rather than rely on cleanup performed by previous test.

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #27)
> The leak in case of failure is intentional ... BTW we do the
> same in nsCacheService::NotifyListener().

Ah, I wasn't aware.  Maybe just add a comment.


I also agree with the rest.  Thanks.

Comment 29

[Michal Novotny [:michal]]

Attachment: part3 v3

(In reply to Honza Bambas (:mayhemer) from comment #26)
> > +        if (entry) {
> > +            status = NS_OK;
> > +            nsCacheService::gService->DoomEntry_Internal(entry, foundActive);
> 
> Can this fail?

I forgot to answer this question. DoomEntry_Internal() returns always NS_OK and I don't think that we'll change this behavior in future.

(In reply to Honza Bambas (:mayhemer) from comment #28)
> (In reply to Michal Novotny (:michal) from comment #27)
> > The leak in case of failure is intentional ... BTW we do the
> > same in nsCacheService::NotifyListener().
> 
> Ah, I wasn't aware.  Maybe just add a comment.

Comment added.

Pushed to tryserver again because of a strange failure in the last try:
https://tbpl.mozilla.org/?tree=Try&rev=5932f13b5eb4

Comment 30

[Michal Novotny [:michal]]

Attachment: part1 v4 - fixed typo

not sure if this need a sr

Comment 31

[Michal Novotny [:michal]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/6c8d1da1e1db
http://hg.mozilla.org/integration/mozilla-inbound/rev/5b61e3d75735
http://hg.mozilla.org/integration/mozilla-inbound/rev/835ac64631ec

Comment 32

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/6c8d1da1e1db
https://hg.mozilla.org/mozilla-central/rev/5b61e3d75735
https://hg.mozilla.org/mozilla-central/rev/835ac64631ec

Comment 33

[Florian Quèze [:florian]]

Fix for the comm-central bustage: http://hg.mozilla.org/comm-central/rev/d2f3b160dd8b

David (cc'ed) may want to review this change after the fact.