Closed Bug 1249123 Opened 4 years ago Closed 4 years ago

Add telemetry for __defineGetter__/__defineSetter__ this semenatics

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

__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
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?
Assignee: nobody → evilpies
Status: NEW → ASSIGNED
Attachment #8720503 - Attachment is patch: true
Attachment #8720503 - Flags: feedback?(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/
Attachment #8720503 - Flags: feedback?(till)
Attachment #8720503 - Attachment is obsolete: true
Attachment #8722066 - Flags: review?(till)
Attachment #8722066 - Flags: review?(benjamin)
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.
Attachment #8722066 - Flags: review?(benjamin) → review+
(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 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": ["jdemooij@mozilla.com"],
> +    "bug_numbers": [1249123],
> +    "expires_in_version": "55",
> +    "kind": "boolean",

I think you can make this opt-out by adding
"releaseChannelCollection": "opt-out",
Attachment #8722066 - Flags: review?(till) → review+
Maybe you want to take a short look at the isProbablySystemOrAddonCode() function?
Attachment #8722616 - Attachment is patch: true
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.
Attachment #8722616 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/8a2c2d8fc91f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Blocks: 1253016
Duplicate of this bug: 1219366
Blocks: 1285750
You need to log in before you can comment on or make changes to this bug.