1474238 - Add a "report breakage" dialog for Tracking Protection

Status: VERIFIED FIXED

(Firefox :: Protections UI, enhancement, P2)

Description

[Johann Hofmann [:johannh]]

We would like to enable users to report sites that are presumably broken because of Tracking Protection, in the control center.

UI mockups are here: https://mozilla.invisionapp.com/share/KPIYY4DVHXS#/screens/301705297

What we would like to collect is outlined here: https://docs.google.com/document/d/1evZ2LnQdkRFZDU02BSsZbPq2isr6KGx82KH4mBhWaHw/edit

The initial plan is to do a prototype that uses the same reporting endpoint as the web-based WebCompat reporter uses (just with a different, probably private, repository for the reports).

We plan to enable this at least for Nightly, potentially for Beta, probably not for Release.

Comment 1

[Johann Hofmann [:johannh]]

Mike, we talked about this in SF and we decided that we'd like to start off using an endpoint from your team for our breakage reporting backend.

As mentioned we'd like to have this UI in-product and do a POST request to an endpoint that creates an issue in a repository with the breakage data.

Is there anything you need from me to be able to set this up?

Thank you!

Comment 3

[Mike Taylor [:miketaylr]]

(In reply to Johann Hofmann [:johannh] from comment #1)
> As mentioned we'd like to have this UI in-product and do a POST request to
> an endpoint that creates an issue in a repository with the breakage data.

Hey Johann, thanks for filing a bug! We're planning stuff for Q3 but this is one of our priorities. Just to clarify, should the repository be a) private and b) separate from existing webcompat issues (https://github.com/webcompat/web-bugs/issues)?

Comment 4

[Johann Hofmann [:johannh]]

(In reply to Mike Taylor [:miketaylr] (62 Regression Engineering Owner)(PTO July 12, 13, 16, 20, 23) from comment #3)
> (In reply to Johann Hofmann [:johannh] from comment #1)
> > As mentioned we'd like to have this UI in-product and do a POST request to
> > an endpoint that creates an issue in a repository with the breakage data.
> 
> Hey Johann, thanks for filing a bug! We're planning stuff for Q3 but this is
> one of our priorities. Just to clarify, should the repository be a) private
> and b) separate from existing webcompat issues
> (https://github.com/webcompat/web-bugs/issues)?

Yes, both private and separate, if I remember correctly. :)

I'm probably going to start implementing this separately from your backend setup and put in a pref that can be set to the eventual URL of the service whenever we have it. Is there a place where I can find out more about the general structure of requests to the service (e.g. what the parameters are called and how exactly they need to be passed)?

Thanks!

Comment 5

[Johann Hofmann [:johannh]]

Attachment: Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

Review commit: https://reviewboard.mozilla.org/r/256234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256234/

Comment 6

[Johann Hofmann [:johannh]]

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

Comment 7

[Johann Hofmann [:johannh]]

Attachment: WIP State

This patch is relatively complete on the feature side (it just needs tests), and we still have to figure out a bunch of things, including:

- What's the endpoint we can send reports to?
- How should the request be formatted?
- Can we manage to get a list of blocked resources to submit (or should we defer that to a follow-up bug?)
- Do we need to mention all the things we're sending to the user (including the TP prefs)?
- What goes into the "Learn More" URL?
- What are the exact categories for the "observed breakage" radio group.

Comment 8

[Johann Hofmann [:johannh]]

- Do we want to send the user agent string or some other platform/Fx version combination?
- Do we want to send the full URL or only the hostname?

Comment 9

[Mike Taylor [:miketaylr]]

(In reply to Johann Hofmann [:johannh] from comment #7)
> Created attachment 8991318 [details]
> WIP State
> 
> This patch is relatively complete on the feature side (it just needs tests),
> and we still have to figure out a bunch of things, including:
> 
> - What's the endpoint we can send reports to?

We can one of 2 things: https://webcompat.com/api/trackingprotection/issues/new

(this doesn't exist yet...I've filed https://github.com/webcompat/webcompat.com/issues/2549)

Another option would be to stand up a dedicated app on heroku -- I built this for a SHIELD experiment which does basically what you want (create issues in a private repo):

https://github.com/mozilla/webcompat-blipz-experiment-server#how-it-works

> - How should the request be formatted?

(Note: I'll be working on something for this mid-next week -- have some PTO between now and then.)

Comment 10

[Tanvi Vyas[:tanvi]]

(In reply to Johann Hofmann [:johannh] from comment #7)
> - What's the endpoint we can send reports to?
Mike and Johann to work out

> - How should the request be formatted?
Mike to report back.

> - Can we manage to get a list of blocked resources to submit (or should we
> defer that to a follow-up bug?)
Lets do this in a followup bug with the help of Francois to identify where we can find such resources.

> - Do we need to mention all the things we're sending to the user (including
> the TP prefs)?
I will set up a meeting with Marshall to discuss.

> - What goes into the "Learn More" URL?
Michelle gave you a temporary URL - https://support.mozilla.org/en-US/kb/tracking-protection?redirectlocale=en-US&redirectslug=tracking-protection-pbm.  We will need a SUMO page for the Umbrella Term that includes links to the items under the umbrella.  Lets make a followup bug to replace the URL once the umbrella term is defined.

> - What are the exact categories for the "observed breakage" radio group.
Michelle can help with this and copy.

> - Do we want to send the user agent string or some other platform/Fx version
> combination?
Yes

> - Do we want to send the full URL or only the hostname?
Yes, at least for the top level page.  I'm not sure if we need the full URL of the blocked domains.

Comment 11

[Johann Hofmann [:johannh]]

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

Comment 12

[Johann Hofmann [:johannh]]

Comment on attachment 8991316 [details]
Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256234/diff/1-2/

Comment 13

[Johann Hofmann [:johannh]]

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

Comment 14

[Johann Hofmann [:johannh]]

Comment on attachment 8991316 [details]
Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256234/diff/2-3/

Comment 15

[:Paolo Amadini]

The patch looks good to me in general, I'll do a more detailed review tomorrow or Friday.

Comment 16

[Johann Hofmann [:johannh]]

Thanks! As long as you have no fundamental concerns there's no rush, the backend will be in place some time next week (I'd still like to land this asap and change the url pref whenever we have the backend, though).

Comment 17

[:Paolo Amadini]

Comment on attachment 8991316 [details]
Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

https://reviewboard.mozilla.org/r/256234/#review265102

I haven't reviewed the test yet, but in general it looks good.

::: browser/app/profile/firefox.js:1496
(Diff revision 3)
>  
>  pref("privacy.trackingprotection.ui.enabled", true);
>  pref("privacy.trackingprotection.introCount", 0);
>  pref("privacy.trackingprotection.introURL", "https://www.mozilla.org/%LOCALE%/firefox/%VERSION%/tracking-protection/start/");
>  
> +// TODO Bug ???? - Add a report url for this feature and turn it on.

File the bug and add its number before landing (no need to keep the "TODO" marker).

::: browser/base/content/browser-trackingprotection.js:49
(Diff revision 3)
> +      this.reportBreakageButton.toggleAttribute("hidden",
> +        !Services.prefs.getBoolPref(this.PREF_REPORT_BREAKAGE_ENABLED, false));

Sadly, this would set hidden="" instead of hidden="true", which would break styles that make assumptions on the "true" value being present, or code that forces getAttribute() to boolean instead of using hasAttribute().

At some point we may refactor XUL elements to work with attribute="" meaning true, to better align with HTML, but for the time being we should just have the usual if-else here.

::: browser/base/content/browser-trackingprotection.js:117
(Diff revision 3)
> +  backToMainView() {
> +    this.identityPopupMultiView.goBack();
> +  },
> +
> +  submitBreakageReport() {
> +    document.getElementById("identity-popup").hidePopup();

Use the PanelMultiView.hidePopup static method.

::: browser/base/content/browser-trackingprotection.js:119
(Diff revision 3)
> +  },
> +
> +  submitBreakageReport() {
> +    document.getElementById("identity-popup").hidePopup();
> +
> +    let reportURL = Services.prefs.getStringPref(this.PREF_REPORT_BREAKAGE_URL, null);

No need for a default value for preferences defined in "firefox.js".

::: browser/base/content/browser-trackingprotection.js:124
(Diff revision 3)
> +    let formData = new FormData();
> +
> +    // Also add current state of TP prefs.
> +    for (let pref of [this.PREF_ENABLED_GLOBALLY, this.PREF_ENABLED_IN_PRIVATE_WINDOWS]) {
> +      formData.append(pref, Services.prefs.getBoolPref(pref, false));
> +    }
> +
> +    // Add form data from the user interface, to make sure we report exactly what the
> +    // user is seeing in their submission form (particularly the URL could have changed).
> +    formData.append("url", this.reportBreakageURL.textContent);
> +    formData.append("userAgent", this.reportBreakageUA.textContent);

I'm not a fan of round-trips of data through UI elements. Either store the values on the object, or store a pre-built FormData and then submit it here.

In fact, I think we have functions to process URIs before displaying them, so for example Unicode domains are displayed properly. We may need to do this here, but only for the value in the controls, and not what is sumbitted.

Have you tested the display of very long URLs?

I guess "data:" URIs and other special URIs cannot be top-level URIs when this dialog can be displayed?

For good measure, I'd add a test for a long URI containing all the possible valid characters and also for IDNs, to check that the PostData encoding supports these cases properly.

(Of course, regardless of what we do here, the server side would still be responsible for correct decoding and sanitization, like filtering "data:" and "javascript:" URIs, and preventing attacks on the tooling we use to display the reports.)

::: browser/base/content/browser-trackingprotection.js:144
(Diff revision 3)
> +      body: formData,
> +    }).then(function(response) {
> +      if (!response.ok) {
> +        Cu.reportError(`Tracking Protection Report to ${reportURL} failed with status ${response.status}`);
> +      }
> +    });

The "fetch" method can reject, so this needs a ".catch(Cu.reportError);" or equivalent.

::: browser/base/content/test/trackingUI/browser_trackingUI_report_breakage.js:186
(Diff revision 3)
> +
> +    await popuphidden;
> +  });
> +
> +  // Stop the server.
> +  await new Promise(r => server.stop(() => r()));

nit: "r => server.stop(r)" should do.

::: browser/components/controlcenter/content/panel.inc.xul:241
(Diff revision 3)
> +          <label id="identity-popup-breakageReportView-learn-more"
> +                 class="text-link">&trackingProtection.breakageReportView.learnMore;</label>

You forgot "subviewkeynav", but really, we should just fix bug 1473748.

::: browser/components/controlcenter/content/panel.inc.xul:247
(Diff revision 3)
> +                 class="text-link">&trackingProtection.breakageReportView.learnMore;</label>
> +        </vbox>
> +        <vbox id="identity-popup-breakageReportView-body" class="panel-view-body-unscrollable">
> +          <vbox class="identity-popup-breakageReportView-collection-section">
> +            <label class="identity-popup-breakageReportView-collection-label">&trackingProtection.breakageReportView.collection.url.label;</label>
> +            <label id="identity-popup-breakageReportView-collection-url"></label>

nit: use <label/> instead of <label></label>, here and below

::: browser/locales/en-US/chrome/browser/browser.dtd:933
(Diff revision 3)
>  <!ENTITY trackingProtection.reload2.label "Reload Page">
>  <!ENTITY trackingProtection.reload2.accesskey "R">
>  
> +<!ENTITY trackingProtection.openBreakageReportView.label "Report Problems">
> +<!ENTITY trackingProtection.breakageReportView.label "Report Problems">
> +<!ENTITY trackingProtection.breakageReportView.description "Tracking Protection can cause problems with some websites. Help improve Firefox by reporting problems when you see them.">

This and another string need branding.

::: browser/themes/shared/controlcenter/panel.inc.css:306
(Diff revision 3)
> +#identity-popup-breakageReportView-footer {
> +  display: flex;
> +}
> +
> +#identity-popup-breakageReportView-footer > button {
> +  flex: 1;
> +}
> +
> +#identity-popup-breakageReportView-footer > button[default] {
> +  color: white;
> +  background-color: #0996f8;
> +}
> +
> +#identity-popup-breakageReportView-footer > button[default]:hover {
> +  background-color: #0675d3;
> +}
> +
> +#identity-popup-breakageReportView-footer > button[default]:hover:active {
> +  background-color: #0568ba;
> +}

Don't we have shared classes for these buttons, or at least variables for the colors? If not, it's fine, but if we do, we should use them instead.

I recommend avoiding CSS flex layout when we can use box layout, and we can certainly do this here by copying other subview footer buttons. We may also need a toolbarseparator to keep the structure similar.

Comment 18

[Mike Taylor [:miketaylr]]

(In reply to Johann Hofmann [:johannh] from comment #7)
> - What's the endpoint we can send reports to?

https://tracking-protection-issues.herokuapp.com/new

> - How should the request be formatted?

POST to https://tracking-protection-issues.herokuapp.com/new

The endpoint is expecting 3 POST variables:

title: a string (required)
body: a string (required)
labels: a comma delimited string (optional)

Here's some JS that will create an issue:

var f = new FormData();
f.set("title", "sup");
f.set("body", "whatever\nyou\want\here");
f.set("labels", "labels, like, this");

fetch('https://tracking-protection-issues.herokuapp.com/new', {
    method: 'POST',
    body: f
}).then(res => console.log(res))

And these get created in a private GitHub repo. I'll add you to the team that has access (and whoever else needs it, just let me know).

Comment 19

[Mike Taylor [:miketaylr]]

(In reply to Mike Taylor [:miketaylr] (62 Regression Engineering Owner) from comment #18)
> title: a string (required)

I forgot to mention, it's probably useful if you send the domain and breakage category as part of the title, e.g. something like:

google.com - <observed breakage category>

Comment 20

[François Marier [:francois]]

(In reply to Mike Taylor [:miketaylr] (62 Regression Engineering Owner) from comment #19)
> google.com - <observed breakage category>

Firefox doesn't know about categories, but we could specify whether it's the basic list or the strict list:

  google.com - strict list

Comment 21

[Mike Taylor [:miketaylr]]

, (In reply to François Marier [:francois] from comment #20)

> Firefox doesn't know about categories, but we could specify whether it's the
> basic list or the strict list:

(Ah, I just meant one of the radio list options from the "Observed Breakage" header in the gif)
 
>   google.com - strict list

That could be good too!

Comment 22

[Thomas Elin [:relaas]]

Hi Tanvi!

What kind of QA has been performed or is planned for this work in Nightly 63? Did the team file a PI request?

Comment 23

[Johann Hofmann [:johannh]]

Comment on attachment 8991316 [details]
Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

https://reviewboard.mozilla.org/r/256234/#review265102

Thank you for the detailed review. I've updated the patch with your suggestions and made two more notable changes:

While I was on PTO, the team was tasked to think about what options to give the user in the radio group. They found it rather hard to come up with a fixed set, so now we have an input field instead (spam ahoy).

The other major change in the new revision is that per Mike's comments we need to submit one big "body" field instead of smaller fields of data now.


This is still preffed off by default, because we're pending a conversation with legal. After that I will pref it on in Nightly and I'll also file a security review request which should probably block riding this further along the train.

Thanks!

> File the bug and add its number before landing (no need to keep the "TODO" marker).

This has a report URL now but we're still pending legal sign-off and a security review, and since these are moco-only bugs I'll just leave this without comment I think.

> Sadly, this would set hidden="" instead of hidden="true", which would break styles that make assumptions on the "true" value being present, or code that forces getAttribute() to boolean instead of using hasAttribute().
> 
> At some point we may refactor XUL elements to work with attribute="" meaning true, to better align with HTML, but for the time being we should just have the usual if-else here.

Ah, great point, thanks.

> Use the PanelMultiView.hidePopup static method.

I didn't really understand this point. Wouldn't I have to pass in the element to that static call anyway? Can you give me an example of what you meant? We're already doing this kind of call in the same file, I guess you want me to replace both usages?

> I'm not a fan of round-trips of data through UI elements. Either store the values on the object, or store a pre-built FormData and then submit it here.
> 
> In fact, I think we have functions to process URIs before displaying them, so for example Unicode domains are displayed properly. We may need to do this here, but only for the value in the controls, and not what is sumbitted.
> 
> Have you tested the display of very long URLs?
> 
> I guess "data:" URIs and other special URIs cannot be top-level URIs when this dialog can be displayed?
> 
> For good measure, I'd add a test for a long URI containing all the possible valid characters and also for IDNs, to check that the PostData encoding supports these cases properly.
> 
> (Of course, regardless of what we do here, the server side would still be responsible for correct decoding and sanitization, like filtering "data:" and "javascript:" URIs, and preventing attacks on the tooling we use to display the reports.)

I'm using asciiSpec for the titles for simplicity, so unicode domains are not an issue here.

Display of long URLs was indeed broken, thanks for noticing that.

data: and about: URIs will not get the TP UI.

For FormData I don't think it's our job to make sure that it can handle unicode characters correctly (manual testing shows it can). This should be done by web platform tests (and presumably it is).

Yes, the server side is responsible for sanitizing this, there's no point to do it on the client (since anyone could dig into about:config and send a malformed request to the server). As I mentioned, I was planning to file a bug for a security review for this.

> Don't we have shared classes for these buttons, or at least variables for the colors? If not, it's fine, but if we do, we should use them instead.
> 
> I recommend avoiding CSS flex layout when we can use box layout, and we can certainly do this here by copying other subview footer buttons. We may also need a toolbarseparator to keep the structure similar.

We don't, unfortunately, which could be a good follow-up cleanup. The other implementations of two footer buttons (bookmark panel and permission doorhangers) use CSS flex layout as well.

Comment 24

[Johann Hofmann [:johannh]]

Comment on attachment 8991316 [details]
Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256234/diff/3-4/

Comment 25

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76e0771390c1942cb14ba55de308259140bcc4fa

Comment 26

[Johann Hofmann [:johannh]]

(In reply to Thomas Elin [:relaas] from comment #22)
> Hi Tanvi!
> 
> What kind of QA has been performed or is planned for this work in Nightly
> 63? Did the team file a PI request?

Since this was part of the Privacy UI redesign project, it should generally be covered by the FastBlock/Content Blocking PI request. Cristian is the contact for this, IIUC.

Comment 27

[Johann Hofmann [:johannh]]

Cancelling review while we're in a conversation over backend reporting solutions and I don't want to waste Paolo's time on this.

Comment 28

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d8f50e27a05eb88448a352f8cbcff712d36cb9e

Comment 29

[Johann Hofmann [:johannh]]

Comment on attachment 8991316 [details]
Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

We talked about how to best record the reported breakage and came to the following agreement:

In general and particularly for non-Nightly releases, we want to use Firefox Telemetry with custom pings for collecting this data. It gives us better control of the data (and better trust for our users) and allows for more flexible querying.

However, given that the private GitHub repo based collection mechanism is already in place, we want to start off by using this in Nightly only, while implementing Telemetry support in a follow-up bug. This way we can analyze the shape and volume of the earliest reports from Nightly to help us build the right solution using Telemetry that we can then ship to everyone.

We received legal sign-off on this plan.

Francois, do you need anything in particular to give this data collection a data review?

Comment 30

[:Paolo Amadini]

Comment on attachment 8991316 [details]
Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

https://reviewboard.mozilla.org/r/256234/#review268530

::: browser/base/content/test/trackingUI/browser_trackingUI_report_breakage.js:22
(Diff revision 4)
> +  let promisePanelOpen = BrowserTestUtils.waitForEvent(gIdentityHandler._identityPopup, "popupshown");
> +  gIdentityHandler._identityBox.click();
> +  return promisePanelOpen;

nit: this works because waitForEvent by design waits for an extra tick, but for new code I'd still recommend waiting for the ViewShown event on the main view.

::: browser/base/content/test/trackingUI/browser_trackingUI_report_breakage.js:190
(Diff revision 4)
> +        }
> +
> +        Assert.deepEqual(sections, [
> +          "",
> +          "Content-Disposition: form-data; name=\"title\"\r\n\r\ntracking.example.org\r\n",
> +          `Content-Disposition: form-data; name="body"\r\n\r\nFull URL: ${reportURL}\r\nuserAgent: ${reportUA}\r\nPrivate Browsing: false\r\n\r\n**Preferences**\r\n${prefsBody}\r\n**Comments**\r\nThis is a comment\r\n`,

nit: I'd prefer if we wrapped this like:

  "String\r\n" +
  "String\r\n\r\n" +
  "String\r\n"

I guess it's our test HTTP server that changes the line ending style? Anyways, not particularly relevant as long as the test passes on all platforms.

Comment 31

[François Marier [:francois]]

Comment on attachment 8991316 [details]
Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

(In reply to Johann Hofmann [:johannh] - slow to respond, digging out of PTO backlog from comment #29)
> Francois, do you need anything in particular to give this data collection a
> data review?

Just the standard data review request form: https://github.com/mozilla/data-review/blob/master/request.md

Comment 32

[Johann Hofmann [:johannh]]

Attachment: Data Review Request

Comment 33

[François Marier [:francois]]

Comment on attachment 8998569 [details]
Data Review Request

> Website URL: Category 3-4

Could you also specify whether we do any filtering of the URL (e.g. removing the query string)?

I would strongly suggest we remove the query string since on badly-designed sites, that could leak session keys, auth tokens, passwords.

> Whether the user is in private mode: Category 2

Do we need to know this?

In general, we don't collect any data while users are in Private Browsing. In this, case, since it's the user's decision to file a report from Private Browsing and we clearly state that we're collecting the URL, then it seems reasonable to assume that users are aware that we're going to collect this data. 

On the other hand, some people may consider the use of Private Browsing itself as a sensitive topic and not expect that their report will leak this fact too.

Unless we have a strong use case for this flag, I would suggest removing it.

> Content Blocking-related user preferences: Category 2

Are we going to document the data collected somewhere or is this request it? If so, let's expand this to include what exactly we are looking at (or link to the code).

Comment 34

[Johann Hofmann [:johannh]]

Comment on attachment 8991316 [details]
Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256234/diff/4-5/

Comment 35

[Johann Hofmann [:johannh]]

Attachment: Data Review Request v2

Made the changes and updated the data review request, thanks!

Comment 36

[François Marier [:francois]]

Comment on attachment 8998682 [details]
Data Review Request v2

> 5) List all proposed measurements and indicate the category of data collection 
> for each measurement, using the Firefox [data collection categories]
> (https://wiki.mozilla.org/Firefox/Data_Collection) on the Mozilla wiki.   
> 
> Website URL: Category 3-4
> User Agent String: Category 1
> Content Blocking-related user preferences: Category 2

And that's all we're recording, right? The DB will literally have 3 fields (plus, maybe a creation datetime and a row ID)?

There's no unique/ish identifier like client_id, email address, IP address, etc.? There's no free-form text field that the user can fill out? No radio buttons with the kind of breakage?

Comment 37

[Johann Hofmann [:johannh]]

Right, indeed, there's free-form input, sorry, I'll update the request...

Apart from that, no client id, no email address, no IP address stored.

Comment 38

[Johann Hofmann [:johannh]]

Attachment: Data Review Request v3

Comment 39

[François Marier [:francois]]

Comment on attachment 8998685 [details]
Data Review Request v3

Ok, one last thing (sorry I meant to include it last time but I forgot): what about data retention?

We should have something about how long we're going to keep the reports in our DB. 6 months? 1 year? longer? Do we have a way to automatically expire reports on GitHub or do we need to write something to purge old data?

I'd suggest starting with 6 or 12 months. That should give us plenty of time to examine the reports. After that, many of the reports will no longer be relevant.

Comment 40

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0494a724bfc9e0eebf59925f66a4f4f8c1b6b743

Comment 41

[Johann Hofmann [:johannh]]

(In reply to François Marier [:francois] from comment #39)
> Comment on attachment 8998685 [details]
> Data Review Request v3
> 
> Ok, one last thing (sorry I meant to include it last time but I forgot):
> what about data retention?
> 
> We should have something about how long we're going to keep the reports in
> our DB. 6 months? 1 year? longer? Do we have a way to automatically expire
> reports on GitHub or do we need to write something to purge old data?
> 
> I'd suggest starting with 6 or 12 months. That should give us plenty of time
> to examine the reports. After that, many of the reports will no longer be
> relevant.

Honestly you probably know more about this than me. I think we can delete the reports from GitHub (and since this is just for testing I would say we just entirely delete the repository after moving on to Telemetry, so this question is hopefully not relevant there), but I would only have to guess that our Telemetry has the ability to specify an expiry date for data retention.

I'm going to put in 12 months.

Comment 42

[Johann Hofmann [:johannh]]

Attachment: Data Review Request v4

Added a data retention note at the bottom.

Comment 43

[François Marier [:francois]]

Comment on attachment 8998809 [details]
Data Review Request v4

1) Is there or will there be **documentation** that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes, in this data review request.

2) Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. The user has to explicitly take an action in order to report breakage.

3) If the request is for permanent data collection, is there someone who will monitor the data over time?**

Yes, johannh.

4) Using the **[category system of data types](https://wiki.mozilla.org/Firefox/Data_Collection)** on the Mozilla wiki, what collection type of data do the requested measurements fall under?  **

Category 3, potentially up to category 4 depending on the URLs collected and the information that the user puts into the free-form text field.

5) Is the data collection request for default-on or default-off?

Default OFF. User has to manually opt into submitting a report every time, nothing is collected otherwise.

6) Does the instrumentation include the addition of **any *new* identifiers** (whether anonymous or otherwise; e.g., username, random IDs, etc.  See the appendix for more details)?

No.

7) Is the data collection covered by the existing Firefox privacy notice?

Yes. Legal has signed off on this according to comment 29.

8) Does there need to be a check-in in the future to determine whether to renew the data?

No, permanent.

Comment 44

[Johann Hofmann [:johannh]]

(In reply to François Marier [:francois] from comment #43)
> 7) Is the data collection covered by the existing Firefox privacy notice?
> 
> Yes. Legal has signed off on this according to comment 29.
> 

CC'ing Marshall for just to make sure we're on the same page here.

Comment 45

[Johann Hofmann [:johannh]]

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

Comment 46

[Johann Hofmann [:johannh]]

Comment on attachment 8991316 [details]
Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256234/diff/5-6/

Comment 47

[Johann Hofmann [:johannh]]

Quick update, we have a seemingly unrelated try failure on most platforms except OSX. Having investigated this for quite a while it seems like we're breaking the sanitize dialog's "Accept" button because it relies on Preferences.jsm initialization in some frickle way. We're putting a lazy getter for Preferences.jsm into browser.js, which seems to have tipped this over some edge.

It's probably worth looking into, but for this bug I'd prefer to avoid importing Preferences.jsm and just using Services.prefs to get rid of this failure and be able to finally land this.

Comment 48

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=34c534968c82784539aecb25c2ee77f4fb0bbdde

Comment 49

[Johann Hofmann [:johannh]]

Comment on attachment 8991316 [details]
Bug 1474238 - Add a "report breakage" dialog for Tracking Protection.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256234/diff/6-7/

Comment 50

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb0c0ffeec57
Add a "report breakage" dialog for Tracking Protection. r=Paolo

Comment 51

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/fb0c0ffeec57

Comment 52

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I can confirm the "Report a problem" from the mock up document(V3)is implemented. I verified using Fx 63.0b7 on Windows 10 x64, mac os 10.14 and Ubuntu 14.04 LTS.

Comment 53

[Wellington Torrejais da Silva]

Hi,

"Report a problem" link not working:

===
13:45:28.329 XHRPOSThttps://tracking-protection-issues.herokuapp.com/new[HTTP/1.1 422 UNPROCESSABLE ENTITY 2882ms] 
HeadersCookiesParamsResponseTimingsStack TraceSecurityRequest URL:https://tracking-protection-issues.herokuapp.com/newRequest method:POSTRemote address:54.236.166.251:443Status code:422Edit and ResendRaw headersVersion:HTTP/1.1Response headers (199 B)Connectionkeep-aliveContent-Length40Content-Typetext/html; charset=utf-8DateFri, 14 Dec 2018 15:45:29 GMTServergunicorn/19.9.0Via1.1 vegurRequest headers (404 B)Accept*/*Accept-Encodinggzip, deflate, brAccept-Languageen-US,en;q=0.5Connectionkeep-aliveContent-Length1039content-typemultipart/form-data; boundary=…39755485020227936031255584007DNT1Hosttracking-protection-issues.herokuapp.comOriginnullUser-AgentMozilla/5.0 (X11; Linux x86_64…) Gecko/20100101 Firefox/66.0
===

Thanks!

Comment 54

[(no longer active)]

Where did you see this error?  Can you reproduce?  Thanks!

Comment 55

[Wellington Torrejais da Silva]

(In reply to :Ehsan Akhgari from comment #54)
> Where did you see this error?  Can you reproduce?  Thanks!

Yes, from Nightly Firefox interface at shield icon. 
Just try to report site Tracking protection problem and see the error at Browser console.

Thanks

Comment 56

[(no longer active)]

That is interesting, I can't reproduce!

Comment 57

[(no longer active)]

(Please feel free to file a new bug, commenting on previously fixed bugs usually won't get much attention.)

Comment 58

[Johann Hofmann [:johannh]]

Yes, please file a new bug, and if you feel comfortable with that it would also be great if you could share the site on which it's happening, ideally a screenshot of the "Report Problem" dialog before sending.

Thanks!

Comment 59

[Wellington Torrejais da Silva]

New error:

11:20:25.867 Content Blocking report to https://tracking-protection-issues.herokuapp.com/new failed with status 422 browser-contentblocking.js:784
    submitBreakageReport chrome://browser/content/browser-contentblocking.js:784

Thanks!

Comment 60

[Mike Taylor [:miketaylr]]

Wellington, can you file a new bug please?