Closed Bug 1233438 Opened 4 years ago Closed 4 years ago

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

Categories

(Toolkit :: Telemetry, defect, P3)

defect
Points:
1

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox43 --- wontfix
firefox44 - verified
firefox45 --- fixed
firefox46 --- verified
b2g-v2.5 --- fixed

People

(Reporter: Dexter, Assigned: hu.eric, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [measurement:client])

Attachments

(1 file)

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
Points: --- → 1
Priority: -- → P3
Whiteboard: [measurement:client]
This can be reproduced by looking at the output of:
mach test browser/base/content/test/social/browser_aboutHome_activation.js
Assignee: nobody → pineapple.rice
Mentor: gfritzsche, alessio.placitelli
[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.
Flags: qe-verify+
Flags: needinfo?(alexandra.lucinet)
QA Contact: alexandra.lucinet
ni?dexter for filing a follow-up for test-coverage on this.
Flags: needinfo?(alessio.placitelli)
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
https://hg.mozilla.org/mozilla-central/rev/01a10d89e8a1
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(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
Filed bug 1234197 for adding test coverage.
Flags: needinfo?(alessio.placitelli)
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
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(alexandra.lucinet)
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
Attachment #8699933 - Flags: approval-mozilla-beta?
Attachment #8699933 - Flags: approval-mozilla-aurora?
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+
Attachment #8699933 - Flags: approval-mozilla-beta?
Attachment #8699933 - Flags: approval-mozilla-beta+
Attachment #8699933 - Flags: approval-mozilla-aurora?
Attachment #8699933 - Flags: approval-mozilla-aurora+
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.
Flags: qe-verify+
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
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.