1848708 - Though Glean global is available in privileged "about:" contexts, none of the instrumentation APIs can be called due to CreateWrapperDeniedForOrigin errors

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect, P1)

Description

[Chris H-C :chutten]

I neglected to test beyond "Glean and GleanPings are available in privileged about contexts" and thus missed that no instrumentation APIs actually work. Trying e.g. Glean.aCategory.aName.record() returns Permission denied for <%2$S> to create wrapper for object of class %1$S.

Nika explains that XPCOM-wrapped JS isn't allowed in content contexts, regardless of privilege. So it seems as though we're going to have to go full webidl to make this work:

• Move dom/chrome-webidl/ interfaces to dom/webidl/ and replace [ChromeOnly] with [Func="nsGlobalWindowInner::IsGleanNeeded"]
• Move interfaces out of t/c/g/xpcom/nsIGleanMetrics.idl to webidl/, rewriting to webidl (no more AUTF8String. Learn to love DOMString. And drop those nsI prefixes. And nsISupports is now any or object.)
• Reimplement moved interfaces in new headers that you reference in dom/bindings/Bindings.conf
• Basically follow https://firefox-source-docs.mozilla.org/dom/webIdlBindings/index.html and see what happens.

It'd be nice if we could do this piecemeal, starting with only supporting Event since that's the interesting one, but I'm getting a sinking feeling that we'll have to convert this all in one go.

Comment 1

[Chris H-C :chutten]

Attachment: Bug 1848708 - Move Glean and GleanPings to plain webidl r?nika!,janerik!,perry.mcmanis!

Since we're exposing Glean types in not-chrome-only contexts like privileged
"about:" pages, we can't live in chrome-webidl/ any longer.

Comment 2

[Chris H-C :chutten]

Attachment: Bug 1848708 - Convert all Glean Metric types to webidl. r?nika!,janerik!,perry.mcmanis!

Depends on D186266

Comment 3

[Chris H-C :chutten]

ni?nika -- Is https://phabricator.services.mozilla.com/D186267 about right? I'm specifically wondering how much of my cargo cult is needed (can I just... drop? nsISupports? if we're not doing XPCOMish things?), whether my hunch about using metrics' WrapObject to generate the JSObject for Category and Labeled's NamedGetters are in the right ballpark, whether I'm safe to return nullptr from GetParentObject (and, if not, how I should think about "parent" here).

All in all, I've been finding the docs to be a lovely resource. I just want to make sure I'm not completely off course before I take care of the rest of the interfaces.

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Clearing the ni? as I've left some comments.

Comment 5

[Chris H-C :chutten]

Attachment: WIP: Bug 1848708 - Support UniquePtr for non-refcounted dom bindings r?nika!

Comment 6

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:chutten, could you have a look please?

For more information, please visit BugBot documentation.

Comment 7

[Chris H-C :chutten]

Sorry, bugbug, I forgot that one.

Since I'm here, might as well supply an update. I'm working through review comments, hoping to have a new version up for reviewers early-mid next week. Other things, like setting fires in the woods (camping. I was camping for three days), keep taking priority but this is still on my desk.

Comment 8

[Nick Alexander :nalexander [he/him] (PTO August 22-September 12)]

chutten: it looks like this patch does not impact Glean metric generation in a way that would directly impact Bug 1852098. Can you confirm? Thanks!

Comment 9

[Chris H-C :chutten]

Correct. The scope of this problem is limited to JS modules in non-Chrome contexts, and Use Counters are managed entirely within C++.

Comment 10

[Chris H-C :chutten]

Attachment: Bug 1848708 - Glean test methods now return null instead of undefined r?rpl!,florian!

Depends on D186267

Comment 11

[Chris H-C :chutten]

Attachment: Bug 1848708 - Corner case for event extra with undefined/null value r?perry.mcmanis!

Not sure this is the ideal semantics, but it appeases some tests that depend on
the behaviour.

Depends on D187812

Comment 12

[Chris H-C :chutten]

Attachment: Bug 1848708 - Update 'new metric type' dev docs for refactor r?perry.mcmanis!,janerik!

Depends on D187813

Comment 13

[Chris H-C :chutten]

Attachment: Bug 1848708 - testGetValue now returns null not undefined on no value r?pbz!

Also: add a missing await for testClickResultTelemetry and,
since this is a multiprocess test, ensure we always wait for FOG IPC to flush
before clearing data.

Depends on D187937

Comment 14

[Chris H-C :chutten]

Attachment: Bug 1848708 - testGetValue returns null now r?perry.mcmanis!

Depends on D187938

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/726d5138b1d2
Move Glean and GleanPings to plain webidl r=nika,janerik,perry.mcmanis
https://hg.mozilla.org/integration/autoland/rev/06061d509ab5
Convert all Glean Metric types to webidl. r=nika,janerik,perry.mcmanis,application-update-reviewers,nalexander
https://hg.mozilla.org/integration/autoland/rev/1261ba650483
Glean test methods now return null instead of undefined r=florian,extension-reviewers,rpl
https://hg.mozilla.org/integration/autoland/rev/d230a932c850
Corner case for event extra with undefined/null value r=perry.mcmanis,emilio
https://hg.mozilla.org/integration/autoland/rev/f1a77753fa0c
Update 'new metric type' dev docs for refactor r=perry.mcmanis,janerik
https://hg.mozilla.org/integration/autoland/rev/242478709593
testGetValue now returns null not undefined on no value r=pbz
https://hg.mozilla.org/integration/autoland/rev/e63a855c0acf
testGetValue returns null now r=perry.mcmanis

Comment 16

[Narcis Beleuzu [:NarcisB]]

Backed out for bustages on TimingDistribution.cpp

Backout link: https://hg.mozilla.org/integration/autoland/rev/1105d2849f18b5435e9333a0a66d820178af01f0
Log link: https://treeherder.mozilla.org/logviewer?job_id=429686709&repo=autoland&lineNumber=64640

Comment 17

[Chris H-C :chutten]

Ah, non-unified builds are Tier 1 now. Lemme tighten up these includes and try another local build before I resubmit.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a88fa44abdc9
Move Glean and GleanPings to plain webidl r=nika,janerik,perry.mcmanis
https://hg.mozilla.org/integration/autoland/rev/b5f79a977cd5
Convert all Glean Metric types to webidl. r=nika,janerik,perry.mcmanis,application-update-reviewers,nalexander
https://hg.mozilla.org/integration/autoland/rev/87d76dd03552
Glean test methods now return null instead of undefined r=florian,extension-reviewers,rpl
https://hg.mozilla.org/integration/autoland/rev/074f9e36e8a0
Corner case for event extra with undefined/null value r=perry.mcmanis,emilio
https://hg.mozilla.org/integration/autoland/rev/e2c9789f7de1
Update 'new metric type' dev docs for refactor r=perry.mcmanis,janerik
https://hg.mozilla.org/integration/autoland/rev/7369ca94287c
testGetValue now returns null not undefined on no value r=pbz
https://hg.mozilla.org/integration/autoland/rev/d63842cc4cf9
testGetValue returns null now r=perry.mcmanis

Comment 19

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/a88fa44abdc9
https://hg.mozilla.org/mozilla-central/rev/b5f79a977cd5
https://hg.mozilla.org/mozilla-central/rev/87d76dd03552
https://hg.mozilla.org/mozilla-central/rev/074f9e36e8a0
https://hg.mozilla.org/mozilla-central/rev/e2c9789f7de1
https://hg.mozilla.org/mozilla-central/rev/7369ca94287c
https://hg.mozilla.org/mozilla-central/rev/d63842cc4cf9