746018 - Read from the cache while we do revalidation on the assumption revalidation will succeed

Status: RESOLVED DUPLICATE of bug 1013587

(Core :: Networking: HTTP, defect)

Description

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

Right now, we do the following for a given HTTP request, serially:

1. Search for an entry in the cache
2. If we find an entry that needs to be revalidated, then revalidate it.
3. If the revalidation succeeded, then read the cached response from the cache.

Instead, we should assume that revalidation will succeed, and start reading from the cache at the same time we send out the conditional request. Our telemetry data shows that our revalidation success rate is very high.

Comment 1

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

Attachment: Ensure that some properties of the channel are not modified after the channel has been opened

Jason, this is a very simple patch that helps document and enforce that these properties are expected to be set before calling AsyncOpen and not modified afterward. It will help make the main patch for this bug a little easier to understand and it is a prerequisite for bug 722034.

Comment 2

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

Attachment: Split nsHttpChannel::Connect into two parts (Connect and ContinueConnect)

This patch simply splits Connect() into two functions, Connect() and ContinueConnect. Strictly, it isn't a prerequisite for the main patch for this bug, but it is a prerequisite for bug 722034 like the main patch is, and I don't want to reorder my patch queue.

This looks like a bigger change than it is, because of the whitespace-only change of adjusting the indention in Connect(). I will attach a diff -uw to help the review.

Comment 3

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

Attachment: diff -uw version of patch 615561 [NOT FOR CHECKIN]

Comment 4

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

Attachment: diff -uw version of patch 615561 [NOT FOR CHECKIN]

Comment 5

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

Attachment: Do more logging in nsCacheEntryDescriptor::nsInputStreamWrapper

This patch simply adds more logging into nsCacheEntryDescriptor::
 nsInputStreamWrapper, which is useful for verifying the correct behavior of the main patch.

Comment 6

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

Try run for verifying that my touch-ups to the main patch did break anything:

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

Comment 7

[Jason Duell]

Comment on attachment 615560 [details] [diff] [review]
Ensure that some properties of the channel are not modified after the channel has been opened

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

I'd actually like honza to verify that we're good with adding these checks--I don't know the appcache semantics well enough to say.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 615561 [details] [diff] [review]
Split nsHttpChannel::Connect into two parts (Connect and ContinueConnect)

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

Thanks for the -uw patch!

r=honzab.

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #6)
> Try run for verifying that my touch-ups to the main patch did break anything:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=31964bc716c5

What's this try run?  I don't see patches in this bug being tested with it.

Comment 10

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

That try run is for the patches that actually fix the bug. The currently-attached patches are just the prerequisites.

Comment 11

[Honza Bambas (:mayhemer)]

The "Ensure that some properties..." should go though try it self.  I don't see this patch in the series... or is it there?

Comment 12

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

Attachment: More cache logging

It is helpful to have even more logging within the cache to observe this patch working.

Comment 13

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

Attachment: Start reading cache entries before revalidating them

I will use Splinter to add some additional commentary on the patch that may be helpful for the review.

Comment 14

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

Comment on attachment 615928 [details] [diff] [review]
Start reading cache entries before revalidating them

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +89,5 @@
>  static NS_DEFINE_CID(kStreamListenerTeeCID, NS_STREAMLISTENERTEE_CID);
>  
> +} // unnamed namespace
> +
> +namespace mozilla { namespace net {

It isn't necessary to put this in mozilla::net, especially for this patch, but looking forward to the patches for bug 722034, it is helpful to do so.

@@ +123,5 @@
>  
> +void
> +MaybeMarkCacheEntryValid(const void * channel,
> +                         nsICacheEntryDescriptor * cacheEntry,
> +                         nsCacheAccessMode cacheAccess)

It is useful to factor this out now, again more for bug 722034 than for this bug.

@@ +2712,5 @@
>          (mLoadedFromApplicationCache ||
>           (mCacheAccess == nsICache::ACCESS_READ &&
>            !(mLoadFlags & INHIBIT_CACHING)) ||
>           mFallbackChannel)) {
> +        rv = StartBufferingCachedEntity(isCachedRedirect, usingSSL);

We must start buffering the entry here and anywhere else where we set mCachedContentIsValid = true or mCachedContentIsValid = true.

@@ +2758,5 @@
> +                    if (NS_SUCCEEDED(rv)) {
> +                        rv = StartBufferingCachedEntity(isCachedRedirect,
> +                                                        usingSSL);
> +                    }
> +                    mCachedContentIsPartial = NS_SUCCEEDED(rv);

ditto.

@@ +3035,3 @@
>  nsresult
> +nsHttpChannel::StartBufferingCachedEntity(bool isCachedRedirect,
> +                                          bool usingSSL)

The idea here is that StartBufferingCachedEntity will make all of the access to the cache entry descriptor, so that when we move StartBufferingCachedEntity to a background thread, all those accesses happen on the background thread. For example, we retrieve the security info *now* because that requires accessing mCacheEntry.

@@ +3075,5 @@
> +        // Open an input stream for the entity, but DO NOT create/connect
> +        // mCachePump; that is done only when we decide to actually read the
> +        // cached entity. By opening the input stream here, we allow the stream
> +        // transport service to start reading the entity on one of its
> +        // background threads while we do validation (if any).

The code in this block is what kicks off the pre-fetching of the cache content. The logic is actually copy/pasted/modified from (!mAsyncStream) case of nsInputStreamPump::AsyncRead.

One alternative design would be to connect mCachePump now and Suspend() it, then Resume() it in ReadFromCache(). However, nsInputStreamPump is not thread safe, and in bug 722034 we will move this code to the cache service thread. So, we do it this way. Note that I did not attempt to tune any part of this; I use the same parameters as nsInputStreamPump. I suspect that we will be able to get better performance by doing some tuning here; we should do that in another bug.

You can verify that the cache data is actually read before we kick of revalidation, by adding a PR_Sleep(5) or so immediately after the call to CheckCache(), and running a test like test_bug669001.js. You will see something like this:

0[a3f8f0]: Opened cache input stream [channel=3f6ca40, wrapper=2d0cd70, transport=4007388, stream=3ffb350]
4356[3fdf670]: nsInputStreamWrapper::LazyInit [entry=400ab90, wrapper=3ffb350, mInput=4003c20, rv=0]
4356[3fdf670]: nsInputStreamWrapper::Read [entry=400ab90, wrapper=3ffb350, mInput=4003c20, rv=0]
4356[3fdf670]: nsInputStreamWrapper::Read [entry=400ab90, wrapper=3ffb350, mInput=4003c20, rv=0]
0[a3f8f0]: ProcessPendingRequests for initialized  Valid entry 3fa8c80
0[a3f8f0]: Marking cache entry valid [channel=3f6ca40, entry=400ab90, access=3, result=0]
0[a3f8f0]: nsHTTPChannel::CheckCache exit [this=3f6ca40 doValidation=0]

The fact that nsInputStreamWrapper::Read is being called before CheckCache() even returns is sign that the read is happening on the stream transport thread in the background.

Comment 15

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

(In reply to Honza Bambas (:mayhemer) from comment #11)
> The "Ensure that some properties..." should go though try it self.  I don't
> see this patch in the series... or is it there?

Here is a try run just for that:
https://tbpl.mozilla.org/?tree=Try&rev=e86d707a218c

and here is the try run for all the patches in this bug:
https://tbpl.mozilla.org/?tree=Try&rev=07eee97362e6

and, for comparison purposes, here is the try run for bug 722034:
https://tbpl.mozilla.org/?tree=Try&rev=b3c952a1d018

and, again for comparison purposes, here is a run without any of these patches:
https://hg.mozilla.org/try/rev/ef7cf6591031

Comment 16

[Jason Duell]

Comment on attachment 615926 [details] [diff] [review]
More cache logging

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

::: netwerk/cache/nsCacheEntryDescriptor.cpp
@@ +575,5 @@
>                                                   mStartOffset,
>                                                   getter_AddRefs(mInput));
>  
>      CACHE_LOG_DEBUG(("nsInputStreamWrapper::LazyInit "
> +                      "[entry=%p, wrapper=%p, mInput=%p, rv=%d]",

whitespace only change on this line, or did I miss something?

Comment 17

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

Comment on attachment 615926 [details] [diff] [review]
More cache logging

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

::: netwerk/cache/nsCacheEntryDescriptor.cpp
@@ +575,5 @@
>                                                   mStartOffset,
>                                                   getter_AddRefs(mInput));
>  
>      CACHE_LOG_DEBUG(("nsInputStreamWrapper::LazyInit "
> +                      "[entry=%p, wrapper=%p, mInput=%p, rv=%d]",

I added a "]" after rv=%d

Comment 18

[Honza Bambas (:mayhemer)]

Comment on attachment 615560 [details] [diff] [review]
Ensure that some properties of the channel are not modified after the channel has been opened

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

r=honzab

Comment 19

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

Attachment: Part 1 - Ensure that some properties of the channel are not modified after the channel has been opened [v2]

renamed/rebased version of patch Honza already r+d.

Comment 20

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

Attachment: Part 2 - Split nsHttpChannel::Connect into two parts (Connect and ContinueConnect)

renamed/rebased version of patch 615561 r+d by honzab

Comment 21

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

Attachment: Part 3 - Do more logging in nsCacheEntryDescriptor::nsInputStreamWrapper

Renamed/rebased version of patch 615566 r+d by jduell.

Comment 22

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

Attachment: Part 4 - More cache logging

Renamed/rebased version of patch 615926 r+d by jduell.

Comment 23

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

Attachment: Part 5 - Start buffering cache entries in memory before we validate them

I am re-running this through try now:
https://tbpl.mozilla.org/?tree=Try&rev=61f868a6a8c4

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 624137 [details] [diff] [review]
Part 5 - Start buffering cache entries in memory before we validate them

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2734,5 @@
>      rv = mCachedResponseHead->Parse((char *) buf.get());
>      NS_ENSURE_SUCCESS(rv, rv);
>      buf.Adopt(0);
>  
> +    bool isCachedRedirect = mCachedResponseHead->Status()/100 == 3;

Status() / 100

@@ +2791,5 @@
> +                    if (NS_SUCCEEDED(rv)) {
> +                        rv = StartBufferingCachedEntity(isCachedRedirect,
> +                                                        usingSSL);
> +                    }
> +                    mCachedContentIsPartial = NS_SUCCEEDED(rv);

You should drop the If-Range header when rv is a failure.  Try to be symmetric with the new code bellow for other If- headers.

@@ +2972,5 @@
> +    // then start reading it into memory now. If we have to revalidate the
> +    // entry and the revalidation fails, this will be a wasted effort, but
> +    // it is much more likely that either we don't need to revalidate the entry
> +    // or the entry will successfully revalidate.
> +    if ((mCachedContentIsValid || mCustomConditionalRequest || mDidReval)) {

You probably don't need double parens.

Don't do this for mCustomConditionalRequest.  We don't present the body on 304 then, we just report the 304 response directly.

@@ +2996,2 @@
>      LOG(("nsHTTPChannel::CheckCache exit [this=%p doValidation=%d]\n", this, doValidation));
> +    return rv;

Why?

@@ +3078,5 @@
> +            NS_WARNING("failed to parse security-info");
> +            return rv;
> +        }
> +        if (!mCachedSecurityInfo) {
> +            return NS_ERROR_UNEXPECTED; // XXX error code

LOG ?

@@ +3085,5 @@
> +
> +    nsresult rv = NS_OK;
> +    bool shouldUpdateOffline;
> +
> +    if (isCachedRedirect) {

This doesn't include Peek() for "Location" header in the response.

@@ +3138,5 @@
> +                  "transport=%p, stream=%p]", this, wrapper.get(),
> +                  transport.get(), stream.get()));
> +        } else {
> +            LOG(("Failed to open cache input stream [channel=%p, "
> +                  "wrapper=%p, transport=%p, stream=%p", this,

Close the brace

@@ +3190,5 @@
> +
> +    // TODO: We should be able to make it safe to call mCacheAsyncFunc
> +    // synchronously here, since ReadFromCache() is only called by callbacks.
> +    if (mCacheAsyncFunc)
> +        return AsyncCall(mCacheAsyncFunc);

Drop mCacheAsyncFunc (set back to null).

@@ +3209,4 @@
>                                     true);
> +    if (NS_FAILED(rv)) {
> +        inputStream->Close();
> +        return rv;

It was a bug that we didn't close the stream here?

::: netwerk/protocol/http/nsHttpChannel.h
@@ +74,5 @@
> +// Like an nsAutoPtr for XPCOM streams (e.g. nsIAsyncInputStream) and other
> +// refcounted classes that need to have the Close() method called explicitly
> +// before they are destroyed.
> +template <typename T>
> +class AutoClose

Move this class to a separate file somewhere under netwerk/base please.

@@ +106,5 @@
> +        Close();
> +        mPtr = rhs;
> +    }
> +
> +    void CloseAndRelease()

Maybe in tradition of other methods use |closeAndRelease| - lower camel case.

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 624137 [details] [diff] [review]
Part 5 - Start buffering cache entries in memory before we validate them

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2687,5 @@
>      nsresult rv = NS_OK;
>  
> +    bool usingSSL = false;
> +    rv = mURI->SchemeIs("https", &usingSSL);
> +    NS_ENSURE_SUCCESS(rv,rv);

BTW: why is not mConnectionInfo->UsingSSL() used here?

Consider pending r? until answered.

Comment 26

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

Comment on attachment 624137 [details] [diff] [review]
Part 5 - Start buffering cache entries in memory before we validate them

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2687,5 @@
>      nsresult rv = NS_OK;
>  
> +    bool usingSSL = false;
> +    rv = mURI->SchemeIs("https", &usingSSL);
> +    NS_ENSURE_SUCCESS(rv,rv);

When we move CheckCache() to HttpCacheQuery in the other bug, we will not have access to mConnectionInfo because HttpCacheQuery doesn't have an mConnectionInfo member. I tried to add as few members to HttpCacheQuery as I could.

Also, this is the same logic used in Connect().

Comment 27

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #26)
> When we move CheckCache() to HttpCacheQuery in the other bug, we will not
> have access to mConnectionInfo because HttpCacheQuery doesn't have an
> mConnectionInfo member. I tried to add as few members to HttpCacheQuery as I
> could.
> 
> Also, this is the same logic used in Connect().

OK, just checking on "https" seems odd to me.  Leave it as is then.

Comment 28

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

Attachment: Part 5 - Factor out nsHttpChannel::ShouldUpdateOfflineCacheEntry into a standalone function

(In reply to Honza Bambas (:mayhemer) from bug 722034 comment #44)
> Comment on attachment 624132 [details] [diff] [review]
> Part 5 - Factor out nsHttpChannel::ShouldUpdateOfflineCacheEntry into a
> standalone function
> 
> Review of attachment 624132 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Shouldn't this be part of the patch for bug 746018?  I now can see that
> ShouldUpdateOfflineCacheEntry works with a bad mResponseHead when called
> from StartBufferingCachedEntity()

Yes. I moved it to this bug now.

> So, when there is no mOfflineCacheEntry, you pass 0, that probably passes if
> (docLastModifiedTime > offlineLastModifiedTime) and returns true.  But the
> original implementation returns false for case of no mOfflineCacheEntry.

Now, I only call ShouldUpdateOfflineCacheEntry when there is an offline cache entry.

Comment 29

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

Attachment: Part 6 - Start buffering cache entries in memory before we validate them [v2], r?honzab

Honza, please check out the interdiff of this and the previous version of the patch. I addressed your review comments and added some additional comments to clarify things.

Here is the try run:
https://tbpl.mozilla.org/?tree=Try&rev=5aec4f1b0167

Note that I'm experimenting with the Mercurial bookmarks feature and that makes the tbpl output look like there's a bunch of unrelated patches here. Look at the graph to see that this is just this series of patches:
https://hg.mozilla.org/try/graph/116940

Comment 30

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

Comment on attachment 627631 [details] [diff] [review]
Part 5 - Factor out nsHttpChannel::ShouldUpdateOfflineCacheEntry into a standalone function

Dropping review requests as I may significantly update this patch again due to a failing test caused by one of the patches to bug 722034.

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 627633 [details] [diff] [review]
Part 6 - Start buffering cache entries in memory before we validate them [v2], r?honzab

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

r=honzab with some comments left.

Otherwise looks good.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2782,5 @@
> +                    mCachedContentIsPartial = NS_SUCCEEDED(rv);
> +                    if (!mCachedContentIsPartial) {
> +                      // Make the request unconditional again.
> +                      mRequestHead.ClearHeader(nsHttp::Range);
> +                      mRequestHead.ClearHeader(nsHttp::If_Range);

Indention

@@ +3040,5 @@
> +
> +    nsresult rv = NS_OK;
> +    PRUint32 offlineLastModified;
> +
> +    if (isCachedRedirect) {

This still doesn't include check for the Location header presence.

See http://hg.mozilla.org/mozilla-central/annotate/79262a88881d/netwerk/protocol/http/nsHttpChannel.cpp#l2998

@@ +3114,5 @@
> +// and/or StartBufferingCachedEntity.
> +nsresult
> +nsHttpChannel::ReadFromCache(bool alreadyMarkedValid)
> +{
> +    MOZ_ASSERT(!mCustomConditionalRequest);

Hmm... apparently we may load the cached content even this gets set.  It clearly happens in case the cached content is considered valid w/o checking with the server (that makes your preload happen, btw).

I checked this assertion fails with /tests/content/base/test/test_bug475156.html on a clean unpatched m-c build too.

I don't think this assertion is valid.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +38,5 @@
> +// Like an nsAutoPtr for XPCOM streams (e.g. nsIAsyncInputStream) and other
> +// refcounted classes that need to have the Close() method called explicitly
> +// before they are destroyed.
> +template <typename T>
> +class AutoClose

Please really move this to some more general file.

@@ +70,5 @@
> +        Close();
> +        mPtr = rhs;
> +    }
> +
> +    void CloseAndRelease()

closeAndRelease

Comment 32

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

Attachment: Simplify ShouldUpdateOfflineCacheEntry

I found some unfixable bugs with my previous approach to this patch, so I basically reverted the logic to be the same. So, strictly, this patch isn't required for this bug. However, I built all my patches for bug 722034 on it and it is a good change as it makes the code more readable.

ALSO, there is a logic change: Previously, we would not update the offline cache entry if the entry in the offline cache did not have a Last-Modified date but the new version (from the cache or network) does. Now, we will update the offline cache entry in that case.

Comment 33

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

Attachment: Start buffering cache entries in memory before we validate them

Honza, previously I was trying to call ShouldUpdateOfflineCacheEntry in StartBufferingCachedEntity. But, in future patches for bug 722034, we don't have the information we need to call ShouldUpdateOfflineCacheEntry during StartBufferingCachedEntity--in particular, we won't have the last-modified time of the offline cache entry. Consequently, I had to make a small but significant change to this patch in StartBufferingCachedEntity and ReadCache. I believe these changes also take care of the issue with mCustomConditionalRequest.

I moved the AutoClose class to netwerk/base, but I did not rename CloseAndRelease to closeAndRelease, because CloseAndRelease calls Close() (capital C) and Release.

Comment 34

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

Here is the try run with this set of patches:
https://tbpl.mozilla.org/?tree=Try&rev=dd2690a6160e

Comment 35

[Honza Bambas (:mayhemer)]

Comment on attachment 628421 [details] [diff] [review]
Simplify ShouldUpdateOfflineCacheEntry

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2940,3 @@
>      PRUint32 offlineLastModifiedTime;
>      rv = mOfflineCacheEntry->GetLastModified(&offlineLastModifiedTime);
> +    NS_ENSURE_SUCCESS(rv, true);

if (NS_FAILED(rv)) return true; since you don't consider this as fatal error.

Comment 36

[Honza Bambas (:mayhemer)]

Comment on attachment 628422 [details] [diff] [review]
Start buffering cache entries in memory before we validate them

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

How the new file gets propagated to any include location?  IMO it should be in net/base/pub and properly exported in its Makefile.in via EXPORTS list.

Looks good, however, I didn't test it my self and didn't check the logic extremely deeply.

r=honzab

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2656,5 @@
>      nsresult rv = NS_OK;
>  
> +    bool usingSSL = false;
> +    rv = mURI->SchemeIs("https", &usingSSL);
> +    NS_ENSURE_SUCCESS(rv,rv);

mConnectionInfo->UsingSSL() please.  Or any major reason why not to?

@@ +3184,5 @@
> +        // to avoid event dispatching latency.
> +        MOZ_ASSERT(!mCacheAsyncInputStream);
> +        LOG(("Skipping skip read of cached redirect entity\n"));
> +        rv = AsyncCall(&nsHttpChannel::HandleAsyncRedirect);
> +        return rv;

Maybe return directly..

@@ +3209,5 @@
> +            return rv;
> +        }
> +    }
> +
> +    MOZ_ASSERT(mCacheAsyncInputStream);

What happens in release build when this is null?  Please add a null check here with NS_ERROR, maybe accompanied with LOG().

Comment 37

[Honza Bambas (:mayhemer)]

Comment on attachment 628421 [details] [diff] [review]
Simplify ShouldUpdateOfflineCacheEntry

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2940,3 @@
>      PRUint32 offlineLastModifiedTime;
>      rv = mOfflineCacheEntry->GetLastModified(&offlineLastModifiedTime);
> +    NS_ENSURE_SUCCESS(rv, true);

Err... this should be if NS_FAILED(rv) return false; !

Comment 38

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

(In reply to Honza Bambas (:mayhemer) from comment #36)
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +2656,5 @@
> >      nsresult rv = NS_OK;
> >  
> > +    bool usingSSL = false;
> > +    rv = mURI->SchemeIs("https", &usingSSL);
> > +    NS_ENSURE_SUCCESS(rv,rv);
> 
> mConnectionInfo->UsingSSL() please.  Or any major reason why not to?

Please see bug 746018 comment 27. Let's clean this up in nsHttpChannel in general in a follow-up bug. I filed bug 760232 for that.

(In reply to Honza Bambas (:mayhemer) from comment #37)
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +2940,3 @@
> >      PRUint32 offlineLastModifiedTime;
> >      rv = mOfflineCacheEntry->GetLastModified(&offlineLastModifiedTime);
> > +    NS_ENSURE_SUCCESS(rv, true);
> 
> Err... this should be if NS_FAILED(rv) return false; !

To match the existing code, it should be |if NS_FAILED(rv) return false;| But, is that actually the correct logic? If the new response has a Last-Modified value, but the offline cache entry did not have a Last-Modified value, shouldn't we update the offline cache?

Comment 39

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

(In reply to Honza Bambas (:mayhemer) from comment #36)
> How the new file gets propagated to any include location?  IMO it should be
> in net/base/pub and properly exported in its Makefile.in via EXPORTS list.

I do not think we should export this file; it is a pretty specialized class that needs to be used carefully. That is why I originally put it only in nsHttpChannel.h. Since the build works currently, let's not make any changes here.

Comment 40

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

(In reply to Brian Smith (:bsmith) from comment #38)
> (In reply to Honza Bambas (:mayhemer) from comment #37)
> > ::: netwerk/protocol/http/nsHttpChannel.cpp
> > @@ +2940,3 @@
> > >      PRUint32 offlineLastModifiedTime;
> > >      rv = mOfflineCacheEntry->GetLastModified(&offlineLastModifiedTime);
> > > +    NS_ENSURE_SUCCESS(rv, true);
> > 
> > Err... this should be if NS_FAILED(rv) return false; !
> 
> To match the existing code, it should be |if NS_FAILED(rv) return false;|
> But, is that actually the correct logic? If the new response has a
> Last-Modified value, but the offline cache entry did not have a
> Last-Modified value, shouldn't we update the offline cache?

For now, I took Honza's suggestion and made it match the existing logic. I filed bug 760239 to change the logic.

Comment 41

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

https://hg.mozilla.org/mozilla-central/rev/e9edae96ea91
https://hg.mozilla.org/mozilla-central/rev/d4eacb5c2151
https://hg.mozilla.org/mozilla-central/rev/aa61440b072f
https://hg.mozilla.org/mozilla-central/rev/d13cd8ffde89
https://hg.mozilla.org/mozilla-central/rev/0f3f9ff4439d
https://hg.mozilla.org/mozilla-central/rev/301d0d6bf24e

Comment 42

[[e-mail]]

hg blames this bug for the following code block, although I can't figure out which patch it's from:

MOZ_ASSERT(mCacheAsyncInputStream);
if (!mCacheAsyncInputStream) {
    NS_ERROR("mCacheAsyncInputStream is null but we're expecting to "
                    "be able to read from it.");
    return NS_ERROR_UNEXPECTED;
}

So, how expected is this, if you assert it, and then error-check it?

Comment 43

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

(In reply to [e-mail] from comment #42)
> MOZ_ASSERT(mCacheAsyncInputStream);
> if (!mCacheAsyncInputStream) {
>     NS_ERROR("mCacheAsyncInputStream is null but we're expecting to "
>                     "be able to read from it.");
>     return NS_ERROR_UNEXPECTED;
> }
> 
> So, how expected is this, if you assert it, and then error-check it?

It's defensive programming. I'd rather not do it that way, but in this case it turned out to be a good idea, I guess.

Anyway, the main part of this optimization got undone as part of fixing bug 764171, so we're no longer pre-buffering while we validate the cached response with the server. I am going to mark the patches above obsolete because they may not all exactly match the changesets checked in in comment 41. Note that NO changesets have been backed out.

In order to properly implement this, the cache must be changed to allow us to invalidate an input stream and to allow us to open an output stream for a cache entry while that an input stream is still open.

Comment 44

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

I asked for approval to back this out of aurora completely as part of attachment 638894 [details] [diff] [review] of bug 722034.

Comment 45

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

(In reply to Brian Smith (:bsmith) from comment #44)
> I asked for approval to back this out of aurora completely as part of
> attachment 638894 [details] [diff] [review] of bug 722034.

Backed out of mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/c576c2b8d9ba
https://hg.mozilla.org/releases/mozilla-aurora/rev/deec51854e4e

Comment 46

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

My hint for whoever works on this:

My old attempt at implementing this optimization failed because it tried to do everything in nsHttpChannel without any changes to the cache. This didn't work because the cache could not handle reading from an entry's input stream on one thread while trying to open the output stream on another thread. In particular, memory cache entries are just plain thread-unsafe and disk cache entries are only thread-safe because they have specific code to make this fail. That really needs to be fixed within the cache itself instead of worked around at the nsHttpChannel level.

My original implementation of this was based on wrong telemetry data, which supported the idea what we should get a net win with a very simple greedy approach of just always assuming revalidation will succeed. However, revalidation fails about 50% of the time. I bet we could reduce the cost of revalidation failures with this optimization substantially by being just a little bit smarter than I was:

* When a revalidation fails, save a "don't pre-buffer" bit in the cache entry's metadata
* When revalidation succeeds remove the "don't pre-buffer" bit.
* Only do the pre-buffering in CheckCache when the "don't pre-buffer" bit is not present in the cache entry's metadata.

However, I am not sure if would be a net win or not so we'd have to measure it.