(dormant account) Assignee
	Description • 13 years ago

      No description provided.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 1 • 13 years ago

ouch, you really want to fail silently in XPIProvider?

	(dormant account) 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

	Zibi Braniecki [:zbraniecki][:gandalf]
	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

	Dave Townsend [:mossop]
	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.

	(dormant account) 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.

	Dave Townsend [:mossop]
	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.

	(dormant account) 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.

	Dave Townsend [:mossop]
	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.

	(dormant account) Assignee
	Comment 9 • 13 years ago

Thanks, that little detail solved it.

	(dormant account) Assignee
	Comment 10 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdffde7c3b14

	Matt Brubeck (:mbrubeck)
	Comment 11 • 13 years ago

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

	Daniel Veditz [:dveditz]
	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.

	(dormant account) 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.

	Daniel Veditz [:dveditz]
	Comment 14 • 13 years ago

I'll buy that. Sid will still want a privacy-team review.

	Sid Stamm [:geekboy or :sstamm]
	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.

	(dormant account) 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.

	(dormant account) Assignee
	Comment 17 • 13 years ago

And by ff8 I meant ff9

	LegNeato
	Comment 18 • 13 years ago

Backed out of aurora8 as well:

http://hg.mozilla.org/releases/mozilla-aurora/rev/0ce62710d317

	Sid Stamm [:geekboy or :sstamm]
	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.

	(dormant account) Assignee
	Comment 20 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/f24bb32f3103

	Marco Bonardo [:mak]
	Comment 21 • 13 years ago

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

btw, the checkin comment is missing bug number...

	Daniel C
	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.

	(dormant account) 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.

	Tom Lowenthal
	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.

	(dormant account) 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 :(

	(dormant account) Assignee
	Comment 26 • 13 years ago

Backed out + relanded with bug# on inbound
http://hg.mozilla.org/integration/mozilla-inbound/rev/69f7d8cc0c00

	Ed Morley [:emorley]
	Comment 27 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/69f7d8cc0c00