JS histogram add functions should not throw

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
Telemetry
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: gfritzsche, Assigned: Dexter)

Tracking

(Blocks: 1 bug)

Trunk
mozilla53
Points:
1
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox53 fixed)

Details

(Whiteboard: [measurement:client])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
For calls like:
> h = Services.telemetry.getKeyedHistogramById("SEARCH_COUNTS");
> h.add("foo-bar", "1");

... we should warn, but not throw an exception.
Recent cross-checking between SEARCH_COUNTS and the engagement navigation search measurements in bug 1308705 point to slight discrepancies between the two which could be explained by rejected .add() calls.

We will need to figure out though how to get these warnings flagged as errors in test harnesses (xpcshell, mochitest, marionette), but this can happen in follow-up bugs.
(Reporter)

Updated

2 years ago
See Also: → bug 1321790
(Reporter)

Updated

2 years ago
Priority: P2 → P1
(Assignee)

Updated

a year ago
Assignee: nobody → alessio.placitelli
Points: --- → 1
(Assignee)

Comment 1

a year ago
Created attachment 8820207 [details] [diff] [review]
bug1315906.patch

This patch changes the |internal_JSHistogram_Add| so that it doesn't throw an exception but rather prints an error-like message to the console.

It behaves exactly the same as before, minus the exception.

Since the function is passed as a JSNative to JS, its behaviour is documented in [1]: to prevent it from throwing, we have to return |true| and replace the |JS_ReportErrorASCII|, since the latter sets a "pending exception" flag in the JS context.

The test_nsITelemetry.js test confirms that this change works.

[1] - https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JSNative
Attachment #8820207 - Flags: review?(gfritzsche)
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
(Assignee)

Updated

a year ago
Depends on: 1324774
(Assignee)

Comment 2

a year ago
(In reply to Georg Fritzsche [:gfritzsche] from comment #0)
> We will need to figure out though how to get these warnings flagged as
> errors in test harnesses (xpcshell, mochitest, marionette), but this can
> happen in follow-up bugs.

Filed bug 1324774 for that part.
(Reporter)

Comment 3

a year ago
Comment on attachment 8820207 [details] [diff] [review]
bug1315906.patch

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

What about keyed histograms?

::: toolkit/components/telemetry/TelemetryHistogram.cpp
@@ +1573,5 @@
>      // For categorical histograms we allow passing a string argument that specifies the label.
>      nsAutoJSString label;
>      if (!label.init(cx, args[0])) {
> +      LogToBrowserConsole(nsIScriptError::errorFlag, NS_LITERAL_STRING("Invalid string parameter"));
> +      return true;

From [1]:
"On success, the callback must set JS::CallArgs::rval() or call JS_SET_RVAL (at least once) and return true."
We should always return undefined from this function. Ditto below.

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/SpiderMonkey/JSAPI_reference/JSNative
Attachment #8820207 - Flags: review?(gfritzsche)
(Assignee)

Comment 4

a year ago
Created attachment 8820304 [details] [diff] [review]
bug1315906.patch

Good catch, I didn't notice we were not setting it before either.

Also, I've changed the keyed histograms version to match the same behaviour.
Attachment #8820207 - Attachment is obsolete: true
Attachment #8820304 - Flags: review?(gfritzsche)
(Reporter)

Comment 5

a year ago
Comment on attachment 8820304 [details] [diff] [review]
bug1315906.patch

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

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ +208,5 @@
>    }
>    for (let s of ["", "Label4", "1234"]) {
> +    // The |add| method should not throw for unexpected values, but rather
> +    // print an error message in the console.
> +    h1.add(s);

So, we don't have test coverage for this for the other histogram types (plain & keyed)?
Can you please add that?
Attachment #8820304 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 6

a year ago
Created attachment 8820356 [details] [diff] [review]
Part 2 - Add test coverage

This patch adds test coverage for the new |add| behaviour for plain and keyed histograms.
Attachment #8820356 - Flags: review?(gfritzsche)
(Reporter)

Comment 7

a year ago
Comment on attachment 8820356 [details] [diff] [review]
Part 2 - Add test coverage

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

::: toolkit/components/telemetry/tests/unit/test_nsITelemetry.js
@@ +235,5 @@
>  
> +add_task(function* test_add_error_behaviour() {
> +  const PLAIN_HISTOGRAMS_TO_TEST = [
> +    "TELEMETRY_TEST_FLAG", "TELEMETRY_TEST_EXPONENTIAL",
> +    "TELEMETRY_TEST_LINEAR", "TELEMETRY_TEST_BOOLEAN"

Nit: Let's just put each entry on a new line.
Ditto below.
Attachment #8820356 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 8

a year ago
Created attachment 8820606 [details] [diff] [review]
Part 2 - Add test coverage

Thanks for the review!
Attachment #8820356 - Attachment is obsolete: true
Attachment #8820606 - Flags: review+

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/02d37af6e76f
https://hg.mozilla.org/mozilla-central/rev/473bef59dcfa
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
I guess we don't want to uplift that to 52.
status-firefox52: affected → wontfix
(Assignee)

Comment 13

a year ago
(In reply to Sylvestre Ledru [:sylvestre] from comment #12)
> I guess we don't want to uplift that to 52.

Nope :)
You need to log in before you can comment on or make changes to this bug.