1249123 - Add telemetry for __defineGetter__/__defineSetter__ this semenatics

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Tom S]

__defineGetter__ is special, because when invoked with null/undefined it will instead use the global object as |this| value.

JSC doesn't do this and we should measure how important this behavior is. See: https://github.com/tc39/ecma262/pull/381

Comment 1

[Tom S]

Attachment: v1 Measure __define{G,S}etter__ this values

I wasn't sure how to handle the telemetry part. Is adding a new probe okay or should I reuse JS_TELEMETRY_DEPRECATED_LANGUAGE_EXTENSIONS_*? Should the type of probe be flag or enumerated?

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 8720503 [details] [diff] [review]
v1 Measure __define{G,S}etter__ this values

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

Nice! I like the intrinsic a lot.

I guess the question is what it is exactly that we want to measure. If it's really just about undefined|null as the accessor receiver, then a flag would be fine. The enumerated setup you have now lets us measure overall usage of __define{G,S}etter__, so that's kinda nice, too. Then again, it's exceedingly unlikely that we'll see low enough usage to justify removing them in the version 55 time frame, so if the probe expires them, it'll all but certainly not have given us actionable information.

All in all, I think a flag makes sense.

In case you're not aware of that, you need a Telemetry peer's review for a new probe: https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval

::: js/src/vm/SelfHosting.cpp
@@ +1449,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args.length() == 2);
> +
> +    int id = args[0].toInt32();
> +    MOZ_ASSERT(id < JS_TELEMETRY_END);

Also assert >= 0?

::: toolkit/components/telemetry/Histograms.json
@@ +516,5 @@
>      "n_values": 10,
>      "description": "Use of SpiderMonkey's deprecated language extensions in add-ons: ForEach=0, DestructuringForIn=1 (obsolete), LegacyGenerator=2, ExpressionClosure=3, LetBlock=4 (obsolete), LetExpression=5 (obsolete), NoSuchMethod=6 (obsolete), FlagsArgument=7, RegExpSourceProp=8 (obsolete), RestoredRegExpStatics=9 (obsolete), BlockScopeFunRedecl=10"
>    },
> +  "JS_DEFINE_GETTER_SETTER_THIS": {
> +    "expires_in_version": "55",

I guess you'll need to add an alert email address.

@@ +519,5 @@
> +  "JS_DEFINE_GETTER_SETTER_THIS": {
> +    "expires_in_version": "55",
> +    "kind": "enumerated",
> +    "n_values": 2,
> +    "description": "__defineGetter__ or __defineSetter__ invoked with 0 = null/undefined or 1 = everything else as this value"

s/this value/|this| value/

Comment 3

[Tom S]

Attachment: v2 Measure __define{G,S}etter__ |this| values

Comment 4

[Benjamin Smedberg]

Comment on attachment 8722066 [details] [diff] [review]
v2 Measure __define{G,S}etter__ |this| values

data-review=me I'd encourage you to collect this opt-out, even, to get better representative data across locales and into enterprise environments which rarely use beta or flip telemetry on.

Comment 5

[Tom S]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #4)
> Comment on attachment 8722066 [details] [diff] [review]
> v2 Measure __define{G,S}etter__ |this| values
> 
> data-review=me I'd encourage you to collect this opt-out, even, to get
> better representative data across locales and into enterprise environments
> which rarely use beta or flip telemetry on.
"I'd encourage you to collect this opt-out," Excuse me, but I have no idea what this means. Do we have some fine grained opt-out mechanism?

@till
Should we ignore add-ons/trusted code in AddTelemetry? I am afraid add-ons might falsify our data: https://mxr.mozilla.org/addons/search?string=[^.\%22]__define[SG]etter__&regexp=on&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=addons

Comment 6

[Till Schneidereit [:till]]

Comment on attachment 8722066 [details] [diff] [review]
v2 Measure __define{G,S}etter__ |this| values

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

Nice, with this setup we get all the information we want, I think.

> Should we ignore add-ons/trusted code in AddTelemetry? I am afraid add-ons
> might falsify our data:

Yes, that is a good idea.

r=me with the opt-out change and addon/chrome compartments filtered out.

::: toolkit/components/telemetry/Histograms.json
@@ +527,5 @@
> +  "JS_DEFINE_GETTER_SETTER_THIS_NULL_UNDEFINED": {
> +    "alert_emails": ["[e-mail]"],
> +    "bug_numbers": [1249123],
> +    "expires_in_version": "55",
> +    "kind": "boolean",

I think you can make this opt-out by adding
"releaseChannelCollection": "opt-out",

Comment 7

[Tom S]

Attachment: v3 Measure __define{G,S}etter__ |this| values

Maybe you want to take a short look at the isProbablySystemOrAddonCode() function?

Comment 8

[Till Schneidereit [:till]]

Comment on attachment 8722616 [details] [diff] [review]
v3 Measure __define{G,S}etter__ |this| values

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

Ideally, the reporting would integrate with JSCompartment::reportTelemetry, which is called in ~JSCompartment.

This is ok for now though: we can do something more principled if and when we add more probes.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a2c2d8fc91f

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a2c2d8fc91f