482934 - Response to a non-necko-initiated conditional request should be cached

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

Currently, if the user of Necko requests a conditional request (by setting a header like If-Modified-Since), we don't cache the result.

We ought to cache the response to these requests though, if it's a 200 response. That would allow web pages to trigger a cache update even if Firefox considers the resource to be fresh.

Comment 1

[Michal Novotny [:michal]]

Attachment: patch v1

When user makes a conditional request the cache entry is opened normally but any reading is avoided even if it is opened in READ_WRITE mode. The patch also fixes unittest test_bug331825.js which seems to be broken. It passes successfully when the fix for bug 331825 is reverted (i.e. line http://hg.mozilla.org/mozilla-central/annotate/8a50fbd3288e/netwerk/protocol/http/src/nsHttpChannel.cpp#l1576 is commented out). The problem is that there is no valid cache entry. I've added the code to store cache entry and I've also added a check that the cache entry wasn't removed.

Comment 2

[Patrick McManus [:mcmanus]]

is there a reason this r+d patch wasn't landed? Should the bug be otherwise resolved?

Comment 3

[Honza Bambas (:mayhemer)]

The patch needs to be reevaluated.  Cache 2 integration to http channel made too many changes.

Comment 4

[Michal Novotny [:michal]]

Junior will have a look at it. This might be a good first bug to get in touch with the cache code.

Comment 5

[Junior [inactive]]

I tested with a local apache server and it's good in Nightly.
Do you think if we need to have a test to protect our code, michal?

Comment 6

[Michal Novotny [:michal]]

Yes please, take test_bug482934.js and change it so it uses new cache API.

Comment 7

[Junior [inactive]]

Attachment: test_v1_WIP

a WIP patch
base on discussion, will change the first and third HTTP requests by cache2 API

Comment 8

[Junior [inactive]]

Attachment: test_v2_WIP

I spent a whole afternoon to find out we can't use |OpenCallback| to check the cache for a HTTP request since we don't set the "meto" metadata. Instead I read the entry directly.

So here's the todo list:
1. Use cache2 API like [1] to cache old data instead of using HTTP request:

[1] http://searchfox.org/mozilla-central/rev/62db1c9021cfbde9fa5e6e9601de16c21f4c7ce4/netwerk/test/unit/test_bug482601.js#87

2. Use only one httpd path handler

Do the patch and todo list make sense, michal?
Thanks.

Comment 9

[Michal Novotny [:michal]]

Comment on attachment 8812118 [details] [diff] [review]
test_v2_WIP

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

(In reply to Junior [:junior] from comment #8)
> So here's the todo list:
> 1. Use cache2 API like [1] to cache old data instead of using HTTP request:
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 62db1c9021cfbde9fa5e6e9601de16c21f4c7ce4/netwerk/test/unit/test_bug482601.
> js#87
> 
> 2. Use only one httpd path handler
> 
> Do the patch and todo list make sense, michal?

Yes

::: netwerk/test/unit/test_bug482934.js
@@ +72,5 @@
> +
> +  httpserver.registerPathHandler(resource, resource_old_handler);
> +  cache = getCacheStorage("disk");
> +
> +  wait_for_cache_index(run_next_test);

You don't need to wait for the index in this test.

@@ +92,5 @@
> +});
> +
> +add_test(() => {
> +  // A 200 OK with new data after revalidation
> +  var ch = make_channel(resource_url, "no-cache");

Setting "Cache-control: no-cache" doesn't make sense here. I guess it is a leftover from test_cache-control_request.js which you took as an example. Make sure to set correct If-Modified-Since header and also to test it in the handler.

@@ +124,5 @@
> +
> +// ============================================================================
> +// Helpers
> +
> +function date_string_from_now(delta_secs) {

I don't think you have to calculate Last-Modified value from the current date. A fixed date should work.

Comment 10

[Junior [inactive]]

Attachment: test_v3_WIP

Actually if I removed "cache-control", the test would fail to cache the content to a conditional request.

However, I can not reproduce in beta use local apache.
My steps:
1. load a file like localhost/cache.html from local server. The response contains Last-Modified header; and does not contains ETag and Cache-Control.
2. wait for several seconds, press enter in address bar again.
(I don't use refresh since I want to avoid sending cache-control)
3. check the "about:cache" to see if the data is updated

I can not check the nightly and aurora by using about:cache since bug 1318664. 

I'll update my code and use asyncVisitStorage to check what happened.

Hello michal,
Could I have your feedback to check if the test is what we want?
Thanks.

Comment 11

[Michal Novotny [:michal]]

(In reply to Junior [:junior] from comment #10)
> Actually if I removed "cache-control", the test would fail to cache the
> content to a conditional request.

The test looks good and the functionality is obviously broken. The test doesn't pass if I add "cache-control: no-cache" header.

> However, I can not reproduce in beta use local apache.
> My steps:
> 1. load a file like localhost/cache.html from local server. The response
> contains Last-Modified header; and does not contains ETag and Cache-Control.
> 2. wait for several seconds, press enter in address bar again.
> (I don't use refresh since I want to avoid sending cache-control)
> 3. check the "about:cache" to see if the data is updated

This manual test doesn't do the most important step, it doesn't set conditional request outside necko.

Comment 12

[Junior [inactive]]

(In reply to Michal Novotny (:michal) from comment #11)
> (In reply to Junior [:junior] from comment #10)
> > Actually if I removed "cache-control", the test would fail to cache the
> > content to a conditional request.
> 
> The test looks good and the functionality is obviously broken. The test
> doesn't pass if I add "cache-control: no-cache" header.
The test passes if I remove |req.loadFlags = Ci.nsIRequest.VALIDATE_ALWAYS| 
and add "cache-control: no-cache" header to make it revalidate.
But I know there's still a problem here.
> 
> > However, I can not reproduce in beta use local apache.
> > My steps:
> > 1. load a file like localhost/cache.html from local server. The response
> > contains Last-Modified header; and does not contains ETag and Cache-Control.
> > 2. wait for several seconds, press enter in address bar again.
> > (I don't use refresh since I want to avoid sending cache-control)
> > 3. check the "about:cache" to see if the data is updated
> 
> This manual test doesn't do the most important step, it doesn't set
> conditional request outside necko.

Yes that's a key point. Thanks.

Comment 13

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=590eadda1c71

Comment 14

[Junior [inactive]]

Attachment: bug482934.patch-v1

Per comment in [1],
we do not override conditional headers when consumer has defined its own.

However, the logic here prevent opening input-stream which is not we want.

Simultaneously push to treeherder to see if the patch breaks anything.

[1] http://searchfox.org/mozilla-central/rev/b4e6fa3527436bdbcd9dd2e1982382fe423431f3/netwerk/protocol/http/nsHttpChannel.cpp#4045

Comment 15

[Michal Novotny [:michal]]

Comment on attachment 8814353 [details] [diff] [review]
bug482934.patch-v1

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

The change looks good to me, but I'd like Honza to take a look at it too.

I see also a problem in returning rv at the end of the method. In this test, rv is a failure returned by GetMetaDataElement("strongly-framed") before "if (doValidation)" statement. So if it is a concurrent access and we set wantCompleteEntry to true, we won't try to open the input stream and we will return RECHECK_AFTER_WRITE_FINISHED while throwing an error. I.e. I think we should set rv=NS_OK at http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/netwerk/protocol/http/nsHttpChannel.cpp#4030

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 8814353 [details] [diff] [review]
bug482934.patch-v1

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +4044,3 @@
>          if (!mCachedResponseHead->NoStore() &&
>              (mRequestHead.IsGet() || mRequestHead.IsHead()) &&
> +            !weaklyFramed && !isImmutable &&

here you must leave the condition.  if the custom header would lead to a revalidation, we might crash when the http channel would do more than 1 request during its lifetime - when the concurrent load would fail.

that is rarely exercised condition, try may not catch this at all.

so, this actually ruins the whole patch.

and honestly, I absolutely don't understand how it should fix the original bug as described anyway.

is there some summary of why you wrote the patch this way?

Comment 17

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> here you must leave the condition.  if the custom header would lead to a
> revalidation, we might crash when the http channel would do more than 1
> request during its lifetime - when the concurrent load would fail.

I understand that we shouldn't override conditional headers when consumer has defined its own. But it's not obvious that we shouldn't do revalidation of the existing cache entry. IIUC we should completely ignore the existing entry and just in case the server returns 200, we should cache the result?

Comment 18

[Junior [inactive]]

(In reply to Honza Bambas (:mayhemer) from comment #16)
> Comment on attachment 8814353 [details] [diff] [review]
> bug482934.patch-v1
> 
> Review of attachment 8814353 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +4044,3 @@
> >          if (!mCachedResponseHead->NoStore() &&
> >              (mRequestHead.IsGet() || mRequestHead.IsHead()) &&
> > +            !weaklyFramed && !isImmutable &&
> 
> is there some summary of why you wrote the patch this way?
The root cause is the problem mentioned by michal in comment 15.

|OpenCacheInputStream| will reset |rv| to NS_OK in this patch.
(if the rv is not NS_OK, CacheEntry will regard the client as ENTRY_NOT_WANTED)
Therefore the patch walks around this issue.

Btw, I set rv=NS_OK at http://searchfox.org/mozilla-central/rev/957458d8fa2328c2a760dbb30e7f1f1efa55b4d0/netwerk/protocol/http/nsHttpChannel.cpp#4030
and pass the test. It might be the solution.

Comment 19

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b07409332a8

Comment 20

[Junior [inactive]]

Attachment: bug482934.patch-v2

Based on comment 15
I think that reseting rv is intuitive, so I didn't put comment here.

Comment 21

[Honza Bambas (:mayhemer)]

Comment on attachment 8815210 [details] [diff] [review]
bug482934.patch-v2

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

Yes!  Now I get it.  This is a correct fix.

r=me with the comments fixed.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3980,5 @@
>              doValidation = true;
>          }
>      }
>  
> +    rv = NS_OK;

please add a good comment why you are resetting the rv here!

::: netwerk/test/unit/test_bug482934.js
@@ +61,5 @@
> +             "  Expected: " + written  + "\n" +
> +             "  Actual: " + aContent.length + "\n");
> +  }
> +  oStream.close();
> +  aCacheEntry.close();

nsICacheEntry.close() with new backend is a no-op, remove this line.

Comment 22

[Honza Bambas (:mayhemer)]

Comment on attachment 8814353 [details] [diff] [review]
bug482934.patch-v1

Looking at this patch again, the idea behind might not be that wrong.  However, it more directs my eyes on a different problem here.  I will file a separate bug for it.

Comment 23

[Michal Novotny [:michal]]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> Comment on attachment 8815210 [details] [diff] [review]
> bug482934.patch-v2
> 
> Review of attachment 8815210 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Yes!  Now I get it.  This is a correct fix.

Junior, you've used the old test that sets VALIDATE_ALWAYS flag. If the test doesn't pass without this flag then this is not a correct fix.

Comment 24

[Junior [inactive]]

(In reply to Michal Novotny (:michal) from comment #23)
> (In reply to Honza Bambas (:mayhemer) from comment #21)
> > Comment on attachment 8815210 [details] [diff] [review]
> > bug482934.patch-v2
> > 
> > Review of attachment 8815210 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Yes!  Now I get it.  This is a correct fix.
> 
> Junior, you've used the old test that sets VALIDATE_ALWAYS flag. If the test
> doesn't pass without this flag then this is not a correct fix.

I have tried to removed VALIDATE_ALWAYS, and the HTTP request will use the cache, not hit the net.
Bug 522463 comment 10 also mentioned this issue.

I think it's not related to this issue.
However, I'm not sure if it's a bug.
Maybe we still think the cache is fresh even if we have If-Modified-Since header.

Do you have suggestion of next step? 
a) The cache is still fresh in this condition, so it's fine.
b) The cache is still fresh but we could switch to use "Cache-Control: no-cache"
c) The cache should be stale. We need to file another bug or use bug 522463.
d) We should let the request hit the net before this bug land.

Comment 25

[Junior [inactive]]

(In reply to Junior [:junior] from comment #24)
> (In reply to Michal Novotny (:michal) from comment #23)
> > (In reply to Honza Bambas (:mayhemer) from comment #21)
> > > Comment on attachment 8815210 [details] [diff] [review]
> > > bug482934.patch-v2
> > > 
> > > Review of attachment 8815210 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > Yes!  Now I get it.  This is a correct fix.
> > 
> > Junior, you've used the old test that sets VALIDATE_ALWAYS flag. If the test
> > doesn't pass without this flag then this is not a correct fix.
> 
> I have tried to removed VALIDATE_ALWAYS, and the HTTP request will use the
> cache, not hit the net.
> Bug 522463 comment 10 also mentioned this issue.
> 
> I think it's not related to this issue.
> However, I'm not sure if it's a bug.
> Maybe we still think the cache is fresh even if we have If-Modified-Since
> header.
> 
> Do you have suggestion of next step? 
> a) The cache is still fresh in this condition, so it's fine.
> b) The cache is still fresh but we could switch to use "Cache-Control:
> no-cache"
> c) The cache should be stale. We need to file another bug or use bug 522463.
> d) We should let the request hit the net before this bug land.

By RFC 2616, Sec 13.2.4 Expiration Calculations

If none of Expires, Cache-Control: max-age, or Cache-Control: s- maxage (see section 14.9.3) appears in the response, and the response does not include other restrictions on caching, the cache MAY compute a freshness lifetime using a heuristic.

Therefore, the fresh time could be calculated in arbitrary way if we don't have hints from the respond headers.
I guess we're fine in this case. Maybe we may add a header |Expires| or |max-age=0| in the test?

Comment 26

[Junior [inactive]]

Attachment: bug482934.patch-v3

Modify by comment 22
and use |Cache-Control: no-cache| instead of |VALIDATE_ALWAYS|.

I'm not sure why it doesn't work if I set |max-age=0| in the first |storeCache|.
It will not hit the net for the next request.

Comment 27

[Michal Novotny [:michal]]

(In reply to Junior [:junior] from comment #25)
> > I have tried to removed VALIDATE_ALWAYS, and the HTTP request will use the
> > cache, not hit the net.
> > Bug 522463 comment 10 also mentioned this issue.
> > 
> > I think it's not related to this issue.
> > However, I'm not sure if it's a bug.
> > Maybe we still think the cache is fresh even if we have If-Modified-Since
> > header.
> > 
> > Do you have suggestion of next step? 
> > a) The cache is still fresh in this condition, so it's fine.
> > b) The cache is still fresh but we could switch to use "Cache-Control:
> > no-cache"
> > c) The cache should be stale. We need to file another bug or use bug 522463.
> > d) We should let the request hit the net before this bug land.
> 
> By RFC 2616, Sec 13.2.4 Expiration Calculations
> 
> If none of Expires, Cache-Control: max-age, or Cache-Control: s- maxage (see
> section 14.9.3) appears in the response, and the response does not include
> other restrictions on caching, the cache MAY compute a freshness lifetime
> using a heuristic.
> 
> Therefore, the fresh time could be calculated in arbitrary way if we don't
> have hints from the respond headers.
> I guess we're fine in this case. Maybe we may add a header |Expires| or
> |max-age=0| in the test?

If the conditional request is set by the user we should IMO always validate with the server, because the user probably knows better why he is doing it. Honza, do you agree?

Comment 28

[Junior [inactive]]

(In reply to Junior [:junior] from comment #26)
> Created attachment 8816091 [details] [diff] [review]
> bug482934.patch-v3
> 
> Modify by comment 22
> and use |Cache-Control: no-cache| instead of |VALIDATE_ALWAYS|.
> 
> I'm not sure why it doesn't work if I set |max-age=0| in the first
> |storeCache|.
> It will not hit the net for the next request.

FWIW, there's a bug 1306500 which indicates that we still use cache for a |max-age=0, must-revalidate| subresource. However, |must-revalidate| should revalidate with a stale resource, and |max-age=0| hints us the resource is stale immediately.

If it's an issue, bug 1306500 might be more general for a |max-age=0|.

Comment 29

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #27)
> If the conditional request is set by the user we should IMO always validate
> with the server, because the user probably knows better why he is doing it.
> Honza, do you agree?

Hmm.. some of the If-* request headers may force validation, yes.  One by one then:

If-Match: we already force validation when privately cached content doesn't match
If-None-Match: here is the validation explicitly expected (so, yes, force it)
If-Modified-Since: as well
If-Unmodified-Since: I think this should be handled the same way as If-Match
If-Range: as well

So, to sum, yes, I think all of them by either pure presence or not evaluating the condition should force validation.

Comment 30

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #29)
> If-Range: as well

I wanted to say "as well as If-None-Match"

Comment 31

[Michal Novotny [:michal]]

If I understand correctly the code around mCustomConditionalRequest, we should always send a conditional request to the server, even if we don't have the cache entry or if VALIDATE_NEVER flag is passed (which is in fact an unexpected behavior). Also we should never read from the entry, we should pass to the client what the server returns. So the best way how to solve this bug is IMO to not open the cache entry at all in nsHttpChannel::OpenCacheEntry() and just in case server returns 200 and it was a custom conditional request, cache the result (regardless if we have the entry or not) using nsICacheStorage::openTruncate().

Junior, I don't think we have tests for the above. In the fix for this patch, please include following tests:

- send custom conditional request when we don't have an entry, server returns 304 -> client receives 304
- send custom conditional request when we have an entry, server returns 304 -> client receives 304 and cached entry is unchanged
- send custom conditional request when we don't have an entry, server returns 200 -> result is cached
- send custom conditional request when we have an entry, server returns 200 -> result is cached (this is the current test)

Comment 32

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #31)
> If I understand correctly the code around mCustomConditionalRequest, we
> should always send a conditional request to the server, 

looks like, yes.  but think of the "positive" conditions too (like in case of If-Match).

> even if we don't
> have the cache entry

Yes.

>  or if VALIDATE_NEVER flag is passed (which is in fact
> an unexpected behavior). 

I'd give any conditional header a priority over it.  But worth checking how VALIDATE_NEVER is used.

> Also we should never read from the entry, 

You are probably right.  I remember Honza O. asking for something like this: get 304 rather then cached 200 when a cond request was setup externally (=custom cond req header).  I think the If-Match handling is the result.

> we should
> pass to the client what the server returns. So the best way how to solve
> this bug is IMO to not open the cache entry at all in
> nsHttpChannel::OpenCacheEntry() and just in case server returns 200 and it
> was a custom conditional request, cache the result (regardless if we have
> the entry or not) using nsICacheStorage::openTruncate().
> 
> Junior, I don't think we have tests for the above. In the fix for this
> patch, please include following tests:
> 
> - send custom conditional request when we don't have an entry, server
> returns 304 -> client receives 304
> - send custom conditional request when we have an entry, server returns 304
> -> client receives 304 and cached entry is unchanged
> - send custom conditional request when we don't have an entry, server
> returns 200 -> result is cached
> - send custom conditional request when we have an entry, server returns 200
> -> result is cached (this is the current test)

There is also a possibility of 412 Precondition Failed when positive validation fails.  But that should propagate to client as well, IMO.

Comment 33

[Junior [inactive]]

> So the best way how to solve
> this bug is IMO to not open the cache entry at all in
> nsHttpChannel::OpenCacheEntry() and just in case server returns 200 and it
> was a custom conditional request, cache the result (regardless if we have
> the entry or not) using nsICacheStorage::openTruncate().
> 

If I understand correctly, current flow for caching 200 is:
Step 1. async open cache entry
nsHTTPChannel::Connect ->
nsHTTPChannel::OpenCacheEntry ->
nsICacheStorage::AsyncOpenURI

Stop 2. init cache entry and install cache listener
nsHTTPChannel::OnStartReqeust ->
nsHTTPChannel::ProcessNormal ->
nsHTTPChannel::InitCacheEntry and InstallCacheListener
 
If we like to escape AsyncOpenURI in step 1 for a customized conditional request, 
we should move the openTruncate to OnStartRequest. However, we can not guarantee
the order of the callbacks for openTruncate (OnCacheEntryCheck/Available) and OnDataAvailable.
And I guess all the operations in OnStartReqeust should be synchronous.

Any suggestion?

Comment 34

[Junior [inactive]]

(In reply to Junior [:junior] from comment #33)
> > So the best way how to solve
> > this bug is IMO to not open the cache entry at all in
> > nsHttpChannel::OpenCacheEntry() and just in case server returns 200 and it
> > was a custom conditional request, cache the result (regardless if we have
> > the entry or not) using nsICacheStorage::openTruncate().
> > 
> 
> If I understand correctly, current flow for caching 200 is:
> Step 1. async open cache entry
> nsHTTPChannel::Connect ->
> nsHTTPChannel::OpenCacheEntry ->
> nsICacheStorage::AsyncOpenURI
> 
> Stop 2. init cache entry and install cache listener
> nsHTTPChannel::OnStartReqeust ->
> nsHTTPChannel::ProcessNormal ->
> nsHTTPChannel::InitCacheEntry and InstallCacheListener
>  
> If we like to escape AsyncOpenURI in step 1 for a customized conditional
> request, 
> we should move the openTruncate to OnStartRequest. However, we can not
> guarantee
> the order of the callbacks for openTruncate (OnCacheEntryCheck/Available)
> and OnDataAvailable.
> And I guess all the operations in OnStartReqeust should be synchronous.
> 
> Any suggestion?

Sorry for misunderstanding. OpenTruncate should be a sync method.

Comment 35

[Junior [inactive]]

Attachment: bug482934.patch-v4

Delay open cache entry for a custom conditional request

TODO
Add the full test indicated by comment 31

Comment 36

[Junior [inactive]]

Attachment: bug482934.patch-withFullTest-WIP-v1

I found that the case will cache 304, which might not what we want.

- send custom conditional request when we have an entry, server returns 304 -> client receives 304 and cached entry is unchanged

Comment 37

[Michal Novotny [:michal]]

Comment on attachment 8820648 [details] [diff] [review]
bug482934.patch-v4

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2346,5 @@
> +        bool isHttps = false;
> +        rv = mURI->SchemeIs("https", &isHttps);
> +        NS_ENSURE_SUCCESS(rv,rv);
> +
> +        Unused << OpenCacheEntry(isHttps);

(In reply to Junior [:junior] from comment #36)
> I found that the case will cache 304, which might not what we want.

We want to open the entry only if it is 200 response. Also I think that most of the checks in OpenCacheEntry are not applicable in this case, so I'd prefer to create a separate function for opening the entry or do it directly in this block if the code is small enough.

Comment 38

[Junior [inactive]]

Attachment: bug482934.patch-v5

Modified by comment 37

Comment 39

[Michal Novotny [:michal]]

Comment on attachment 8822147 [details] [diff] [review]
bug482934.patch-v5

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2345,5 @@
> +    // Only cache 200 response for a custom conditional request
> +    if (mCustomConditionalRequest && mResponseHead->Status() == 200) {
> +        bool isHttps = false;
> +        rv = mURI->SchemeIs("https", &isHttps);
> +        NS_ENSURE_SUCCESS(rv,rv);

This is now unused.

@@ +3433,5 @@
> +    mRequestHead.GetHeader(nsHttp::Cache_Control, cacheControlRequestHeader);
> +    CacheControlParser cacheControlRequest(cacheControlRequestHeader);
> +    if (cacheControlRequest.NoStore()) {
> +        return;
> +    }

We can store no-store entries in memory.

::: netwerk/test/unit/test_bug482934.js
@@ +5,5 @@
> +const new_content = "new_content_body";
> +
> +const old_cache_time = "Thu, 1 Jan 2009 00:00:00 GMT";
> +const new_cache_time = "Fri, 2 Jan 2009 00:00:00 GMT";
> +

With multiple tests it's easy to get lost when old_content and new_content should be used. So instead of using a global variables, set them always before running the test the same way as response code is set. E.g. something like this:

add_test(() => {
  // A 304 Not Modified
  response_code = "304";
  response_body = "";
  request_time = "Thu, 1 Jan 2009 00:00:00 GMT";
  response_time = "Thu, 1 Jan 2009 00:00:00 GMT";
...

@@ +95,5 @@
> +  // A 304 Not Modified
> +  response_code = "304";
> +  hit_server = false;
> +  var ch = make_channel(resource_url);
> +  ch.asyncOpen2(new ChannelListener(function(aRequest, aData) {

Check also the response status (in other tests too).

@@ +97,5 @@
> +  hit_server = false;
> +  var ch = make_channel(resource_url);
> +  ch.asyncOpen2(new ChannelListener(function(aRequest, aData) {
> +    do_check_true(hit_server);
> +    do_check_throws_nsIException(() => cache_storage.exists(make_uri(resource_url), ""), 'NS_ERROR_NOT_AVAILABLE');

This is wrong. The return value should be false. It throws NS_ERROR_NOT_AVAILABLE because the index is not yet in READY state and not because the entry is not available. Call wait_for_cache_index() before running this test to make sure the index is ready.

@@ +119,5 @@
> +  }, null));
> +});
> +
> +add_test(() => {
> +  // Check the cache data is saved

This checks the result of the second test, so I don't see a reason, why to put it into a separate test. Include it in the test above.

@@ +132,5 @@
> +  );
> +});
> +
> +add_test(() => {
> +  evict_cache_entries();

Why not to reuse the entry saved in test 2 in the next test?

@@ +140,5 @@
> +// 3. send custom conditional request when we have an entry
> +//    server returns 304 -> client receives 304 and cached entry is unchanged
> +add_test(() => {
> +  // Cache old data
> +  with_cache = true;

This isn't used anywhere.

@@ +172,5 @@
> +  }, null));
> +});
> +
> +add_test(() => {
> +  // Check the cache data is not changed

Again, include this in the test above.

@@ +200,5 @@
> +  }, null));
> +});
> +
> +add_test(() => {
> +  // Check the cache data is updated

Like in the tests above, include the check in the test. Also the code is the same in all cases so separate it into a function.

@@ +201,5 @@
> +});
> +
> +add_test(() => {
> +  // Check the cache data is updated
> +  asyncOpenCacheEntry(resource_url, "disk", Ci.nsICacheStorage.OPEN_NORMALLY, null,

When only reading the data you can use OPEN_READONLY.

Comment 40

[Junior [inactive]]

Attachment: bug482934.patch-v6

Thanks for your great and detailed review, michal!
I learnt a lot and the test looks much more beautiful.

Obsolete the previous patches and modify by last comment.

Comment 41

[Junior [inactive]]

Attachment: bug482934.patch-v7

Fix a nit. Sorry for the spam

Comment 42

[Michal Novotny [:michal]]

Comment on attachment 8824948 [details] [diff] [review]
bug482934.patch-v7

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

Nice, thanks! r+ with the 2 nits fixed. I'd like Honza to have a look at the patch too.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3472,5 @@
> +        nsAutoCString cacheControlRequestHeader;
> +        mRequestHead.GetHeader(nsHttp::Cache_Control, cacheControlRequestHeader);
> +        CacheControlParser cacheControlRequest(cacheControlRequestHeader);
> +        if (cacheControlRequest.NoStore()) {
> +            return;

This should never happen because nsHttpChannel::UpdateInhibitPersistentCachingFlag() should always set INHIBIT_PERSISTENT_CACHING flag if there is no-store in the header. You can turn this into debug assertion. Also there is no need to use CacheControlParser here, use simply mResponseHead->NoStore().

::: netwerk/test/unit/test_bug482934.js
@@ +136,5 @@
> +  ch.asyncOpen2(new ChannelListener(function(aRequest, aData) {
> +    do_check_true(hit_server);
> +    do_check_eq(aRequest.responseStatus, 304);
> +    do_check_true(cache_storage.exists(make_uri(resource_url), ""));
> +    do_check_eq(aRequest.getResponseHeader("Returned-From-Handler"), "1");

Sorry that I missed this in the previous review. I'm not sure this header is useful. IIUC it should guarantee that the response is from the handler and not from the cache entry which should not be used for reading. The problem is that the cached headers are merged with the header from the network response (http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/netwerk/protocol/http/nsHttpChannel.cpp#3259), so if cached response would be used, it would contain this header too. Instead, we should IMO compare aData with response_body which is empty here. This will ensure that the content was not served from the cache.

Comment 43

[Honza Bambas (:mayhemer)]

Comment on attachment 8824948 [details] [diff] [review]
bug482934.patch-v7

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

I'm not happy with not opening an entry for read at all when there is a custom cond header.  it breaks following:
- chossing appcache (we don't maintain appcache but we don't want to break it if not necessary)
- not clear what to do when the channel is directly assigned an appcache
- If-Match, when eval to true, should go from cache when valid 

you can always set mContentIsValid to false or use some other mechanism to never get to the point we do ReadFromCache() and rather do CloseCacheEntry() + ProcessNormal() when you don't want to use the existing entry.  forcing validation is always very easy - doValidation = true in OnCacheEntryCheck.

but feel free to persuade me otherwise.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +403,3 @@
>      // open a cache entry for this channel...
> +    if (!mCustomConditionalRequest)
> +        rv = OpenCacheEntry(isHttps);

if { } please..

write a comment why you don't want to open an entry at all at this place.

OTOH, this breaks the If-Match checking at https://dxr.mozilla.org/mozilla-central/rev/8eaf154b385bbe0ff06155294ccf7962aa2d3324/netwerk/protocol/http/nsHttpChannel.cpp#4011

When the match passes, I think we want to go from the cache then.

Also, what if the channel should check from appcache or is assigned an appacache?  The 'choose' appcache case is not simple to solve...  The case when the channel is assigned an appcache MUST open a cache entry.

@@ +3467,5 @@
> +    if (mLoadFlags & INHIBIT_PERSISTENT_CACHING) {
> +        rv = cacheStorageService->MemoryCacheStorage(info,
> +             getter_AddRefs(cacheStorage));
> +    }
> +    else {

} else {

@@ +3472,5 @@
> +        nsAutoCString cacheControlRequestHeader;
> +        mRequestHead.GetHeader(nsHttp::Cache_Control, cacheControlRequestHeader);
> +        CacheControlParser cacheControlRequest(cacheControlRequestHeader);
> +        if (cacheControlRequest.NoStore()) {
> +            return;

Michal, this is checking for REQUEST cache-control header.  You refer to the RESPONSE header.

@@ +3476,5 @@
> +            return;
> +        }
> +
> +        rv = cacheStorageService->DiskCacheStorage(info,
> +             !mPostID && (mChooseApplicationCache || (mLoadFlags & LOAD_CHECK_OFFLINE_CACHE)),

hmm... first, to opentruncate a cache entry when appcache is to be checked is nonsense.  please pass false here (I think for OpenTruncate the arg is ignored).

@@ +3487,5 @@
> +        extension.Append(nsPrintfCString("%d", mPostID));
> +    }
> +
> +    rv = cacheStorage->OpenTruncate(mURI, extension, getter_AddRefs(mCacheEntry));
> +    NS_ENSURE_SUCCESS_VOID(rv);

if you want a console warning, then use NS_WARNING or make this method return nsresult.  but NS_ENSURE_SUCCESS_VOID(rv); as the last line in a method is strange.

::: netwerk/protocol/http/nsHttpChannel.h
@@ +363,5 @@
>      nsresult InstallCacheListener(int64_t offset = 0);
>      nsresult InstallOfflineCacheListener(int64_t offset = 0);
>      void     MaybeInvalidateCacheEntryForSubsequentGet();
>      void     AsyncOnExamineCachedResponse();
> +    void     OpenCacheTruncate();

OpenTruncateCacheEntry please

Comment 44

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #43)
> Comment on attachment 8824948 [details] [diff] [review]
> bug482934.patch-v7
> 
> Review of attachment 8824948 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not happy with not opening an entry for read at all when there is a
> custom cond header.  it breaks following:
> - chossing appcache (we don't maintain appcache but we don't want to break
> it if not necessary)

This is not a strong argument.  Appcache is being chosen only for toplevel docs that will probably never be set a cond request header.

> - not clear what to do when the channel is directly assigned an appcache

In this case we should ignore cond request headers.



Overall, I'm not sure what is the right fix here.

Comment 45

[Michal Novotny [:michal]]

> - If-Match, when eval to true, should go from cache when valid 

Why we shouldn't pass the response from the server in this case?

Comment 46

[Honza Bambas (:mayhemer)]

(In reply to Michal Novotny (:michal) from comment #45)
> > - If-Match, when eval to true, should go from cache when valid 
> 
> Why we shouldn't pass the response from the server in this case?

I thought the response from the server could be different if we ignored our cache, but that is not true.  I think it's OK to let even If-Match go to the server and bypass our cache for reading.

Then, maybe as part of this patch remove the code at [1]


Now something else: the reason I suggested to open the cache entry even when a cond req header is set is to not reimplement (incompletely) the cache storage selection to OpenTruncate an entry later.  To recreate the entry you just call Recreate() on the already open entry directly, as at [2].

But, we may need some mechanism to open-truncate cache entries later as part of the cache racing anyway.  So, I'll leave it up to you.


[1] https://dxr.mozilla.org/mozilla-central/rev/8eaf154b385bbe0ff06155294ccf7962aa2d3324/netwerk/protocol/http/nsHttpChannel.cpp#4009
[2] https://dxr.mozilla.org/mozilla-central/rev/8eaf154b385bbe0ff06155294ccf7962aa2d3324/netwerk/protocol/http/nsHttpChannel.cpp#4871

Comment 47

[Junior [inactive]]

I agree that current implementation changes more.
However, we might use |OpenTruncateCacheEntry| later

My plan is 
1. update |OpenTruncateCacheEntry| by comment 43, and leave the patch here.
   If we really need this, we won't rebuild the wheel.
2. Try this solution: open the cache while connect, and try to hit the net without reading the cache

Comment 48

[Junior [inactive]]

Attachment: openCacheEntryTruncate_PLEASE_DONT_LAND_HERE

Leave the patch here for future reference

Comment 49

[Junior [inactive]]

Attachment: bug482934.patch-v8

Comment 50

[Honza Bambas (:mayhemer)]

Comment on attachment 8827828 [details] [diff] [review]
bug482934.patch-v8

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +2184,5 @@
>                  return NS_OK;
>              }
>          }
>  
> +        // Do not write 304 to cache for a custom conditional reqeust

can you say WHY?

@@ +3942,5 @@
>      else if (mCachedResponseHead->MustValidate()) {
>          LOG(("Validating based on MustValidate() returning TRUE\n"));
>          doValidation = true;
> +    } else if (mCustomConditionalRequest) {
> +        LOG(("Validating based on a custom conditional reqeust returning TRUE\n"));

"Validating based on a custom conditional request"

the "returning TRUE" belongs to a reference to the MustValidate() method..

when you do the check here, there are few conditions that may set doValidation = false, unconditionally: LOAD_FROM_CACHE, VALIDATE_NEVER, force-valid.  I don't know if that's what you want or not.  Let's try the way you wrote it, under normal circumstances it will do what you want.

@@ +3944,5 @@
>          doValidation = true;
> +    } else if (mCustomConditionalRequest) {
> +        LOG(("Validating based on a custom conditional reqeust returning TRUE\n"));
> +        doValidation = true;
> +    }else {

} else {

Comment 51

[Junior [inactive]]

> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +2184,5 @@
> >                  return NS_OK;
> >              }
> >          }
> >  
> > +        // Do not write 304 to cache for a custom conditional reqeust
> 
> can you say WHY?

Will replace with
// Don't cache uninformative 304

In fact, three cases went to here:
!mDidReval and NS_FAILED(rv) are with null mCacheEntry.
Maybe we can remove the |if (mCustomConditionalRequest)|

Btw, this |if| statement seems unusable here
http://searchfox.org/mozilla-central/rev/d20e4431d0cf40311afa797868bc5c58c54790a2/netwerk/protocol/http/nsHttpChannel.cpp#2188

Comment 52

[Junior [inactive]]

Attachment: bug482934.patch-v9

update by comment 50

Comment 53

[Michal Novotny [:michal]]

Comment on attachment 8833257 [details] [diff] [review]
bug482934.patch-v9

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

r+ with the following 2 changes

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ -3718,5 @@
> -        mRequestHead.HasHeader(nsHttp::If_None_Match) ||
> -        mRequestHead.HasHeader(nsHttp::If_Unmodified_Since) ||
> -        mRequestHead.HasHeader(nsHttp::If_Match) ||
> -        mRequestHead.HasHeader(nsHttp::If_Range);
> -

Moving this block is not needed anymore because the early return was removed. Remove this change.

@@ +3942,5 @@
>      else if (mCachedResponseHead->MustValidate()) {
>          LOG(("Validating based on MustValidate() returning TRUE\n"));
>          doValidation = true;
> +    } else if (mCustomConditionalRequest) {
> +        LOG(("Validating based on a custom conditional reqeust\n"));

Fix the typo "request".

Comment 54

[Junior [inactive]]

Attachment: bug482934.patch-v10

modify nits and carry r+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d651ba53663f66ad4fdc8dd110d4549b3bd2ecc4

Comment 55

[Junior [inactive]]

(In reply to Michal Novotny (:michal) from comment #53)
> Comment on attachment 8833257 [details] [diff] [review]
> bug482934.patch-v9
> 
> Review of attachment 8833257 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with the following 2 changes
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ -3718,5 @@
> > -        mRequestHead.HasHeader(nsHttp::If_None_Match) ||
> > -        mRequestHead.HasHeader(nsHttp::If_Unmodified_Since) ||
> > -        mRequestHead.HasHeader(nsHttp::If_Match) ||
> > -        mRequestHead.HasHeader(nsHttp::If_Range);
> > -
> 
> Moving this block is not needed anymore because the early return was
> removed. Remove this change.
> 

For the first test scenario:

// send custom conditional request when we don't have an entry
// server returns 304 -> client receives 304

We don't open a cache entry, so the onCacheEntryCheck isn't called.
But we rely on mCustomConditionalRequest to avoid caching 304.

I'd like to keep this change.

Do you agree, michal?

Comment 56

[Junior [inactive]]

Bug 475156 with a test stands on an opposite side with comment 27 and comment 29.

That is, bug 475156 tends to use cache for XHR,
instead of validation always for a custom conditional request.

I'd like to remove the test of bug 475156.
What do you think, honza?

Comment 57

[Michal Novotny [:michal]]

(In reply to Junior[:junior] from comment #55)
> For the first test scenario:
> 
> // send custom conditional request when we don't have an entry
> // server returns 304 -> client receives 304
> 
> We don't open a cache entry, so the onCacheEntryCheck isn't called.
> But we rely on mCustomConditionalRequest to avoid caching 304.
> 
> I'd like to keep this change.
> 
> Do you agree, michal?

You're right. I overlooked this.

Comment 58

[Honza Bambas (:mayhemer)]

(In reply to Junior[:junior] from comment #56)
> Bug 475156 with a test stands on an opposite side with comment 27 and
> comment 29.
> 
> That is, bug 475156 tends to use cache for XHR,
> instead of validation always for a custom conditional request.
> 
> I'd like to remove the test of bug 475156.
> What do you think, honza?

So, what do you think happens here?  The test obviously found a regression!  We really don't want to regress bug 475156.  Hence, you need to fix the patch for this bug to make that regression-catching test pass.

So, my statement in comment 29 should be restate for If-Match and If-Unmodified-Since as: if the cached content matches the cond request, serve from the cache.  

Sorry, I didn't catch this before in bugzilla.  But that is what we have tests for ;)

Comment 59

[Junior [inactive]]

Attachment: bug482934.patch-v11

Update by last comment.

Hello Honza,
Sorry for requesting another review due to a small change :)

Try result
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2916e2ebb651d261c4e2178cc25705bc3ee2b5f

Comment 60

[Honza Bambas (:mayhemer)]

Comment on attachment 8835269 [details] [diff] [review]
bug482934.patch-v11

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +3941,5 @@
>      // check if validation is strictly required...
>      else if (mCachedResponseHead->MustValidate()) {
>          LOG(("Validating based on MustValidate() returning TRUE\n"));
>          doValidation = true;
> +    // possibly serve from cache for a custum If-Match/If-Unmodified-Since

custom

Comment 61

[Junior [inactive]]

Attachment: bug482934.patch-v12

Fix a typo by last comment.
Try result is in comment 59 and I think we don't need to have another try-run.

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec7a46fa914
force to revalidate for a custom conditional reqeust, r=michal, honzab

Comment 63

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/6ec7a46fa914