963312 - Implement GetJSEngineTelemetryValue on Components.utils, not nsXPConnect

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

Bug 670059 made nsXPConnect implement a new IDL interface, nsIJSEngineTelemetryStats.  All this does, nsXPConnect::GetTelemetryValue, is call some JS API functions without accessing any fields of nsXPConnect, so there surely must be some better way to implement this.

This requires updating
  toolkit/components/telemetry/TelemetryPing.jsm 
and also smaug found some addon r2d2b2g that would need to be updated in some trivial way.

In addition to general cleanliness, this appears to be the only way any addons or browser code access XPConnect from JS, so maybe with this out of the way we could de-XPIDL XPConnect.

Comment 1

[Bobby Holley (:bholley)]

Godspeed. Can we just put GetTelemetryValue on Components.utils?

Comment 2

[Bobby Holley (:bholley)]

Also, note that r2d2b2g is the b2g simulator, which is kind of a big deal.

Comment 3

[Andrew McCreight [:mccr8]]

(In reply to Bobby Holley (:bholley) from comment #2)
> Also, note that r2d2b2g is the b2g simulator, which is kind of a big deal.

Ah, yes.  Perhaps the way to do this is to add a new nicer interface, then at some later point deprecate the old one once we're sure the b2g simulator is updated.  It should be a simple fix, but we want to make sure it happens.

Comment 4

[Bobby Holley (:bholley)]

Yeah. Given that the simulator is actively maintained, I'm also fine with dropping the interface at the same time we create the new one, and telling them to add a conditional somewhere.

Comment 5

[Andrew McCreight [:mccr8]]

From talking to jst, it sounds like r2d2b2g just contains a copy of Gecko, so fixing the telemetry.jsm is probably enough.  We'll want to double check, of course.

Comment 6

[Andrew McCreight [:mccr8]]

With bonus marking nsIXPConnect noscript.  We'll see how that goes.
  https://tbpl.mozilla.org/?tree=Try&rev=d8ae5d0bddfd

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: Get JS engine telemetry values from Components.utils, not nsIXPConnect.

The body of the get telemetry value should be unchanged.

r?Taras for TelemetryPing.jsm, bholley for the rest.

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8365563 [details] [diff] [review]
Get JS engine telemetry values from Components.utils, not nsIXPConnect.

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

::: js/xpconnect/src/XPCComponents.cpp
@@ +3444,5 @@
>  
> +NS_IMETHODIMP
> +nsXPCComponents_Utils::GetJSEngineTelemetryValue(JSContext *cx, MutableHandleValue rval)
> +{
> +    RootedObject obj(cx, JS_NewObject(cx, nullptr, JS::NullPtr(), JS::NullPtr()));

nit - no JS:: prefixing in XPConnect cpp files. |using namespace JS|.

@@ +3452,5 @@
> +    unsigned attrs = JSPROP_ENUMERATE | JSPROP_READONLY | JSPROP_PERMANENT;
> +
> +    size_t i = JS_SetProtoCalled(cx);
> +    RootedValue v(cx, DoubleValue(i));
> +    if (!JS_DefineProperty(cx, obj, "setProto", v, nullptr, nullptr, attrs))

This should really pass JS_PropertyStub and JS_StrictPropertyStub instead of the nullptrs, since nullptr means 'use the PropertyOp from the JSClass', which is always scary (though for ObjectClass it's fine). You don't have to change it though.

Comment 9

[Andrew McCreight [:mccr8]]

Thanks for the fast reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/b182f29bb7b1

I kept the JS:: for NullPtr, because the compiler complained about ambiguity with some NullPtr() thing in the JS rooting API when I removed it.

a full try run was green: https://tbpl.mozilla.org/?tree=Try&rev=c6c74c797a99

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b182f29bb7b1