1380754 - about:addons does not let you switch from DNT disabled to DNT enabled

Status: RESOLVED DUPLICATE of bug 1381047

(Toolkit :: Add-ons Manager, defect)

Description

[Kumar McMillan [:kumar]]

Steps to reproduce on Firefox desktop (Nightly or Release) :

- In about:config, change the host of extensions.webservice.discoverURL to be https://discovery.addons.allizom.org (AMO stage). Leave the rest of the URL in tact.
- Go to about:preferences and make sure DNT (Do Not Track) is not enabled
- Go to about:addons
- Take note that a request is made to https://www.google-analytics.com/analytics.js and to https://www.google-analytics.com/r/collect which is expected
- Go back to about:preferences and enable DNT
- Switch tabs to about:addons and do a hard refresh

What happened:

I see a request to https://www.google-analytics.com/analytics.js but not to https://www.google-analytics.com/r/collect

What I expected:

No requests should be made to https://www.google-analytics.com at all

Additional info:

- If you restart the browser, everything works as expected
- If you replay about:addon's exact request to https://discovery.addons.allizom.org (headers and all) using curl, you can see that the server is doing what it should: it does add a <script> tag to google analytics when the DNT header is set to 1
- Some kind of weird caching is happening but it's not obvious to me
- Disabling google analytics for DNT was added in https://github.com/mozilla/addons-frontend/issues/2792

Comment 1

[Kumar McMillan [:kumar]]

(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #0)
> - If you replay about:addon's exact request to
> https://discovery.addons.allizom.org (headers and all) using curl, you can
> see that the server is doing what it should: it does add a <script> tag to
> google analytics when the DNT header is set to 1

That was a typo. Correction: [the server] does *not* add a <script> tag to google analytics when the DNT header is set to 1

...which is why I think this is something that needs to be fixed in about:addons (unless we need to adjust the response headers somnehow?)

Comment 2

[Kumar McMillan [:kumar]]

The DNT changes are now live in production so you don't have to change extensions.webservice.discoverURL to reproduce this.

One thing I forgot to mention is that I can see the expected DNT behavior by loading https://discovery.addons.mozilla.org/en-US/firefox/discovery/pane/46.0/Darwin/normal directly in a normal tab which is why I suspect some special caching is happening on about:addons.

Comment 3

[tofumatt [:tofumatt]]

It's possible this is caused by nginx not varying on the DNT header, which Stuart and I chatted about this morning.

A patch added to cloudops gets the Disco proxy not to cache based on DNT but we need to `Vary` on `DNT` as well as `Accept-Encoding` for both AMO and Disco Pane.

Comment 4

[Stuart Colville [:muffinresearch]]

(In reply to tofumatt [:tofumatt] from comment #3)
> It's possible this is caused by nginx not varying on the DNT header, which
> Stuart and I chatted about this morning.
> 
> A patch added to cloudops gets the Disco proxy not to cache based on DNT but
> we need to `Vary` on `DNT` as well as `Accept-Encoding` for both AMO and
> Disco Pane.

FWIW The vary header change should only be necessary for addons-frontend and *not* addons-server because addons-server does the DNT logic purely client side. 

Currently the disco pane nginx cache correctly serves different responses when DNT changes but the browser (and any other cache downstream) can cache the responses and doesn't know to go back to the origin without us varying on DNT in the response. 

The behaviour seen looking at the source from Comment1 might be because at some point the browser is going back to origin unless there's an additional bug, once we have the vary in place for addons-frontend on stage and can test it we should be able to confirm.

Comment 5

[Stuart Colville [:muffinresearch]]

Current plan is to look to get this patch tested and into production on Monday.

Comment 6

[Kumar McMillan [:kumar]]

There may be several things at play here but aswan verified yesterday (on IRC) that about:addons is indeed not respecting a hard-refresh the same way a normal tab would, so that's definitely a bug. Rather than pass this bug over to ops I'd rather see a separate bug opened as a dependency for resolving the Vary header issue (although I'm still trying to understand how that comes into play).

Comment 7

[Kumar McMillan [:kumar]]

I find it really hard to see how this bug is a client side issue in addons-frontend. Here's why:

* When you reproduce the bug above, the main symptom is that https://www.google-analytics.com/analytics.js gets loaded -- this is controlled by a script tag delivered via HTML from the first server response
* If you inspect the response that delivered the script tag (which includes DNT:1), you can see <script...> in the response using devtools
* However, when you replay the same request with curl using the same exact headers, the script tag is not there
* Devtools also shows that the response is cached which it shouldn't be, since the page was hard-refreshed
* This same bug cannot be reproduced in a normal tab (i.e. outside of about:addons) which makes me doubt that this is an addons-frontend issue
* As an aside, jason mentioned that nginx (for the Discovery Pane) doesn't use Vary for caching, it has a custom cache key: https://github.com/mozilla-services/cloudops-deployment/blob/715617ef6b8ee5ac07ba475b134a3a9266c33037/projects/amo/puppet/modules/amo_proxy/templates/nginx.discovery.conf.erb#L59

Comment 8

[Kumar McMillan [:kumar]]

After discussion with Stuart we're going to keep this bug in Toolkit since it's specifically an issue with about:addons. We may have other bugs relating to the server but we will file them separately.

Comment 9

[Kumar McMillan [:kumar]]

Aha, we did discover a server change that needs to be made: bug 1381046 . When that's fixed, the STR will no longer help reproduce the about:addons caching issue so I refiled it as bug 1381047

Comment 10

[Kumar McMillan [:kumar]]

FYI, the STR in this bug will not be possible after this patch deploys https://github.com/mozilla/addons-frontend/issues/2806