Closed
Bug 1315906
Opened 8 years ago
Closed 8 years ago
JS histogram add functions should not throw
Categories
(Toolkit :: Telemetry, defect, P1)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: gfritzsche, Assigned: Dexter)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Whiteboard: [measurement:client])
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
gfritzsche
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Dexter
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
Reporter | ||
Updated•8 years ago
|
Priority: P2 → P1
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → alessio.placitelli
Points: --- → 1
Assignee | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•8 years 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•8 years 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•8 years ago
|
||
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•8 years 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•8 years ago
|
||
This patch adds test coverage for the new |add| behaviour for plain and keyed histograms.
Attachment #8820356 -
Flags: review?(gfritzsche)
Reporter | ||
Comment 7•8 years 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•8 years ago
|
||
Thanks for the review!
Attachment #8820356 -
Attachment is obsolete: true
Attachment #8820606 -
Flags: review+
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/02d37af6e76f
https://hg.mozilla.org/mozilla-central/rev/473bef59dcfa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 12•8 years ago
|
||
I guess we don't want to uplift that to 52.
Assignee | ||
Comment 13•8 years 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.
Description
•