1509040 - Don't allow snapshotting in non-main processes

Status: RESOLVED FIXED

(Toolkit :: Telemetry, enhancement, P1)

Description

[Chris H-C :chutten]

A report on Telemetry from :Bas[1] shows that we permit content processes in mochitests to try to snapshot histograms.

Whoops.

We should either early-return or, in the case of the JS{Keyed}Histogram objects, omit unsupported functions from their prototypes.

We should also audit Scalars and Events to ensure that they do something sensible and predictable, too.

[1]: https://mozilla.logbot.info/telemetry/20181121#c15640130-c15640199

Comment 1

[Chris H-C :chutten]

Taking the bug to investigate alternatives and propose a solution.

Comment 2

[Chris H-C :chutten]

SCALARS AUDIT:
On non-parent processes we return `{}` for snapshots

EVENTS AUDIT:
On non-parent processes we return an error for snapshots.

HISTOGRAMS AUDIT:
Two ways to snapshot: `getSnapshotFor{Keyed}Histograms` and `JS{Keyed}HistogramInstance.snapshot()`
getSnapshotFor{Keyed}Histograms might successfully generate a snapshot with default values for all flag histograms, but it checks its return values so likely isn't an immediate problem.
JS{Keyed}HistogramInstance.snapshot() doesn't check its return values, so derefs a potentially-nullptr. 
JS{Keyed}HistogramInstance.clear() checks its return values, so likely isn't an immediate problem.

---

We need to throw some early returns in there. I vote for NS_ERROR_FAILURE to echo Events' behaviour.

We can optionally also not define "snapshot" and "clear" on JS{Keyed}Histogram objects in non-parent processes. I like this approach from a "tests with bad uses can't even pass by accident" point-of-view, but I don't know how widely this behaviour might be spread (and how e10s/Fennec might interfere).

Comment 3

[Chris H-C :chutten]

Investigation complete. Popping back out for retriage.

Comment 4

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1509040 - Don't allow snapshotting/clearing in non-parent processes

Comment 5

[Jan-Erik Rediger [:janerik]]

Attachment: Bug 1509040 - Remove useless wrapper function

Depends on D17920

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2ff9bc983ae7
Don't allow snapshotting/clearing in non-parent processes r=chutten
https://hg.mozilla.org/integration/autoland/rev/a5d034708f99
Remove useless wrapper function r=chutten

Comment 7

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/2ff9bc983ae7
https://hg.mozilla.org/mozilla-central/rev/a5d034708f99