1315906 - JS histogram add functions should not throw

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Georg Fritzsche [:gfritzsche]]

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.

Comment 1

[Alessio Placitelli [:Dexter]]

Attachment: 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

Comment 2

[Alessio Placitelli [:Dexter]]

(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.

Comment 3

[Georg Fritzsche [:gfritzsche]]

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

Comment 4

[Alessio Placitelli [:Dexter]]

Attachment: 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.

Comment 5

[Georg Fritzsche [:gfritzsche]]

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?

Comment 6

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add test coverage

This patch adds test coverage for the new |add| behaviour for plain and keyed histograms.

Comment 7

[Georg Fritzsche [:gfritzsche]]

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.

Comment 8

[Alessio Placitelli [:Dexter]]

Attachment: Part 2 - Add test coverage

Thanks for the review!

Comment 9

[Alessio Placitelli [:Dexter]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f70043cca0dd

Comment 10

[Alessio Placitelli [:Dexter]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/02d37af6e76fc7f8b13f62f9a7ddf82ccb8c5d45
Bug 1315906 - Change JS histogram add functions so that it doesn't throw. r=gfritzsche

https://hg.mozilla.org/integration/mozilla-inbound/rev/473bef59dcfa065110ec044a8326a3ccd78affaa
Bug 1315906 - Add test coverage. r=gfritzsche

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/02d37af6e76f
https://hg.mozilla.org/mozilla-central/rev/473bef59dcfa

Comment 12

[Sylvestre Ledru [:Sylvestre]]

I guess we don't want to uplift that to 52.

Comment 13

[Alessio Placitelli [:Dexter]]

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

Nope :)