Closed Bug 1211404 Opened 4 years ago Closed 4 years ago

Limit the length of addon description (& other text fields) in Telemetry

Categories

(Toolkit :: Telemetry, defect, P2)

defect
Points:
1

Tracking

()

VERIFIED FIXED
mozilla44
Tracking Status
firefox42 --- wontfix
firefox43 --- verified
firefox44 --- verified

People

(Reporter: Dexter, Assigned: Dexter)

References

(Blocks 1 open bug)

Details

(Whiteboard: [unifiedTelemetry][measurement:client])

Attachments

(4 files, 4 obsolete files)

In bug 1187864 comment 3 we found out that some addons are cluttering the pings with unnecessarily long names and descriptions.

We can mitigate this behaviour by setting an arbitrary length limit for the addon name, description & other text fields in Telemetry (100 chars).
Blocks: 1122482
Points: --- → 1
Depends on: 1187864
Priority: -- → P2
Whiteboard: [unifiedTelemetry][measurement:client]
Blocks: 1186986
No longer blocks: 1122482
I'm taking this one.
Assignee: nobody → alessio.placitelli
Flags: needinfo?(thuelbert)
Attached patch bug1211404.patch (obsolete) — Splinter Review
Test coverage coming in a separate patch.
Attachment #8669661 - Flags: review?(gfritzsche)
Comment on attachment 8669661 [details] [diff] [review]
bug1211404.patch

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

::: toolkit/components/telemetry/TelemetryEnvironment.jsm
@@ +38,5 @@
>  
>  const CHANGE_THROTTLE_INTERVAL_MS = 5 * 60 * 1000;
>  
> +// The maximum length of a string (e.g. addon description) in the environment section.
> +const MAX_ENVIRONMENT_STRING_LENGTH = 100;

We might not want to use the same limit everywhere.
Better make it specifically about the addon data now and revisit later as needed.

`MAX_ADDON_STRING_LENGTH` and update the comment accordingly.

@@ +259,5 @@
> + * @param {Integer} aMaxLength The maximum length of the returned substring. If this is
> + *        greater than the length of the input string, we return the whole input string.
> + * @return {String} The substring or null if the input string is null or undefined.
> + */
> +function getSubstring(aString, aMaxLength) {

getSubstring() is very generic, there is no readability improvement over just directly using .substring() at the callers side.
limitStringToLength() or something like that?
Attachment #8669661 - Flags: review?(gfritzsche) → feedback+
Attached patch bug1211404.patch (obsolete) — Splinter Review
Attachment #8669661 - Attachment is obsolete: true
Attachment #8669736 - Flags: review?(gfritzsche)
Attachment #8669736 - Flags: review?(gfritzsche)
Comment on attachment 8669736 [details] [diff] [review]
bug1211404.patch

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

Lets also update the docs as discussed on IRC.
Attachment #8669736 - Flags: review+
Thanks. As discussed, I've slightly changed the limitStringToLength to account for the AddonManager not strictly defining the behaviour of the "version" field.

Following up with the docs and the test patches.
Attachment #8669736 - Attachment is obsolete: true
Attachment #8669787 - Flags: review+
Attached patch part 3 - Update the docs. (obsolete) — Splinter Review
Attachment #8669796 - Flags: review?(gfritzsche)
Flags: needinfo?(thuelbert)
Attached patch part 2 - Add test coverage (obsolete) — Splinter Review
Attachment #8670140 - Flags: review?(gfritzsche)
Comment on attachment 8669796 [details] [diff] [review]
part 3 - Update the docs.

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

::: toolkit/components/telemetry/docs/environment.rst
@@ +302,5 @@
> +
> +activeAddons
> +~~~~~~~~~~~~
> +
> +Starting from Firefox 44, the length of some string fields (i.e. ``name``, ``description`` and ``version``) is limited to 100 characters. The same limitation applies to the same fields in ``theme`` and ``activePlugins``.

Instead of "some string fields" lets be precise and say "the following string fields: ...".
Attachment #8669796 - Flags: review?(gfritzsche) → review+
Comment on attachment 8670140 [details] [diff] [review]
part 2 - Add test coverage

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

r=me with the below addressed.

::: toolkit/components/telemetry/tests/addons/long-fields/install.rdf
@@ +16,5 @@
> +    </em:targetApplication>
> +
> +    <!-- Front End MetaData -->
> +    <em:name>This is a really long addon name, that will get limited to 100 characters. We're much longer, we're at about 113.</em:name>
> +    <em:description>This is a really long addon description, that will get limited to 100 characters. We're much longer, we're at about 120.</em:description>

I would make them both longer, filling it up with dummy text to 200 chars or so.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +1019,5 @@
>  
> +add_task(function* test_addonsFieldsLimit() {
> +  const ADDON_INSTALL_URL = gDataRoot + "long-fields.xpi";
> +  const ADDON_ID = "tel-longfields-xpi@tests.mozilla.org";
> +  const ADDON_INSTALL_DATE = truncateToDays(Date.now());

ADDON_INSTALL_DATE is unused, remove it.

@@ +1028,5 @@
> +    "limited to 100 characters. We're much longer, we're at about 116.").substring(0, 100);
> +  const EXPECTED_NAME_STRING = ("This is a really long addon name, that will get limited " +
> +    "to 100 characters. We're much longer, we're at about 113.").substring(0, 100);
> +  const EXPECTED_DESC_STRING = ("This is a really long addon description, that will get " +
> +    "limited to 100 characters. We're much longer, we're at about 120.").substring(0, 100);

I think its enough to just assert that the resulting string lengths are <=100, we don't need to assert & maintain the exact contents.

@@ +1040,5 @@
> +  yield AddonTestUtils.installXPIFromURL(ADDON_INSTALL_URL);
> +
> +  yield deferred.promise;
> +  // Unregister the listener.
> +  TelemetryEnvironment.unregisterChangeListener("test_longFieldsAddon");

Nit: This looks a bit hard to read, maybe:
// Install the addon and wait for the TelemetryEnvironment to pick it up.
let deferred = ...
...registerChangeListener...
yield ...installXPI
yield deferred...
...unregisterChangeListener
Attachment #8670140 - Flags: review?(gfritzsche) → review+
Attachment #8669796 - Attachment is obsolete: true
Attachment #8670662 - Flags: review+
Attachment #8670140 - Attachment is obsolete: true
Attachment #8670663 - Flags: review+
Approval Request Comment
[Feature/regressing bug #]: 1211404
[User impact if declined]: This doesn't impact on users. Instead, misbehaving addons reported by Telemetry could be arbitrarily big, increasing server storage costs.
[Describe test coverage new/current, TreeHerder]: This has been on m-c for almost a month now and has xpcshell coverage. There were no reported issues.
[Risks and why]: Low, doesn't change any user facing feature.
[String/UUID change made/needed]: None
Attachment #8683069 - Flags: approval-mozilla-beta?
Comment on attachment 8683069 [details] [diff] [review]
Fx_43 - Folded patches for Beta

Should have no impact to users, has tests & docs, taking it.
Should be in 43 beta 2.
Attachment #8683069 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
This can be verified by installing this test XPI with some long text fields [0]. Telemetry should limit those fields when reporting the addon.

[0] - https://drive.google.com/a/mozilla.com/file/d/0B1RACVTBwZ94MkQwU0ZYU1ljX1U/view?usp=sharing
Reproduced with Nightly 2015-10-08 on Ubuntu 14.04 32-bit.
Verified fixed with 43.0b2 (Build ID: 20151109145326) and 44.0a2 (from 2015-11-10), across platforms [1]: in about:telemetry page - Environment Data/Addons section, with the provided .xpi the value is limited to 100 characters.

[1] Mac OS X 10.11.1, Windows 7 64-bit and Ubuntu 14.04 32-bit
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1233438
You need to log in before you can comment on or make changes to this bug.