I currently can't see a way to tell whether an addon in environment.activeAddons is a system addon. If we can't tell from the existing data we should add that info.

Dave, can we use the currently available properties in some way to tell an addon is a system addon?

You're right we're not including this right now. A quick way would be to include the hidden property, that is probably useful anyway. Right now it only applies to system add-ons but that might be changing in the future so we might want to think about something more accurate.

Do you think adding a hidden property here is useful? Which property is that by the way? Regarding something more accurate, is there anything we can do now so we can rely on this measure long-term?

Rather than the hidden property, couldn't we check that the installLocation is one of the system locations?

(In reply to Robert Helmer [:rhelmer] from comment #5) > Rather than the hidden property, couldn't we check that the installLocation > is one of the system locations? Right now that isn't exposed in the public API so we would need to add something. Probably just an isSystem flag would work.

(In reply to Dave Townsend [:mossop] from comment #6) > (In reply to Robert Helmer [:rhelmer] from comment #5) > > Rather than the hidden property, couldn't we check that the installLocation > > is one of the system locations? > > Right now that isn't exposed in the public API so we would need to add > something. Probably just an isSystem flag would work. Hm, I do see this in about:telemetry right now: firefox@getpocket.com {"scan_items": 1, "scan_MS": 0, "location": "app-system-defaults", "startup_MS": 12} loop@mozilla.org {"scan_items": 1, "scan_MS": 0, "location": "app-system-defaults", "startup_MS": 26} It looks like this is coming from XPIProvider.jsm which is why it's able to use a non-public method I bet: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm?from=toolkit%2Fmozapps%2Fextensions%2Finternal%2FXPIProvider.jsm#2204

(In reply to Robert Helmer [:rhelmer] from comment #7) > (In reply to Dave Townsend [:mossop] from comment #6) > > (In reply to Robert Helmer [:rhelmer] from comment #5) > > > Rather than the hidden property, couldn't we check that the installLocation > > > is one of the system locations? > > > > Right now that isn't exposed in the public API so we would need to add > > something. Probably just an isSystem flag would work. > > Hm, I do see this in about:telemetry right now: > > firefox@getpocket.com {"scan_items": 1, "scan_MS": 0, "location": > "app-system-defaults", "startup_MS": 12} > loop@mozilla.org {"scan_items": 1, "scan_MS": 0, "location": > "app-system-defaults", "startup_MS": 26} > > It looks like this is coming from XPIProvider.jsm which is why it's able to > use a non-public method I bet: > > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > internal/XPIProvider. > jsm?from=toolkit%2Fmozapps%2Fextensions%2Finternal%2FXPIProvider.jsm#2204 Yeah, that's not in the environment though which I assume is where we need it.

(In reply to Dave Townsend [:mossop] from comment #8) > (In reply to Robert Helmer [:rhelmer] from comment #7) > > (In reply to Dave Townsend [:mossop] from comment #6) > > > (In reply to Robert Helmer [:rhelmer] from comment #5) > > > > Rather than the hidden property, couldn't we check that the installLocation > > > > is one of the system locations? > > > > > > Right now that isn't exposed in the public API so we would need to add > > > something. Probably just an isSystem flag would work. > > > > Hm, I do see this in about:telemetry right now: > > > > firefox@getpocket.com {"scan_items": 1, "scan_MS": 0, "location": > > "app-system-defaults", "startup_MS": 12} > > loop@mozilla.org {"scan_items": 1, "scan_MS": 0, "location": > > "app-system-defaults", "startup_MS": 26} > > > > It looks like this is coming from XPIProvider.jsm which is why it's able to > > use a non-public method I bet: > > > > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > > internal/XPIProvider. > > jsm?from=toolkit%2Fmozapps%2Fextensions%2Finternal%2FXPIProvider.jsm#2204 > > Yeah, that's not in the environment though which I assume is where we need > it. Oh right, I see now thanks.

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

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

https://reviewboard.mozilla.org/r/32365/#review29081 Would it be hard to add a simple system addon to toolkit/components/telemetry/tests/addons/ and add/modify a test for .isSystem having the correct values in test_TelemetryEnvironment.js? Please also update the documentation in toolkit/components/telemetry/docs/environment.rst

Comment on attachment 8711889 [details] MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32365/diff/1-2/

https://reviewboard.mozilla.org/r/32365/#review29081 Thanks for reviewing! > Would it be hard to add a simple system addon to toolkit/components/telemetry/tests/addons/ and add/modify a test for .isSystem having the correct values in test_TelemetryEnvironment.js? Sure thing - I verified this manually in about:telemetry but I'd feel much better having a test here to make sure it doesn't regress (there's a test on the add-ons manager side of course but couldn't hurt to have it in telemetry too). There is nothing special about the "system" add-ons themselves, but it does need to be copied into one of the `features` directories (either in the app or the profile) and enabled in `extensions.systemAddonSet`. > Please also update the documentation in toolkit/components/telemetry/docs/environment.rst Fixed this bit, thanks.

https://reviewboard.mozilla.org/r/32365/#review29597 Thanks - as this might become an important data piece with Go Faster i would much prefer if we have basic test coverage in test_TelemetryEnvironment.js (checking represantations of system vs. non-system addon). I think you didn't have any review flags set (which is why i missed this for a while). Please flag me for review on the update for this part, Dave Townsend on the other patch.

(In reply to Georg Fritzsche [:gfritzsche] from comment #15) > https://reviewboard.mozilla.org/r/32365/#review29597 > > Thanks - as this might become an important data piece with Go Faster i would > much prefer if we have basic test coverage in test_TelemetryEnvironment.js > (checking represantations of system vs. non-system addon). I was out today, I have a patch which is almost ready. > I think you didn't have any review flags set (which is why i missed this for > a while). > Please flag me for review on the update for this part, Dave Townsend on the > other patch. Ah, I was just pushing to mozreview so I could get a try run before flagging anyone for review :) I should just push to try directly. Will flag you for the next one.

Note that we will need to uplift these patches to at least Firefox 46, as bug 1235627 is tracking Firefox 46 and will likely rely on the fix here.

Comment on attachment 8711888 [details] MozReview Request: bug 1232222 - expose isSystem flag if add-on is a system add-on r?Mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32363/diff/1-2/

Comment on attachment 8711889 [details] MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32365/diff/2-3/

Comment on attachment 8711889 [details] MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32365/diff/3-4/

Comment on attachment 8711889 [details] MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche https://reviewboard.mozilla.org/r/32365/#review30333 Thanks! ::: toolkit/components/telemetry/tests/unit/head.js:170 (Diff revision 4) > + // manually. Please mention what this is actually needed for so we remember in the future. ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:1080 (Diff revision 4) > +add_task(function* test_systemAddon() { We could just do this as part of test_addonsAndPlugins()? Feel free to just integrate it there if you agree.

Comment on attachment 8711888 [details] MozReview Request: bug 1232222 - expose isSystem flag if add-on is a system add-on r?Mossop https://reviewboard.mozilla.org/r/32363/#review30359

Comment on attachment 8711889 [details] MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32365/diff/4-5/

https://hg.mozilla.org/integration/fx-team/rev/38ba7c48145e https://hg.mozilla.org/integration/fx-team/rev/06bc151231a5

I had to back this out for xpcshell bustage: https://treeherder.mozilla.org/logviewer.html#?job_id=7107635&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/e0043a092deb

(In reply to Wes Kocher (:KWierso) from comment #26) > I had to back this out for xpcshell bustage: > https://treeherder.mozilla.org/logviewer.html#?job_id=7107635&repo=fx-team > > https://hg.mozilla.org/integration/fx-team/rev/e0043a092deb Thanks! Sorry I must have forgotten to run the test again after making the final review-suggested change... will fix it up momentarily.

(In reply to Robert Helmer [:rhelmer] from comment #27) > (In reply to Wes Kocher (:KWierso) from comment #26) > > I had to back this out for xpcshell bustage: > > https://treeherder.mozilla.org/logviewer.html#?job_id=7107635&repo=fx-team > > > > https://hg.mozilla.org/integration/fx-team/rev/e0043a092deb > > Thanks! Sorry I must have forgotten to run the test again after making the > final review-suggested change... will fix it up momentarily. Oh, huh... is this only on Windows 7? Not sure why this would only be happening there, the failure is: 16:24:16 WARNING - TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | test_addonsAndPlugins - [test_addonsAndPlugins : 1077] installDay must have the correct value. - 14610 == 16837 I am going to do a try run with just that platform... it's not failing on Windows 8, oddly. I wonder if it's something wrong w/ a particular server?

(In reply to Robert Helmer [:rhelmer] from comment #28) > (In reply to Robert Helmer [:rhelmer] from comment #27) > > (In reply to Wes Kocher (:KWierso) from comment #26) > > > I had to back this out for xpcshell bustage: > > > https://treeherder.mozilla.org/logviewer.html#?job_id=7107635&repo=fx-team > > > > > > https://hg.mozilla.org/integration/fx-team/rev/e0043a092deb > > > > Thanks! Sorry I must have forgotten to run the test again after making the > > final review-suggested change... will fix it up momentarily. > > Oh, huh... is this only on Windows 7? Not sure why this would only be > happening there, the failure is: > > 16:24:16 WARNING - TEST-UNEXPECTED-FAIL | > toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | > test_addonsAndPlugins - [test_addonsAndPlugins : 1077] installDay must have > the correct value. - 14610 == 16837 > > I am going to do a try run with just that platform... it's not failing on > Windows 8, oddly. I wonder if it's something wrong w/ a particular server? The code here is doing |truncateToDays(Date.now())| - I suspect what happened here is that the clock rolled over to midnight after the test started, since the system add-on is installed when the test is started there's more of a chance for this to happen. I'll change the test to record the date closer to the time the test starts, if the above is correct then there's always the chance of this happening (to the existing add-on test too) unless the time is mocked instead of using |Date.now()| so that might be a better fix (maybe for a followup bug if you agree)

Comment on attachment 8711889 [details] MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32365/diff/5-6/

(In reply to Robert Helmer [:rhelmer] from comment #29) > > 16:24:16 WARNING - TEST-UNEXPECTED-FAIL | > > toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | > > test_addonsAndPlugins - [test_addonsAndPlugins : 1077] installDay must have > > the correct value. - 14610 == 16837 > > > > I am going to do a try run with just that platform... it's not failing on > > Windows 8, oddly. I wonder if it's something wrong w/ a particular server? > > The code here is doing |truncateToDays(Date.now())| - I suspect what > happened here is that the clock rolled over to midnight after the test > started, since the system add-on is installed when the test is started > there's more of a chance for this to happen. While this is a failure scenario that we should fix, the installDay value differs by much more than one day. There has to be more going on.

These days are: new Date(14610 * MILLISECONDS_PER_DAY) => Date 2010-01-01T00:00:00.000Z new Date(16837 * MILLISECONDS_PER_DAY) => Date 2016-02-06T00:00:00.000Z On a first quick look, i assume installDate & updateDate come from here for this scenario: https://dxr.mozilla.org/mozilla-central/rev/584870f1cbc5d060a57e147ce249f736956e2b62/toolkit/mozapps/extensions/internal/XPIProviderUtils.js#1656 Could the mtimes on disk be off on that machine?

Comment on attachment 8711889 [details] MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche https://reviewboard.mozilla.org/r/32365/#review30595 ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:731 (Diff revisions 5 - 6) > do_get_file("system.xpi").copyTo(distroDir, "tel-system-xpi@tests.mozilla.org.xpi"); I think the addons `installDate` & `updateDate` are set from this files mtime? If so, we should set both the `.lastModifiedTime` & `SYSTEM_ADDON_INSTALL_DATE` here.

(In reply to Georg Fritzsche [:gfritzsche] from comment #32) > These days are: > new Date(14610 * MILLISECONDS_PER_DAY) => Date 2010-01-01T00:00:00.000Z > new Date(16837 * MILLISECONDS_PER_DAY) => Date 2016-02-06T00:00:00.000Z > > On a first quick look, i assume installDate & updateDate come from here for > this scenario: > https://dxr.mozilla.org/mozilla-central/rev/ > 584870f1cbc5d060a57e147ce249f736956e2b62/toolkit/mozapps/extensions/internal/ > XPIProviderUtils.js#1656 > > Could the mtimes on disk be off on that machine? Hm that's a good point, I didn't notice how far off that first date is.

Comment on attachment 8711889 [details] MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32365/diff/6-7/

Comment on attachment 8711889 [details] MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche https://reviewboard.mozilla.org/r/32365/#review30667 ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js:732 (Diff revisions 6 - 7) > + let system_addon = FileUtils.File(distroDir.path + "/" + "tel-system-xpi@tests.mozilla.org.xpi"); This should use `.append()`.

Comment on attachment 8711889 [details] MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32365/diff/7-8/

Actually let me make sure we have a good try run before landing this again.

still has conflicts when landing on fx-team, robert could you take a look ,thanks!

Comment on attachment 8711888 [details] MozReview Request: bug 1232222 - expose isSystem flag if add-on is a system add-on r?Mossop Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32363/diff/2-3/

Comment on attachment 8711889 [details] MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32365/diff/8-9/

Rebased against fx-team, thanks!

https://hg.mozilla.org/integration/fx-team/rev/45cc53e68f35 https://hg.mozilla.org/integration/fx-team/rev/516dc8aba9a5

https://hg.mozilla.org/mozilla-central/rev/45cc53e68f35 https://hg.mozilla.org/mozilla-central/rev/516dc8aba9a5