Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reload goes back in time (http cache issue?) #24385

Open
paulrouget opened this issue Oct 7, 2019 · 6 comments · May be fixed by #24388
Open

Reload goes back in time (http cache issue?) #24385

paulrouget opened this issue Oct 7, 2019 · 6 comments · May be fixed by #24388

Comments

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Oct 7, 2019

STR:

  • create a file test.html which contains just the letter a
  • make it available via http (python3 -m http.server 8080)
  • load it with servo
  • change the content of test.html: a to b
  • reload. see b
  • reload. see a

Can't reproduce using a file:/// url

@jdm
Copy link
Member

@jdm jdm commented Oct 7, 2019

Hmm, I could reproduce this in a build based on ea4e3ae, but not in a build based on 37ab273. Now the cached entry is always used and I never see the updated content.

@jdm
Copy link
Member

@jdm jdm commented Oct 7, 2019

Nevermind, I can still reproduce it.

@jdm
Copy link
Member

@jdm jdm commented Oct 7, 2019

This looks like it happens because http_network_or_cache_fetch calls HttpCache::store after a revalidation request that returns a non-306 response. That means there are two cached responses, and then reloading calls HttpCache::refresh when the new revalidation request returns a 304. HttpCache::refresh just returns the first cached response, which is the older one in this case.

@jdm
Copy link
Member

@jdm jdm commented Oct 7, 2019

I think we should make HttpCache store overwrite any existing complete response so that we only ever have one complete response and an arbitrary number of incomplete responses for a particular cache entry.

@jdm jdm linked a pull request that will close this issue Oct 7, 2019
3 of 4 tasks complete
@jdm jdm self-assigned this Oct 7, 2019
bors-servo added a commit that referenced this issue Oct 8, 2019
Fix HTTP cache revalidation bugs

This fixes two cache related issues:
* old cached content could be viewed as a result of revalidating a newer cached response
* the expiry of a cached response refreshed through a revalidation could be calculated without any of the original response's headers

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #24385
- [ ] There are tests for these changes

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/24388)
<!-- Reviewable:end -->
@gterzian
Copy link
Member

@gterzian gterzian commented Oct 8, 2019

It's weird, on my Mac with python 3, it seems the server doesn't understand the if-modified-since request header, and responds with a 200 even though the resource has been modified...

see some printed logs.

Request headers: {"accept": "text/html, application/xhtml+xml, application/xml; q=0.9, */*; q=0.8", "accept-language": "en-US, en; q=0.5", "referer": "http://0.0.0.0:8080/test.html", "user-agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:63.0) Servo/1.0 Firefox/63.0", "host": "0.0.0.0:8080", "accept-encoding": "gzip, deflate, br", "if-modified-since": "Tue, 08 Oct 2019 09:37:43 GMT"}
Forward response: {"server": "SimpleHTTP/0.6 Python/3.6.5", "date": "Tue, 08 Oct 2019 09:38:08 GMT", "content-type": "text/html", "content-length": "1", "last-modified": "Tue, 08 Oct 2019 09:38:04 GMT"} Some((200, "OK"))

So I can't seem to hit the refresh case.

Also note that this occurs because the python server by default sends a response containing a Last-Modified header(and also a Date).

I think a more appropriate fix could be to find the "right" stored resource to use in the refresh operation, by comparing the stored Last-Modified header with the one from the 304 response, as described at https://tools.ietf.org/html/rfc7232#section-2.2.2 in the second "or" paragraph(see also the use of the Date header in there).

@gterzian
Copy link
Member

@gterzian gterzian commented Oct 8, 2019

Also note that the "need to revalidate" is calculated based on the Last-Modified header of the cached resource, so one really needs to create, or modify, the test.html file right before starting the manual test, since otherwise the cached resource will remain valid for quite some time(10% of the time elapsed since the file was modified).

Just FYI in case someone starts to wonder why the test cannot be reproduced anymore...

@gterzian gterzian mentioned this issue Jun 20, 2020
0 of 5 tasks complete
@jdm jdm mentioned this issue Jul 28, 2020
4 of 4 tasks complete
bors-servo added a commit that referenced this issue Jul 28, 2020
Make reload button clear the network cache.

The developer workflow in FxR is frustrating right now because of bugs like #24385. To allow us to put out a new release soon that addresses this papercut, these changes make the reload button clear the network cache in FxR.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix (kind of) #26411.
- [x] These changes do not require tests because can't test FxR.
bors-servo added a commit that referenced this issue Jul 28, 2020
Make reload button clear the network cache.

The developer workflow in FxR is frustrating right now because of bugs like #24385. To allow us to put out a new release soon that addresses this papercut, these changes make the reload button clear the network cache in FxR.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix (kind of) #26411.
- [x] These changes do not require tests because can't test FxR.
bors-servo added a commit that referenced this issue Jul 28, 2020
Make reload button clear the network cache.

The developer workflow in FxR is frustrating right now because of bugs like #24385. To allow us to put out a new release soon that addresses this papercut, these changes make the reload button clear the network cache in FxR.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix (kind of) #26411.
- [x] These changes do not require tests because can't test FxR.
bors-servo added a commit that referenced this issue Jul 28, 2020
Make reload button clear the network cache.

The developer workflow in FxR is frustrating right now because of bugs like #24385. To allow us to put out a new release soon that addresses this papercut, these changes make the reload button clear the network cache in FxR.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix (kind of) #26411.
- [x] These changes do not require tests because can't test FxR.
bors-servo added a commit that referenced this issue Jul 28, 2020
Make reload button clear the network cache.

The developer workflow in FxR is frustrating right now because of bugs like #24385. To allow us to put out a new release soon that addresses this papercut, these changes make the reload button clear the network cache in FxR.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix (kind of) #26411.
- [x] These changes do not require tests because can't test FxR.
bors-servo added a commit that referenced this issue Jul 28, 2020
Make reload button clear the network cache.

The developer workflow in FxR is frustrating right now because of bugs like #24385. To allow us to put out a new release soon that addresses this papercut, these changes make the reload button clear the network cache in FxR.

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix (kind of) #26411.
- [x] These changes do not require tests because can't test FxR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.