1515712 - check bundled JAR and XPI files for corruption

Status: RESOLVED FIXED

(Firefox :: General, enhancement, P2)

Description

[Robert Helmer [:rhelmer]]

We've seen problems for quite a while related to JAR and XPI files that are bundled with Firefox being corrupted, such as crashes. Let's start measuring this.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: Bug 1515712 - check bundled JAR and XPI files for corruption r?kmag,janerik

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: Bug 1515712 - add default pref and start corroborator if enabled r?kmag

Depends on D15123

Comment 3

[Robert Helmer [:rhelmer]]

Attachment: Bug 1515712 - provide OMNIJAR_NAME constant for browser r?kmag

Comment 4

[Robert Helmer [:rhelmer]]

Attachment: Data Collection Review for Bug 1515712

Comment 5

[Chris H-C :chutten]

Comment on attachment 9035095 [details]
Data Collection Review for Bug 1515712

Preliminary note:

When it comes to Linux distributions who build their own Firefox the channels they set do not necessarily map to our nightly/beta/release set. By having releaseChannelCollection: opt-out you are enabling collection for all non-mozilla channels. By turning corroborator off for beta and release by pref you are only turning them off for those two mozilla-built channels. It is possible that Linux distros may specifically disable corroborator or build using their own firefox.js or prefs.js that supercede the one you edit, but I don't expect them to keep up with all of our changes.

In short, you may be collecting data from more Firefox installs than you think with the code written the way it's currently written. From a Data Collection Review POV this is acceptable: it's Category 1 data. It just may be unintentional or unexpected.

Another note: since you state in Q9 that you're interested in the current prevalence of these errors it seems as though you could start these collections as expiring collections to serve that need and "upgrade" them to eternal collection pending the results of your analysis. Something to consider.

DATA COLLECTION REVIEW RESPONSE:

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

Yes. These collections are Telemetry so are documented in their definitions file (Scalars.yaml), the Probe Dictionary, and on telemetry.mozilla.org's Measurement Dashboards.

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

Yes. These collections are Telemetry so can be controlled through Firefox's Preferences. Also the functionality of the underlying feature can be controlled by the preference corroborator.enabled. Setting that to false disables the feature including the data collection.

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

Yes, :rhelmer.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical. (I suppose corrupt XPI detection counts are proportional to addon install counts which could be an argument for Category 2... but a seriously weak argument)

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

Default on for all channels, though the feature (including its data collection) is presently disabled in beta and release.

Does the instrumentation include the addition of any new identifiers?

No.

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

Yes.

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

No. This collection is permanent.

---

Result: datareview+

Comment 6

[Robert Helmer [:rhelmer]]

(In reply to Chris H-C :chutten from comment #5)

Comment on attachment 9035095 [details]
Data Collection Review for Bug 1515712

Preliminary note:

When it comes to Linux distributions who build their own Firefox the channels they set do not necessarily map to our nightly/beta/release set. By having releaseChannelCollection: opt-out you are enabling collection for all non-mozilla channels. By turning corroborator off for beta and release by pref you are only turning them off for those two mozilla-built channels. It is possible that Linux distros may specifically disable corroborator or build using their own firefox.js or prefs.js that supercede the one you edit, but I don't expect them to keep up with all of our changes.

This is a good point, thanks! I think that data from builds other than ours are going to be junk, since they won't be coming from us and will be unsigned.

In short, you may be collecting data from more Firefox installs than you think with the code written the way it's currently written. From a Data Collection Review POV this is acceptable: it's Category 1 data. It just may be unintentional or unexpected.

Another note: since you state in Q9 that you're interested in the current prevalence of these errors it seems as though you could start these collections as expiring collections to serve that need and "upgrade" them to eternal collection pending the results of your analysis. Something to consider.

So in Scalars.yaml I set:

expires: "71"

Is there anything else I need to do to make them expiring collections?

Comment 7

[Chris H-C :chutten]

(In reply to Robert Helmer [:rhelmer] from comment #6)

Another note: since you state in Q9 that you're interested in the current prevalence of these errors it seems as though you could start these collections as expiring collections to serve that need and "upgrade" them to eternal collection pending the results of your analysis. Something to consider.

So in Scalars.yaml I set:

expires: "71"

Is there anything else I need to do to make them expiring collections?

We should probably cut another edition of the data review request that says you only want it until the end Firefox 70 for now, and I'll reply with a marginally-different data review response. This'll help future us understand what the final result was.

But yeah, as far as code goes, that's it.

When Nightly hits 70 you should (the service that does this is somewhat reliable) get an email about it expiring in the next version. And shortly thereafter a bug might be filed when the version increase simulation builds start running if you have an automated test that doesn't account for the possibility of your collection being expired. (collecting to an expired metric is a no-op. No errors, but snapshots won't have the accumulations). This'll help us remember to look at the data and make our conclusions, and see if the metric should be removed or renewed.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:rhelmer, could you have a look please?

Comment 9

[Robert Helmer [:rhelmer]]

We manually tested all the various options for signing omni JAR in bug 1533818 and everything looks good, going to get this patch in shape to land so we'll be ready once releng starts shipping signed JARs with Nightly.

Comment 10

[Robert Helmer [:rhelmer]]

Attachment: Bug 1515712 - check bundled JAR and XPI files for corruption r?kmag,chutten

Comment 11

[Robert Helmer [:rhelmer]]

Attachment: Bug 1515712 - add default pref and start corroborator if enabled r?kmag

Depends on D28245

Comment 12

[Robert Helmer [:rhelmer]]

Attachment: Bug 1515712 - provide OMNIJAR_NAME constant for browser r?kmag

Depends on D28246

Comment 13

[Robert Helmer [:rhelmer]]

Attachment: Data Collection Review for Bug 1515712 (v2)

re-requesting review per comment 7

Comment 14

[Robert Helmer [:rhelmer]]

Attachment: Bug 1515712 - add default pref and start corroborator if enabled r?kmag

Depends on D15123

Comment 15

[Robert Helmer [:rhelmer]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c2dfb76974200d57980c780c7d9f86f0f1d22f4

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2d24fd1fd67
check bundled JAR and XPI files for corruption r=kmag,chutten

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/acb99631797a
provide OMNIJAR_NAME constant for browser r=kmag

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e46d5fddd21
add default pref and start corroborator if enabled r=kmag

Comment 19

[Cristian Brindusan [:cbrindusan]]

Backed out 3 changesets (Bug 1515712) for build bustages at 'cpp_guard'.

Backout: https://hg.mozilla.org/integration/autoland/rev/785fe31ffe40f8e97afcbceae3fc4f29713934e4

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=superseded%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=a2d24fd1fd67761b25655f78df2473494f08977d&selectedJob=241550955

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=241550955&repo=autoland&lineNumber=3764

Comment 20

[Robert Helmer [:rhelmer]]

There is a stray line from a merge in the first patch which is fixed by the second, I'll just remove it entirely before re-landing, thanks!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/718402b108aa
provide OMNIJAR_NAME constant for browser r=kmag
https://hg.mozilla.org/integration/autoland/rev/ac6c0bc531e3
check bundled JAR and XPI files for corruption r=kmag,chutten
https://hg.mozilla.org/integration/autoland/rev/6e082b675763
add default pref and start corroborator if enabled r=kmag

Comment 22

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/718402b108aa
https://hg.mozilla.org/mozilla-central/rev/ac6c0bc531e3
https://hg.mozilla.org/mozilla-central/rev/6e082b675763

Comment 23

[Robert Helmer [:rhelmer]]

I went ahead and landed this ahead of the latest data review since it's a very minor change to an already-approved data review, see comment 7.

Comment 24

[Chris H-C :chutten]

Comment on attachment 9059584 [details]
Data Collection Review for Bug 1515712 (v2)

DATA COLLECTION REVIEW RESPONSE:

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

Yes. This collection is Telemetry so is documented in its definitions file [Scalars.yaml](https://hg.mozilla.org/mozilla-central/file/tip/toolkit/components/telemetry/Scalars.yaml) and the [Probe Dictionary](https://telemetry.mozilla.org/probe-dictionary/).

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

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

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

No. This collection will expire in Firefox 71.

    Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

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

Default on for all channels (though controlled by pref)

    Does the instrumentation include the addition of any new identifiers?

No.

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

Yes.

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

Yes. :rhelmer is responsible for renewing or removing the collection before it expires in Firefox 71.

---
Result: datareview+

Comment 25

[Justin Wood (:Callek)]

Doubling back on this.

Releng is close to deploying support for this, which would need uplift to be used in 68. I feel the risk is minimal but will approach that request once it is running sanely on Nightly.

Feel free to let me know if there are any product concerns over the timing.