1233438 - Addon data missing in environment if there is an addon with undefined .description

Status: VERIFIED FIXED

(Toolkit :: Telemetry, defect, P3)

Description

[Alessio Placitelli [:Dexter] - PTO until 26/July]

I just noticed that mochitest-browser-chrome bc4 tests logs are cluttered with this errors:

06:16:35     INFO -  *************************
06:16:35     INFO -  A coding exception was thrown and uncaught in a Task.
06:16:35     INFO -  Full message: TypeError: aString is undefined
06:16:35     INFO -  Full stack: limitStringToLength@resource://gre/modules/TelemetryEnvironment.jsm:267:3
06:16:35     INFO -  EnvironmentAddonBuilder.prototype._getActiveAddons<@resource://gre/modules/TelemetryEnvironment.jsm:521:22
06:16:35     INFO -  EnvironmentAddonBuilder.prototype._updateAddons<@resource://gre/modules/TelemetryEnvironment.jsm:479:27
06:16:35     INFO -  TaskImpl_run@resource://gre/modules/Task.jsm:315:40
06:16:35     INFO -  TaskImpl@resource://gre/modules/Task.jsm:276:3
06:16:35     INFO -  createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:250:14
06:16:35     INFO -  EnvironmentAddonBuilder.prototype._checkForChanges@resource://gre/modules/TelemetryEnvironment.jsm:437:25
06:16:35     INFO -  EnvironmentAddonBuilder.prototype._onAddonChange@resource://gre/modules/TelemetryEnvironment.jsm:422:5
06:16:35     INFO -  EnvironmentAddonBuilder.prototype.onInstalled@resource://gre/modules/TelemetryEnvironment.jsm:414:5
06:16:35     INFO -  AddonManagerInternal.callAddonListeners@resource://gre/modules/AddonManager.jsm:1801:11
06:16:35     INFO -  this.AddonManagerPrivate.callAddonListeners@resource://gre/modules/AddonManager.jsm:2842:5
06:16:35     INFO -  _setCurrentTheme@resource://gre/modules/LightweightThemeManager.jsm:700:8
06:16:35     INFO -  this.LightweightThemeManager.currentTheme@resource://gre/modules/LightweightThemeManager.jsm:147:12
06:16:35     INFO -  @chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug591465.js:255:3
06:16:35     INFO -  log_exceptions@chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/head.js:185:12
06:16:35     INFO -  run_next_test/<@chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/head.js:227:21
06:16:35     INFO -  testScope/test_executeSoon/<.run@chrome://mochikit/content/browser-test.js:968:9
06:16:35     INFO -  *************************

We should check in [0] that aString is both non-null and non-undefined.

[0] - https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/toolkit/components/telemetry/TelemetryEnvironment.jsm#264

Comment 1

[Georg Fritzsche [:gfritzsche]]

This can be reproduced by looking at the output of:
mach test browser/base/content/test/social/browser_aboutHome_activation.js

Comment 2

[Georg Fritzsche [:gfritzsche]]

[Tracking Requested - why for this release]:
We broke our addons data collection here:
If users have any addons without a description field installed, we don't collect any addon data.
This is a regression from bug 1211404, which sadly went out on release with 43.

Comment 3

[Georg Fritzsche [:gfritzsche]]

So, this might not be a large-scale issue, we are currently looking into the impact here.
Normal extensions appear to always have a null .description if there is none provided in the .rdf.

The cause in tests here is the SocialProvider and its AddonWrapper:
https://dxr.mozilla.org/mozilla-central/rev/66fb852962c0d5f6f5fe0604204da4f5d17763c9/toolkit/components/social/SocialService.jsm#1176
I assume some of those test manifests simply don't have a description, which might not be an issue on release.

Comment 4

[Eric Hu]

Attachment: Add safety checking for undefined value of aString. Prevents error on snippet code attempting to add an addon without a description.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8699933 [details] [diff] [review]
Add safety checking for undefined value of aString.  Prevents error on snippet code attempting to add an addon without a description.

Review of attachment 8699933 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 6

[Georg Fritzsche [:gfritzsche]]

We took this through good testing here locally, it looks like the main issue is with the fake addons the SocialProvider testing sets up.

Pushed this to try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db545b100c21

Comment 7

[Georg Fritzsche [:gfritzsche]]

Alexandra, can you verify this when it hits Nightly?
I would love to get this verified quickly so it can get uplifted to 44 & 45 soon.

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/01a10d89e8a1

Comment 9

[Georg Fritzsche [:gfritzsche]]

ni?dexter for filing a follow-up for test-coverage on this.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Related: bug 1190346 mentions malware addons etc. having missing .name, .version, .description

Comment 11

[Georg Fritzsche [:gfritzsche]]

Follow-up on fhr-dev:
https://mail.mozilla.org/pipermail/fhr-dev/2015-December/000745.html

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/01a10d89e8a1

Comment 13

[Alessio Placitelli [:Dexter] - PTO until 26/July]

(In reply to Georg Fritzsche [:gfritzsche] [away Dec 19 - Jan 3] from comment #11)
> Follow-up on fhr-dev:
> https://mail.mozilla.org/pipermail/fhr-dev/2015-December/000745.html

A larger sample [0] confirmed that the percentage of clients using addons/themes/plugins with no description or name is still the 14%.

[0] - https://gist.github.com/Dexterp37/20d2631e0b0090494500

Comment 14

[Alessio Placitelli [:Dexter] - PTO until 26/July]

Filed bug 1234197 for adding test coverage.

Comment 15

[Ada [:adalucinet]]

Successfully reproduced and verified this issue on builds provided by Alessio (thanks!), under Windows 7 64-bit; also, some additional exploratory testing was performed with latest 46.0a1 (from 2015-12-21), across platforms [1]

[1] Windows 10 32-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11

Comment 16

[Alessio Placitelli [:Dexter] - PTO until 26/July]

Comment on attachment 8699933 [details] [diff] [review]
Add safety checking for undefined value of aString.  Prevents error on snippet code attempting to add an addon without a description.

Approval Request Comment
[Feature/regressing bug #]: bug 1211404
[User impact if declined]: Users with broken addons (e.g. malware, etc.) will produce Telemetry pings with no addons, plugins and themes in the related environment's section. Moreover, the about:telemetry will not contain any data for addons, plugins and themes.
[Describe test coverage new/current, TreeHerder]: Manual QA verification.
[Risks and why]: Low risk, as this would impact on Telemetry environment's data collection for addons, plugins and themes, which is already broken for those clients.
[String/UUID change made/needed]: None

Comment 17

[Ritu Kothari (:ritu)]

Comment on attachment 8699933 [details] [diff] [review]
Add safety checking for undefined value of aString.  Prevents error on snippet code attempting to add an addon without a description.

This seems to address data quality issues in Telemetry for broken addon scenarios. Beta44+, Aurora45+

Comment 18

[Ritu Kothari (:ritu)]

I don't feel the need to track this bug as I would not block the FF44 release on this anyway.

Comment 19

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cee07fcf6676

Comment 20

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/releases/mozilla-beta/rev/47c9ab8f0eb6

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/47c9ab8f0eb6

Comment 22

[Andrei Vaida [:avaida]]

Setting [qe-verify+] once again for verification on 44b4.

Comment 23

[Ada [:adalucinet]]

Confirming this fix with a 44 beta build (Build ID: 20160104173512) provided by :Dexter, under Windows 7 64-bit, along with some additional exploratory testing performed with Firefox 44.0b4 (Build ID: 20151228134903), across platforms [1]

[1] Mac OS X 10.11, Ubuntu 14.04 32-bit, Windows 7 x64 and Windows 10 x64