Closed Bug 1075620 Opened 5 years ago Closed 5 years ago

Switch to GET for fetch to allow caching of links data from redirect

Categories

(Firefox :: New Tab Page, defect)

33 Branch
defect
Not set
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
35.3
Tracking Status
firefox32 --- unaffected
firefox33 + fixed
firefox34 + fixed
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

Attachments

(2 files, 2 obsolete files)

[Tracking Requested - why for this release]: We'll end up unnecessarily sending TBs of cacheable data for Release users once tiles is enabled. It's currently disabled for 33 by bug 1073823, so we won't hit this yet. I understand the last 33 beta is happening tomorrow, so I'll try to fix this today although it would be nice to test on Nightly first.

oyiptong noticed that we're sending a ton of data (hundreds of GB a day?) for fetch requests for just beta users, and turns out it's because we POST the fetch request where the server returns a redirect to a links file, and Firefox invalidates cache entry for subsequent gets in this situation.

The cache headers sent by the redirected endpoint are invalidated:
http://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#6117

The server added the GET functionality:
https://github.com/mozilla/onyx/commit/28796349e6c88ab8381c7707e7763f90c945ed15
Flags: firefox-backlog+
Flags: qe-verify?
Attached patch wip (obsolete) — Splinter Review
Is there an endpoint that I can test this with?
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #8498267 - Flags: feedback?(oyiptong)
oyiptong, is switching to GET necessary or does it even help? Is it actually that we need to add If-Modified-Since header to our POST?


current POST:

x = new XMLHttpRequest(); x.open("POST", "https://tiles.up.mozillalabs.com/v2/links/fetch", false); x.send('{"locale":"en-US"}'); console.log(x.status, x.statusText, x.getAllResponseHeaders().replace(/\r/g, ""))

200 "OK" "Content-Type: application/octet-stream
Content-Length: 216040
Connection: keep-alive
Date: Sun, 28 Sep 2014 14:59:41 GMT
Last-Modified: Thu, 28 Aug 2014 00:57:06 GMT
Etag: "9da3094dbe9ee89cbc70070142daea5b"
Accept-Ranges: bytes
Server: AmazonS3
Age: 13118
X-Cache: Hit from cloudfront
Via: 1.1 587d7cc9256adbdfe54523e1d39a50bb.cloudfront.net (CloudFront)
X-Amz-Cf-Id: 7XsZNyAZW8V6ZwpE2HQyrn91t7mRt_Od_gwZ2jmP_cKef-4ZYWdN_A==


new GET directly to cloudfront:

x = new XMLHttpRequest(); x.open("GET", "https://d1tiksivlekcfk.cloudfront.net/directoryLinks.json", false); /*x.setRequestHeader("If-Modified-Since", "Thu, 28 Aug 2014 00:57:06 GMT");*/ x.send('{"locale":"en-US"}'); console.log(x.status, x.statusText, x.getAllResponseHeaders().replace(/\r/g, ""))

200 "OK" "Content-Type: application/octet-stream
Content-Length: 216040
Connection: keep-alive
Date: Sun, 28 Sep 2014 14:59:41 GMT
Last-Modified: Thu, 28 Aug 2014 00:57:06 GMT
Etag: "9da3094dbe9ee89cbc70070142daea5b"
Accept-Ranges: bytes
Server: AmazonS3
Age: 13234
X-Cache: Hit from cloudfront
Via: 1.1 69dc5215907ea1f94133bedbe807deb4.cloudfront.net (CloudFront)
X-Amz-Cf-Id: upw7rvIcsEqLMvXADx_zXvxm7lGOBcUwb-S8LSR7HzRuHVNo1F9j8g==


subsequent GET to cloudfront:

200 "OK" "Content-Type: application/octet-stream
Content-Length: 216040
Connection: keep-alive
Date: Sun, 28 Sep 2014 14:59:41 GMT
Last-Modified: Thu, 28 Aug 2014 00:57:06 GMT
Etag: "9da3094dbe9ee89cbc70070142daea5b"
Accept-Ranges: bytes
Server: AmazonS3
Age: 13287
X-Cache: Hit from cloudfront
Via: 1.1 69dc5215907ea1f94133bedbe807deb4.cloudfront.net (CloudFront)
X-Amz-Cf-Id: lQEpus6-ytUawvkuk-CXHO46WS8dI9k6TqRBBpza86sd2N_arCW9ow==


However, if I add in If-Not-Modified-Since to the current POST endpoint:

x = new XMLHttpRequest(); x.open("POST", "https://tiles.up.mozillalabs.com/v2/links/fetch", false); x.setRequestHeader("If-Modified-Since", "Thu, 28 Aug 2014 00:57:06 GMT"); x.send('{"locale":"en-US"}'); console.log(x.status, x.statusText, x.getAllResponseHeaders().replace(/\r/g, ""))

304 "Not Modified" "Connection: keep-alive
Date: Wed, 01 Oct 2014 22:09:51 GMT
Etag: "9da3094dbe9ee89cbc70070142daea5b"
Server: AmazonS3
Age: 13337
X-Cache: Hit from cloudfront
Via: 1.1 69dc5215907ea1f94133bedbe807deb4.cloudfront.net (CloudFront)
X-Amz-Cf-Id: v6IZaIZHFSaU7iTMOVe2-a55nWso9bsyMdyk6PJpEXf1Un3rbfAQeg==
Flags: needinfo?(oyiptong)
Ha actually, I should have looked more closely at the subsequent GET output:

x = new XMLHttpRequest(); x.open("GET", "https://d1tiksivlekcfk.cloudfront.net/directoryLinks.json", false); x.send(); console.log(x.status, x.statusText, x.getAllResponseHeaders().replace(/\r/g, ""))

200 "OK" "Content-Type: application/octet-stream
Content-Length: 216040
Connection: keep-alive
Date: Sun, 28 Sep 2014 14:59:41 GMT
Last-Modified: Thu, 28 Aug 2014 00:57:06 GMT
Etag: "9da3094dbe9ee89cbc70070142daea5b"
Accept-Ranges: bytes
Server: AmazonS3
Age: 13664
X-Cache: Hit from cloudfront
Via: 1.1 d13aa395c8a3e4eca4e94319e2655f1e.cloudfront.net (CloudFront)
X-Amz-Cf-Id: j6Qr13rRXyL1xJPko0p82FjOtVY3sV5Mr5xO_tVWEOmdo2C2p8jytw==


It's responding with the exact same date, so I'm guessing it's hitting the Firefox cache and replaying the same 200 status.
Flags: needinfo?(oyiptong)
Just to be complete, I set up a dummy server that always responds 303: s.send_header("Location", "https://dtex4kvbppovt.cloudfront.net/STAR/en-US.4868533e4d1b515ae58dfa7b96adff01b0eb9409.json")

POST: results in Connection: keep-alive and about:cache's fetch count for en-US.4868533e4d1b515ae58dfa7b96adff01b0eb9409.json does not change.

200 "OK" "Content-Type: application/json
Content-Length: 399307
Connection: keep-alive
Date: Fri, 26 Sep 2014 20:32:39 GMT
Cache-Control: max-age=31536000
Last-Modified: Thu, 25 Sep 2014 20:41:20 GMT
Etag: "b8af0b8a143d91caf211b9b813e02769"
Server: AmazonS3
Age: 441628
X-Cache: Hit from cloudfront
Via: 1.1 e2af8a85927835558866752f53562ecd.cloudfront.net (CloudFront)
X-Amz-Cf-Id: J9U73KwAhs6MnOgfwMMIIc5iIr9Qmsz3egPiuW-d8lqTj6GUU-8vfw==


GET: results in no keep-alive and about:cache fetch count increments by 1

200 "OK" "Content-Type: application/json
Content-Length: 399307
Date: Fri, 26 Sep 2014 20:32:39 GMT
Cache-Control: max-age=31536000
Last-Modified: Thu, 25 Sep 2014 20:41:20 GMT
Etag: "b8af0b8a143d91caf211b9b813e02769"
Server: AmazonS3
Age: 440442
X-Cache: Hit from cloudfront
Via: 1.1 107fa1bd02fde2bfcae7dabbfe0b0205.cloudfront.net (CloudFront)
X-Amz-Cf-Id: bGjPlJ9mFmjTZ18evUKPCO0J7KbgzBFXC0M-MyHKnF-DRouU7VeFSQ==
oyiptong, is there a good way to test that we don't run into the opposite problem of Firefox always hitting the cache even if there's an update? Will the file name change for each update? E.g., v2/links/fetch/en-US would 303 to en-US.4868533e4d1b515ae58dfa7b96adff01b0eb9409.json now then 303 to a different file name later?
Ed, go to build for beta 9 will happen in a few hours. could you explain your plan? :) Thanks
Flags: needinfo?(edilee)
I would feel better with some baking on nightly35 before uplift to aurora34 or beta33. Given that beta33 already has tiles turned off, it's not super critical to land now other than avoiding an uplift to release33 when tiles is turned on for that channel.

The server endpoint isn't live yet, so I'm not entirely sure if the client changes fixes the problem (although initial dev testing seems to confirm fixed). But it would also be difficult for other people to verify the fix as well right now.
Flags: needinfo?(edilee)
Attached patch v1 (obsolete) — Splinter Review
adw has been the only reviewer for DirectoryLinksProvider.jsm.

This change switches from a POST with locale passed in the body post data to GET with locale in the url. Firefox does not cache redirected GETs from POSTs, so the new GET endpoint will 303 to the same redirect GET from before but Firefox will now cache that.
Attachment #8498267 - Attachment is obsolete: true
Attachment #8498267 - Flags: feedback?(oyiptong)
Attachment #8500626 - Flags: review?(ttaubert)
Comment on attachment 8500626 [details] [diff] [review]
v1

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

::: browser/modules/DirectoryLinksProvider.jsm
@@ +183,5 @@
>    },
>  
>    _fetchAndCacheLinks: function DirectoryLinksProvider_fetchAndCacheLinks(uri) {
> +    // Replace with the same display locale used for selecting links data
> +    uri = uri.replace("%LOCALE%", this.locale);

Shouldn't we use nsURLFormatter.formatURLPref()? Or does our locale selection have different requirements than what that gives us?

::: browser/modules/test/xpcshell/test_DirectoryLinksProvider.js
@@ +293,4 @@
>    yield DirectoryLinksProvider.init();
>    yield cleanJsonFile();
>    // this must trigger directory links json download and save it to cache file
> +  yield DirectoryLinksProvider._fetchAndCacheLinks(kExampleURL + "%LOCALE%");

Shouldn't this change the pref rather than calling into a private method?
Attachment #8500626 - Flags: review?(ttaubert) → feedback+
(In reply to Tim Taubert [:ttaubert] from comment #10)
> > +    uri = uri.replace("%LOCALE%", this.locale);
> Shouldn't we use nsURLFormatter.formatURLPref()? Or does our locale
> selection have different requirements than what that gives us?
That's what I originally had in the wip patch, but I'm not entirely sure how |this.locale| and formatURLPref can differ. I was even thinking about changing this.locale to use something like formatUrl("%LOCALE%") to read out the value to use.

From my quick research, I didn't use formatUrl because that would use the Firefox built locale instead of potentially something else that the user chooses for display.

Either way, we'll want to be consistent in the locale we send to the server and the locale we use to key off data returned from the server.

> > +  yield DirectoryLinksProvider._fetchAndCacheLinks(kExampleURL + "%LOCALE%");
> Shouldn't this change the pref rather than calling into a private method?
Yeah, it seems that there's a helper to do just that:
promiseDirectoryDownloadOnPrefChange
Attached patch v1.1Splinter Review
This keeps the |this.locale| which does the same thing as the addon manager in using the OS locale if pref set then falling back to Firefox locale.

AddonManager: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/AddonManager.jsm#246
UrlFormatter: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/urlformatter/nsURLFormatter.js#90

Also adds cleanup to the pref change observer instead of relying on promiseCleanDirectoryLinksProvider to be called.
Attachment #8500626 - Attachment is obsolete: true
Attachment #8500769 - Flags: review?(ttaubert)
Given what comment #0 says in terms of traffic, we should try to QA this if possible.
Flags: qe-verify? → qe-verify+
QA Contact: cornel.ionce
I'd want to ensure that we've done and end to end test with the final end point that is going to be used in the go-live release for tiles. There is some time here, but not a lot of time. In general, the sooner we can do such an end to end test the better.

Karl, is there a final endpoint to test against?
Flags: needinfo?(kthiessen)
Redirecting Clint's question in comment 14 to :mostlygeek, who is in charge of the production deployment.
Flags: needinfo?(bwong)
Flags: needinfo?(kthiessen)
(In reply to Clint Talbert ( :ctalbert ) from comment #14)
> is there a final endpoint to test against?
https://tiles.services.mozilla.com/v2/links/fetch/en-US
Flags: needinfo?(bwong)
Attachment #8500769 - Flags: review?(ttaubert) → review+
Blocks: 1050643
https://hg.mozilla.org/mozilla-central/rev/94a4e0128424
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Iteration: --- → 35.3
I've verified that the "browser.newtabpage.directory.source" pref is changed to "https://tiles.services.mozilla.com/v2/links/fetch/%LOCALE%" using latest Nightly, build ID: 20141015030202.

I'm not sure if and how I could verify the traffic.
Ed, do you have any idea of how I could test it?
Flags: needinfo?(edilee)
Is there a normal way of testing web traffic / caching? I did it through HTTP logging, but maybe using a tool outside of Firefox might be better. Many years ago, I've used things like wireshark although I'm not sure how it works with https.
Flags: needinfo?(edilee)
We tried to monitor this on our end but unfortunately we were unable to do it. Our IT guys were also unable to help us regarding this matter. 

Could someone on your side please verify this on the server as you previously did?
I will set up a staging server that we can use to monitor if caching is working correctly. Stay tuned.
I did some testing with a custom onyx endpoint and a custom tiles_index.json. 
It looks like the caching is working, though not quite what I expected. 

Test #1

  "GET /v2/links/fetch/en-US HTTP/1.1" 303 423
  "GET /test/STAR/en-US.4868533e4d1b515ae58dfa7b96adff01b0eb9409b.json HTTP/1.1" 200 399307

This successfully redirected to the data blobl

Test #2

  "GET /v2/links/fetch/en-US HTTP/1.1" 303 423
  (no fetch)

I was expecting a request with If-Modified-Since, but FF doesn't send anything.

Tests #3, #4 same results as #2.

Test #5 - changing the tile-index.json to point at new data blob

  "GET /v2/links/fetch/en-US HTTP/1.1" 303 423
  "GET /test/STAR/en-US.4868533e4d1b515ae58dfa7b96adff01b0eb9409c.json HTTP/1.1" 200 399307 

It successfully pulls down the new file 

Test #6, #7
  
  "GET /v2/links/fetch/en-US HTTP/1.1" 303 423
  "GET /test/STAR/en-US.4868533e4d1b515ae58dfa7b96adff01b0eb9409c.json HTTP/1.1" 304 0
  "GET /v2/links/fetch/en-US HTTP/1.1" 303 423
  "GET /test/STAR/en-US.4868533e4d1b515ae58dfa7b96adff01b0eb9409c.json HTTP/1.1" 304 0

Firefox properly fetches and gets a 304 response back. 

So it seems a little buggy though caching seems to be working.
(In reply to Benson Wong [:mostlygeek] from comment #23)
>   "GET /v2/links/fetch/en-US HTTP/1.1" 303 423
>   (no fetch)
vs
>   "GET /v2/links/fetch/en-US HTTP/1.1" 303 423
>   "GET /test/STAR/en-US.4868533e4d1b515ae58dfa7b96adff01b0eb9409c.json
> HTTP/1.1" 304 0
I'm not familiar enough with what situations Firefox serves content directly from its own cache or not. But either situation is better than Firefox redownloading the whole json each tile.

One thing you can check if you still have that profile open:
1) load about:cache
2) click "List Cache Entries" under "disk"
3) search for the .json cache entries
4) check the response-head for the case where Firefox uses its own cache vs checking for 304 -- the output should look like..

response-head: 	HTTP/1.1 200 OK 
Content-Type: application/json 
Content-Length: 399307 
Date: Fri, 26 Sep 2014 20:32:39 GMT 
Cache-Control: max-age=31536000 
Last-Modified: Thu, 25 Sep 2014 20:41:20 GMT 
Etag: "b8af0b8a143d91caf211b9b813e02769" 
Server: AmazonS3 
Age: 1194478 
X-Cache: Hit from cloudfront 
Via: 1.1 5a4c25dc864206e0083c239ad3cd0113.cloudfront.net (CloudFront) 
X-Amz-Cf-Id: 4svPgzhmtUEZdAPB-40nInTmI2bxG5cuwSeKcKbvds5iHRQO5h7ZBw==
Interesting checking about:cache. It appears that FF by default gave it a default expiry time, looks to be about 20minutes. 

After that expiry time it will send a request and get back a 304. Nice.
Are we able to see that cloudfront has been returning 304s since last week? Or maybe bandwidth usage? If we do compare bandwidth, make sure to compare with Beta33 numbers when it was still enabled -- it was turned off on Beta Oct 1 by bug 1073823.
Blocks: 986521
Approval Request Comment
[Feature/regressing bug #]: Bug 986521
[User impact if declined]: Users will unnecessarily download unchanged links data every day
[Describe test coverage new/current, TBPL]: Added tests to check for GET with locale. Manual testing from client side and server side of Firefox caching.
[Risks and why]: Low - js changes to low-dependency code and baked on Nightly for a little bit
[String/UUID change made/needed]: none
Attachment #8508322 - Flags: approval-mozilla-release?
Attachment #8508322 - Flags: approval-mozilla-beta?
Comment on attachment 8500769 [details] [diff] [review]
v1.1

Approval Request Comment: See comment 27 for a?beta/release.

Only difference here is jsm+test have moved on aurora35/nightly36 from their original location on beta34/release33.
Attachment #8500769 - Flags: approval-mozilla-aurora?
Attachment #8508322 - Flags: approval-mozilla-release?
Attachment #8508322 - Flags: approval-mozilla-release+
Attachment #8508322 - Flags: approval-mozilla-beta?
Attachment #8508322 - Flags: approval-mozilla-beta+
Attachment #8500769 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: qe-verify+ → qe-verify-
QA Contact: cornel.ionce
You need to log in before you can comment on or make changes to this bug.