Closed Bug 1232222 Opened 9 years ago Closed 8 years ago

Need environment data on which addons are system addons

Categories

(Toolkit :: Telemetry, defect, P3)

Product:
Toolkit
Component:
Telemetry
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox46 --- affected
firefox47 --- fixed

People

(Reporter: gfritzsche, Assigned: rhelmer)

References

Details

(Whiteboard: [measurement:client])

Attachments

(2 files)

MozReview Request: bug 1232222 - expose isSystem flag if add-on is a system add-on r?Mossop
58 bytes, text/x-review-board-request
mossop
: review+
Details
MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche
58 bytes, text/x-review-board-request
gfritzsche
: review+
Details 
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?
Flags: needinfo?(dtownsend)
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.
Flags: needinfo?(dtownsend)
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?
Flags: needinfo?(dtownsend)
Rather than the hidden property, couldn't we check that the installLocation is one of the system locations?
Assignee: nobody → rhelmer
Flags: needinfo?(dtownsend)
(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
Status: NEW → ASSIGNED
(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.
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.
status-firefox46: --- → affected
status-firefox47: --- → affected
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/
Attachment #8711888 - Attachment description: MozReview Request: bug 1232222 - expose isSystem flag if add-on is a system add-on → MozReview Request: bug 1232222 - expose isSystem flag if add-on is a system add-on r?Mossop
Attachment #8711888 - Flags: review?(dtownsend)
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/
Attachment #8711889 - Attachment description: MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons → MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r?gfritzsche
Attachment #8711889 - Flags: review?(gfritzsche)
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/
Attachment #8711889 - Flags: review?(gfritzsche) → review+
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
Attachment #8711888 - Flags: review?(dtownsend) → review+
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/
Keywords: checkin-needed
(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.
Flags: needinfo?(rhelmer)
(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)
Flags: needinfo?(gfritzsche)
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.
Flags: needinfo?(gfritzsche)
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.
Attachment #8711889 - Flags: review+
(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/
Attachment #8711889 - Flags: review?(gfritzsche)
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()`.
Attachment #8711889 - Flags: review?(gfritzsche) → review+
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/
Attachment #8711889 - Attachment description: MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r?gfritzsche → MozReview Request: bug 1232222 - provide telemetry environment data on which addons are system addons r=gfritzsche
Keywords: checkin-needed
Actually let me make sure we have a good try run before landing this again.
Keywords: checkin-needed
Keywords: checkin-needed
still has conflicts when landing on fx-team, robert could you take a look ,thanks!
Flags: needinfo?(rhelmer)
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!
Flags: needinfo?(rhelmer) → needinfo?(cbook)
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/45cc53e68f35
https://hg.mozilla.org/mozilla-central/rev/516dc8aba9a5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: