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

RESOLVED FIXED in Firefox 54

Status

()

Core
Networking: Cache
RESOLVED FIXED
9 years ago
3 months ago

People

(Reporter: Biesinger, Assigned: junior)

Tracking

(Depends on: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [necko-active], URL)

Attachments

(2 attachments, 16 obsolete attachments)

2.88 KB, patch
Details | Diff | Splinter Review
11.24 KB, patch
junior
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → michal
Created attachment 369516 [details] [diff] [review]
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.
Attachment #369516 - Flags: review?(cbiesinger)
Attachment #369516 - Flags: review?(cbiesinger) → review+
Blocks: 522463
See Also: → bug 759582
is there a reason this r+d patch wasn't landed? Should the bug be otherwise resolved?
Flags: needinfo?(michal.novotny)
Whiteboard: [necko-backlog]
The patch needs to be reevaluated.  Cache 2 integration to http channel made too many changes.
Junior will have a look at it. This might be a good first bug to get in touch with the cache code.
Assignee: michal.novotny → juhsu
Flags: needinfo?(michal.novotny)
Whiteboard: [necko-backlog] → [necko-active]
(Assignee)

Comment 5

a year ago
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?
Flags: needinfo?(michal.novotny)
Yes please, take test_bug482934.js and change it so it uses new cache API.
Flags: needinfo?(michal.novotny)
(Assignee)

Comment 7

a year ago
Created attachment 8810799 [details] [diff] [review]
test_v1_WIP

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

Comment 8

a year ago
Created attachment 8812118 [details] [diff] [review]
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.
Attachment #8810799 - Attachment is obsolete: true
Attachment #8812118 - Flags: feedback?(michal.novotny)
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.
Attachment #8812118 - Flags: feedback?(michal.novotny) → feedback+
(Assignee)

Comment 10

a year ago
Created attachment 8812728 [details] [diff] [review]
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.
Attachment #8812118 - Attachment is obsolete: true
Attachment #8812728 - Flags: feedback?(michal.novotny)
(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.
Attachment #8812728 - Flags: feedback?(michal.novotny) → feedback+
(Assignee)

Comment 12

a year ago
(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.
(Assignee)

Comment 13

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=590eadda1c71
(Assignee)

Comment 14

a year ago
Created attachment 8814353 [details] [diff] [review]
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
Attachment #8812728 - Attachment is obsolete: true
Attachment #8814353 - Flags: review?(michal.novotny)
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
Attachment #8814353 - Flags: review?(michal.novotny)
Attachment #8814353 - Flags: review?(honzab.moz)
Attachment #8814353 - Flags: feedback+
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?
Attachment #8814353 - Flags: review?(honzab.moz) → review-
(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?
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 18

a year ago
(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.
(Assignee)

Comment 19

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b07409332a8
(Assignee)

Comment 20

a year ago
Created attachment 8815210 [details] [diff] [review]
bug482934.patch-v2

Based on comment 15
I think that reseting rv is intuitive, so I didn't put comment here.
Attachment #8814353 - Attachment is obsolete: true
Attachment #8815210 - Flags: review?(michal.novotny)
Attachment #8815210 - Flags: review?(honzab.moz)
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 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.
Flags: needinfo?(honzab.moz)
Attachment #8815210 - Flags: review?(honzab.moz) → review+
(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.
Flags: needinfo?(juhsu)
(Assignee)

Comment 24

a year ago
(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.
Flags: needinfo?(juhsu) → needinfo?(michal.novotny)
(Assignee)

Comment 25

a year ago
(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?
(Assignee)

Comment 26

a year ago
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.
Attachment #8815210 - Attachment is obsolete: true
Attachment #8815210 - Flags: review?(michal.novotny)
Attachment #8816091 - Flags: review?(michal.novotny)
(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?
Flags: needinfo?(michal.novotny) → needinfo?(honzab.moz)
(Assignee)

Comment 28

a year ago
(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|.
(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.
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #29)
> If-Range: as well

I wanted to say "as well as If-None-Match"
(Assignee)

Updated

a year ago
See Also: → bug 1321309
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)
(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.
(Assignee)

Comment 33

a year ago
> 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?
Flags: needinfo?(michal.novotny)
(Assignee)

Comment 34

a year ago
(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.
Flags: needinfo?(michal.novotny)
(Assignee)

Comment 35

a year ago
Created attachment 8820648 [details] [diff] [review]
bug482934.patch-v4

Delay open cache entry for a custom conditional request

TODO
Add the full test indicated by comment 31
Attachment #8816091 - Attachment is obsolete: true
Attachment #8816091 - Flags: review?(michal.novotny)
Attachment #8820648 - Flags: feedback?(michal.novotny)
(Assignee)

Comment 36

a year ago
Created attachment 8821102 [details] [diff] [review]
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 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.
Attachment #8820648 - Flags: feedback?(michal.novotny) → feedback+
(Assignee)

Comment 38

a year ago
Created attachment 8822147 [details] [diff] [review]
bug482934.patch-v5

Modified by comment 37
Attachment #8820648 - Attachment is obsolete: true
Attachment #8821102 - Attachment is obsolete: true
Attachment #8822147 - Flags: review?(michal.novotny)
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.
Attachment #8822147 - Flags: review?(michal.novotny) → feedback+
(Assignee)

Comment 40

11 months ago
Created attachment 8824861 [details] [diff] [review]
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.
Attachment #369516 - Attachment is obsolete: true
Attachment #8822147 - Attachment is obsolete: true
Attachment #8824861 - Flags: review?(michal.novotny)
(Assignee)

Comment 41

11 months ago
Created attachment 8824948 [details] [diff] [review]
bug482934.patch-v7

Fix a nit. Sorry for the spam
Attachment #8824861 - Attachment is obsolete: true
Attachment #8824861 - Flags: review?(michal.novotny)
Attachment #8824948 - Flags: review?(michal.novotny)
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.
Attachment #8824948 - Flags: review?(michal.novotny)
Attachment #8824948 - Flags: review?(honzab.moz)
Attachment #8824948 - Flags: review+
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
Attachment #8824948 - Flags: review?(honzab.moz) → review-
(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.
> - 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?
Flags: needinfo?(honzab.moz)
(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
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 47

11 months ago
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
(Assignee)

Comment 48

11 months ago
Created attachment 8827825 [details] [diff] [review]
openCacheEntryTruncate_PLEASE_DONT_LAND_HERE

Leave the patch here for future reference
(Assignee)

Comment 49

11 months ago
Created attachment 8827828 [details] [diff] [review]
bug482934.patch-v8
Attachment #8824948 - Attachment is obsolete: true
Attachment #8827828 - Flags: review?(michal.novotny)
Attachment #8827828 - Flags: review?(honzab.moz)
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 {
Attachment #8827828 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 51

11 months ago
> ::: 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
(Assignee)

Comment 52

11 months ago
Created attachment 8833257 [details] [diff] [review]
bug482934.patch-v9

update by comment 50
Attachment #8827828 - Attachment is obsolete: true
Attachment #8827828 - Flags: review?(michal.novotny)
Attachment #8833257 - Flags: review?(michal.novotny)
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".
Attachment #8833257 - Flags: review?(michal.novotny) → review+
(Assignee)

Comment 54

10 months ago
Created attachment 8834228 [details] [diff] [review]
bug482934.patch-v10

modify nits and carry r+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d651ba53663f66ad4fdc8dd110d4549b3bd2ecc4
Attachment #8833257 - Attachment is obsolete: true
Attachment #8834228 - Flags: review+
(Assignee)

Comment 55

10 months ago
(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?
Flags: needinfo?(michal.novotny)
(Assignee)

Comment 56

10 months ago
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?
Flags: needinfo?(honzab.moz)
(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.
Flags: needinfo?(michal.novotny)
(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 ;)
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 59

10 months ago
Created attachment 8835269 [details] [diff] [review]
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
Attachment #8834228 - Attachment is obsolete: true
Attachment #8835269 - Flags: review?(honzab.moz)
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
Attachment #8835269 - Flags: review?(honzab.moz) → review+
(Assignee)

Comment 61

10 months ago
Created attachment 8835941 [details] [diff] [review]
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.
Attachment #8835269 - Attachment is obsolete: true
Attachment #8835941 - Flags: review+
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 62

10 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ec7a46fa914
force to revalidate for a custom conditional reqeust, r=michal, honzab
Keywords: checkin-needed

Comment 63

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6ec7a46fa914
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

10 months ago
Depends on: 1338675

Updated

10 months ago
Depends on: 1338943

Updated

10 months ago
Component: Networking: HTTP → Networking: Cache
Duplicate of this bug: 522463
You need to log in before you can comment on or make changes to this bug.