Closed
Bug 668392
Opened 13 years ago
Closed 13 years ago
Include enabled addons + persona in telemetry
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
FIXED
mozilla10
Tracking | Status | |
---|---|---|
firefox8 | --- | unaffected |
firefox9 | --- | unaffected |
People
(Reporter: taras.mozilla, Assigned: taras.mozilla)
References
Details
(Whiteboard: [inbound])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
Updated•13 years ago
|
Attachment #543007 -
Attachment is patch: true
Assignee | ||
Comment 2•13 years ago
|
||
(In reply to comment #1) > ouch, you really want to fail silently in XPIProvider? I don't see why not
Comment 3•13 years ago
|
||
hmm, mama told me never to leave catch-all exceptions not handled. But I'm just wondering if for some reason the addon list will not serialize and we'll have no notification, but I guess we can catch it from the data entries anyway. nevermind
Comment 4•13 years ago
|
||
Comment on attachment 543007 [details] [diff] [review] addons + personas Review of attachment 543007 [details] [diff] [review]: ----------------------------------------------------------------- Should add some tests to this. ::: toolkit/mozapps/extensions/XPIProvider.jsm @@ +1739,5 @@ > + > + try { > + const TelemetryPing = Cc["@mozilla.org/base/telemetry-ping;1"].getService(Ci.nsIObserver); > + TelemetryPing.observe(null, "Add-ons", data); > + } catch (e) { } The previous block has no catch block because it is an expected condition (Sometimes crash reporting is disabled in which case annotating throws). I don't think this new bit can ever throw can it? If not the try...catch seems unnecessary.
Attachment #543007 -
Flags: review?(dtownsend) → review-
Assignee | ||
Comment 5•13 years ago
|
||
(In reply to comment #4) > Comment on attachment 543007 [details] [diff] [review] [review] > addons + personas > > Review of attachment 543007 [details] [diff] [review] [review]: > ----------------------------------------------------------------- > > Should add some tests to this. Dave, I'm stuck on this, could use some help. Testing the persona bit is tricky. LightweightThemeManager tests benefit from having the ability to initialize the addon/etc manager. This is a good chunk of code. Do you expect me to copy code from addon manager test harness or can we skip this? Testing the addon list is just a matter of simulating the observer call, that's no problem. > > ::: toolkit/mozapps/extensions/XPIProvider.jsm > @@ +1739,5 @@ > > + > > + try { > > + const TelemetryPing = Cc["@mozilla.org/base/telemetry-ping;1"].getService(Ci.nsIObserver); > > + TelemetryPing.observe(null, "Add-ons", data); > > + } catch (e) { } > > The previous block has no catch block because it is an expected condition > (Sometimes crash reporting is disabled in which case annotating throws). I > don't think this new bit can ever throw can it? If not the try...catch seems > unnecessary. I will get rid of that in the next patch once I know how resolve the lwt problem above.
Comment 6•13 years ago
|
||
(In reply to comment #5) > (In reply to comment #4) > > Comment on attachment 543007 [details] [diff] [review] [review] [review] > > addons + personas > > > > Review of attachment 543007 [details] [diff] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > Should add some tests to this. > > Dave, I'm stuck on this, could use some help. > > Testing the persona bit is tricky. LightweightThemeManager tests benefit > from having the ability to initialize the addon/etc manager. This is a good > chunk of code. Do you expect me to copy code from addon manager test harness > or can we skip this? I don't think you'd need to copy much code at all to test this. Since the telemetry test already creates an nsIXULAppInfo I think really the only code you need to initialise the add-ons manager are these 5 lines: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/head_addons.js#298 After that you ought to just be able to set a persona using LightweightThemeManager.currentTheme = { ... }. > Testing the addon list is just a matter of simulating the observer call, > that's no problem. So I was actually more concerned with verifying that the add-ons manager is calling the telemetry service with the list of add-ons, but given how the code is (and the fact we already test that the crash reporting annotation works) I wonder if that is really a worthwhile use of time, maybe not.
Assignee | ||
Comment 7•13 years ago
|
||
>
> I don't think you'd need to copy much code at all to test this. Since the
> telemetry test already creates an nsIXULAppInfo I think really the only code
> you need to initialise the add-ons manager are these 5 lines:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/xpcshell/head_addons.js#298
>
> After that you ought to just be able to set a persona using
> LightweightThemeManager.currentTheme = { ... }.
>
It seems so simple but I tried that and it didn't work.
See the do_check_true(LightweightThemeManager.currentTheme != undefined) in the test.
Attachment #543007 -
Attachment is obsolete: true
Attachment #548638 -
Flags: feedback?(dtownsend)
Comment 8•13 years ago
|
||
(In reply to comment #7) > Created attachment 548638 [details] [diff] [review] [diff] [details] [review] > patch with nonworking test > > > > > I don't think you'd need to copy much code at all to test this. Since the > > telemetry test already creates an nsIXULAppInfo I think really the only code > > you need to initialise the add-ons manager are these 5 lines: > > http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/ > > test/xpcshell/head_addons.js#298 > > > > After that you ought to just be able to set a persona using > > LightweightThemeManager.currentTheme = { ... }. > > > It seems so simple but I tried that and it didn't work. > > See the do_check_true(LightweightThemeManager.currentTheme != undefined) in > the test. Sorry for the long delay here. I forgot one other thing. The add-ons manager needs a defined profile directory so put do_get_profile(); at the start of run_test and things will work right.
Assignee | ||
Comment 9•13 years ago
|
||
Thanks, that little detail solved it.
Attachment #548638 -
Attachment is obsolete: true
Attachment #551159 -
Flags: review?(dtownsend)
Attachment #548638 -
Flags: feedback?(dtownsend)
Updated•13 years ago
|
Attachment #551159 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 10•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fdffde7c3b14
Whiteboard: [inbound]
Comment 11•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fdffde7c3b14
Assignee: nobody → tglek
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
Updated•13 years ago
|
Keywords: sec-review-needed
Comment 12•13 years ago
|
||
This kind of data verges on "personally identifiable" and is quite a bit different from the type of anonymous statistical data presented when we did the security review of the feature. In fact this is the type of thing that prompted us to request that new types of data seek a review before being added -- did that happen? Maybe it did, we've got a big team now, but I don't see any bugs/wiki pages linked here to indicate that. Some of that information can already be gleaned from AMO update check where it's already covered by our privacy policy.
Keywords: privacy
Updated•13 years ago
|
Keywords: privacy-review-needed
Assignee | ||
Comment 13•13 years ago
|
||
(In reply to Daniel Veditz from comment #12) > This kind of data verges on "personally identifiable" and is quite a bit > different from the type of anonymous statistical data presented when we did > the security review of the feature. In fact this is the type of thing that > prompted us to request that new types of data seek a review before being > added -- did that happen? Maybe it did, we've got a big team now, but I > don't see any bugs/wiki pages linked here to indicate that. I did not request security review for this since it's already sent in AMO ping. As the privacy policy goes this seems covered under "user interface features, memory, and hardware configuration." We aren't collecting persona + addons as an alternative to AMO. The idea is to correlate slow performance in certain metrics with installed addons. This can not be done through AMO check.
Comment 14•13 years ago
|
||
I'll buy that. Sid will still want a privacy-team review.
Keywords: sec-review-needed
Updated•13 years ago
|
Keywords: sec-review-complete
Comment 15•13 years ago
|
||
(In reply to Taras Glek (:taras) from comment #13) > I did not request security review for this since it's already sent in AMO > ping. As the privacy policy goes this seems covered under "user interface > features, memory, and hardware configuration." This collection does not jive with the current opt-in string. We need to adjust the opt in string so that it is clear we could collect things like the add-on list. This opt-in UI adjustment is bug 685373.
Depends on: 685373
Assignee | ||
Comment 16•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/f2da319b3f6a Backed out as privacy opt-in needs to be updated for this. Will reland for ff8 once privacy stuff is ready.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 18•13 years ago
|
||
Backed out of aurora8 as well: http://hg.mozilla.org/releases/mozilla-aurora/rev/0ce62710d317
status-firefox8:
--- → unaffected
status-firefox9:
--- → unaffected
Target Milestone: mozilla8 → mozilla10
Comment 19•13 years ago
|
||
We updated the opt-in string in bug 685373 to include "browser customizations" which encompasses add-ons and persona. If we still find value in collecting this, it could probably go back in.
Keywords: privacy,
privacy-review-needed
Assignee | ||
Updated•13 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 20•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/f24bb32f3103
Keywords: checkin-needed
Whiteboard: [inbound]
Comment 21•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f24bb32f3103 btw, the checkin comment is missing bug number...
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Comment 22•13 years ago
|
||
Yes, I think this needs to be backed out and re-checked in with the bug number (that seems to be the usual practice). I'm also not overly comfortable with this change. I'd be very surprised if there is even one other user with the same combination of addons as me and so this is effectively a unique ID for me that spans sessions. The telemetry reporters before this checkin were much less identifiable. Seeing as there is no way to choose which reporters to opt out of, this may drive people to disable telemetry altogether.
Assignee | ||
Comment 23•13 years ago
|
||
(In reply to Daniel Cater from comment #22) > Yes, I think this needs to be backed out and re-checked in with the bug > number (that seems to be the usual practice). I'll fix it when I get a chance. > > I'm also not overly comfortable with this change. I'd be very surprised if > there is even one other user with the same combination of addons as me and > so this is effectively a unique ID for me that spans sessions. The telemetry > reporters before this checkin were much less identifiable. > > Seeing as there is no way to choose which reporters to opt out of, this may > drive people to disable telemetry altogether. We need to be able to correlate crappy performance with addons. We would've not put this in if we didn't believe this was critical to improve Firefox. The upside is that data is transmitted over https and we do not send data if certificates do not line up.
Comment 24•13 years ago
|
||
Perhaps it would be worth trying to address Daniel's legitimate concern by providing an interface where users can opt into or out of specific reporters, perhaps in about:telemetry or so? I don't think this should be a blocker by any means, but it might ber a desirable enhancement.
Assignee | ||
Comment 25•13 years ago
|
||
(In reply to Tom Lowenthal [:StrangeCharm] from comment #24) > Perhaps it would be worth trying to address Daniel's legitimate concern by > providing an interface where users can opt into or out of specific > reporters, perhaps in about:telemetry or so? I don't think this should be a > blocker by any means, but it might ber a desirable enhancement. Brilliant idea, added to bug 661881. Now to get someone to do a UI for us :(
Assignee | ||
Comment 26•13 years ago
|
||
Backed out + relanded with bug# on inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/69f7d8cc0c00
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [inbound]
Comment 27•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/69f7d8cc0c00
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Flags: sec-review+
You need to log in
before you can comment on or make changes to this bug.
Description
•