Closed Bug 1233438 Opened 5 years ago Closed 5 years ago
Addon data missing in environment if there is an addon with undefined .description
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  that aString is both non-null and non-undefined.  - https://dxr.mozilla.org/mozilla-central/rev/0babaa3edcf908c393b68a3dc2d1c2a2450c31ed/toolkit/components/telemetry/TelemetryEnvironment.jsm#264
Points: --- → 1
Priority: -- → P3
This can be reproduced by looking at the output of: mach test browser/base/content/test/social/browser_aboutHome_activation.js
[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.
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 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!
Attachment #8699933 - Flags: review?(gfritzsche) → review+
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
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.
QA Contact: alexandra.lucinet
ni?dexter for filing a follow-up for test-coverage on this.
Summary: "TypeError: aString is undefined" in browser tests at TelemetryEnvironment.jsm:267:3 → Addon data missing in environment if there is an addon with undefined .description
Related: bug 1190346 mentions malware addons etc. having missing .name, .version, .description
Follow-up on fhr-dev: https://mail.mozilla.org/pipermail/fhr-dev/2015-December/000745.html
(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  confirmed that the percentage of clients using addons/themes/plugins with no description or name is still the 14%.  - https://gist.github.com/Dexterp37/20d2631e0b0090494500
Filed bug 1234197 for adding test coverage.
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   Windows 10 32-bit, Ubuntu 12.04 32-bit and Mac OS X 10.11
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 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+
I don't feel the need to track this bug as I would not block the FF44 release on this anyway.
Setting [qe-verify+] once again for verification on 44b4.
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   Mac OS X 10.11, Ubuntu 14.04 32-bit, Windows 7 x64 and Windows 10 x64
You need to log in before you can comment on or make changes to this bug.