Closed Bug 1417473 Opened 7 years ago Closed 6 years ago

Implement the Hybrid Content Telemetry API

Categories

(Toolkit :: Telemetry, enhancement, P1)

Product:
Toolkit
Component:
Telemetry
Type:
enhancement
Points:
3

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: Dexter, Assigned: Dexter)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1417473 - Implement the hybrid content telemetry API.
59 bytes, text/x-review-board-request
chutten
: review+
Gijs
: review+
Details
Bug 1417473 - Add the docs for hybrid content telemetry.
59 bytes, text/x-review-board-request
chutten
: review+
gfritzsche
: review+
Details
Bug 1417473 - Enable the hybrid content telemetry API.
59 bytes, text/x-review-board-request
chutten
: review+
Details 
This bug is about implementing the HCT API as described in the design document that lives here: https://docs.google.com/document/d/1yaTNs-OuttHWuHSTM4nH9n1HOmqSiBtEQkrge-SvACo/edit#heading=h.l7t1rh78yzzj
Blocks: 1416718
Points: --- → 3
Priority: -- → P3
Assignee: nobody → alessio.placitelli
Priority: P3 → P1
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review211458

::: browser/base/content/browser.js:1254
(Diff revision 1)
>  
>      let mm = window.getGroupMessageManager("browsers");
>      mm.loadFrameScript("chrome://browser/content/tab-content.js", true);
>      mm.loadFrameScript("chrome://browser/content/content.js", true);
>      mm.loadFrameScript("chrome://browser/content/content-UITour.js", true);
> +    mm.loadFrameScript("chrome://global/content/content-HybridContentTelemetry.js", true);

Is there any way to inject this only to pages we care about?

::: toolkit/components/telemetry/hybrid_content/content-HybridContentTelemetry.js:111
(Diff revision 1)
> +};
> +
> +// The following function installs the handler for "mozTelemetry"
> +// events in the chrome. Please note that the name of the message (i.e.
> +// "mozTelemetry") needs to match the one in HybridContentTelemetry-lib.js.
> +addEventListener("mozTelemetry", HybridContentTelemetryListener, false, true);

Can we register this listener only for pages that have the required permission? (i.e. "hc_telemetry")
Gijs, we went through the security review with :freddyb and :pauljt and there was no major problem (see the blocking bug). A couple of good to have things were highlighted: any chance you have suggestions on how to deal with the issues on comment 4?

Any doc/example would be really appreciated.
Flags: needinfo?(gijskruitbosch+bugs)
Generally, content scripts live for the duration of a docshell (ie tab content <browser> element). They don't live in the scope of webpages, so there's no "polluting the page's DOM" or anything like that. Only the pages that care about this will source the -lib.js file itself, so I think we're good in that department.

In terms of the event listener, because multiple pages can load in the same docshell, and frame scripts are there all the time, it's not really possible to restrict things in the way you suggest, unless we have a unified way of waiting for document loads and only attaching event handlers to the relevant documents. I'm not aware of us having an existing bit of infrastructure for this, so you would need to wait for load events of some description yourself, and then attach the handler to the relevant document, and be careful about leaks.

Christoph recently talked to me about coming up with some infrastructure for this, but I do not believe it exists at this point.

Does that help?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(alessio.placitelli)
Blocks: 1423600
(In reply to :Gijs from comment #6)
> Generally, content scripts live for the duration of a docshell (ie tab
> content <browser> element). They don't live in the scope of webpages, so
> there's no "polluting the page's DOM" or anything like that. Only the pages
> that care about this will source the -lib.js file itself, so I think we're
> good in that department.
> 
> In terms of the event listener, because multiple pages can load in the same
> docshell, and frame scripts are there all the time, it's not really possible
> to restrict things in the way you suggest, unless we have a unified way of
> waiting for document loads and only attaching event handlers to the relevant
> documents. I'm not aware of us having an existing bit of infrastructure for
> this, so you would need to wait for load events of some description
> yourself, and then attach the handler to the relevant document, and be
> careful about leaks.
> 
> Christoph recently talked to me about coming up with some infrastructure for
> this, but I do not believe it exists at this point.
> 
> Does that help?

It does, thanks Gijs! As for the new infrastructure, I filed bug 1423600 to port this over once it's ready. My understanding is that it won't be ready before the beginning of Q2.
Flags: needinfo?(alessio.placitelli)
Thanks for investigating this Alessio (and thanks for the info here Gijs, quite helpful and I'll keep in mind for future reviews)
Attachment #8933244 - Flags: review?(gijskruitbosch+bugs)
Attachment #8933244 - Flags: review?(chutten)
@Gijs, would you kindly have a look and review the non-telemetry parts? This was already reviewed by security (except for the |canUpload| part, which is going to happen in parallel). Feel free to have a look at the second commit as well, if that's interesting for you, as it contains the documentation page. Your perspective might prove very valuable there too!

@Chris, would you kindly review the telemetry bits? You might also want to give an high level look at the rest of the patch, if interested :) What do you think about having the validation/data sanitization just in the Telemetry core (current state)? Do you think we're robust enough to handle garbage being injected without the risk of exploding?

I've also flagged you on the docs, Georg is going to take an high level look at that too, if he manages too.
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review211904

Only a field of nits.

::: browser/components/nsBrowserGlue.js:3049
(Diff revision 2)
>  globalMM.addMessageListener("UITour:onPageEvent", function(aMessage) {
>    UITour.onPageEvent(aMessage, aMessage.data);
>  });
> +
> +// Listen for HybridContentTelemetry messages.
> +// As for UITour messages, do it here instead of HybridContentTelemetry.init() so that

Probably best to nix "As for UITour messages, " in case they move out of the file.

::: toolkit/components/telemetry/hybrid_content/HybridContentTelemetry-lib.js:46
(Diff revision 2)
> +    document.dispatchEvent(event);
> +  }
> +
> +  /**
> +   * This internal function is used to register the policy handler. This is
> +   * needed by pages that do not want to use Telemetry but still needs to

s/still needs/still need/

::: toolkit/components/telemetry/hybrid_content/HybridContentTelemetry.jsm:82
(Diff revision 2)
> +        Services.obs.removeObserver(this, "profile-before-change");
> +        this._observerInstalled = false;
> +        break;
> +      case "nsPref:changed":
> +        if (aData != TelemetryUtils.Preferences.FhrUploadEnabled) {
> +          // Only we only watch for pref changes and are only interested

s/Only w/W/

::: toolkit/components/telemetry/hybrid_content/HybridContentTelemetry.jsm:135
(Diff revision 2)
> +            // the Telemetry API expects them to be "null" if something is being
> +            // passed.
> +            Services.telemetry.recordEvent(aData.category,
> +                                           aData.method,
> +                                           aData.object,
> +                                           aData.value ? aData.value : null,

Should these be ("value" in aData) ? aData.value : null so we support the values false, "", and 0?

::: toolkit/components/telemetry/hybrid_content/HybridContentTelemetry.jsm:136
(Diff revision 2)
> +            // passed.
> +            Services.telemetry.recordEvent(aData.category,
> +                                           aData.method,
> +                                           aData.object,
> +                                           aData.value ? aData.value : null,
> +                                           aData.extra ? aData.extra : null);

Ditto

::: toolkit/components/telemetry/hybrid_content/content-HybridContentTelemetry.js:144
(Diff revision 2)
> +        typeof(aMessage.data.canUpload) != "boolean") {
> +      this._log.warn("receiveMessage - received an unexpected message.");
> +      return;
> +    }
> +
> +    // Finally send the message back to the page.

s/back/down/

::: toolkit/components/telemetry/tests/browser/browser_HybridContentTelemetry.js:44
(Diff revision 2)
> +}
> +
> +/**
> + * Remove the trailing null/undefined from an event definition.
> + * This is useful for comparing the sample events (that might
> + * contain null/undefined) to the data from te snapshot (which might

s/te/the/

::: toolkit/components/telemetry/tests/browser/browser_HybridContentTelemetry.js:117
(Diff revision 2)
> +
> +add_task(async function test_untrusted_non_whitelisted_origin() {
> +  Services.telemetry.clearEvents();
> +
> +  // Install a custom handler that intercepts hybrid content telemetry messages
> +  // and makes the test fail. We don't expect any message from non secure contexts.

Ah, but it is a secure context

::: toolkit/components/telemetry/tests/browser/browser_HybridContentTelemetry.js:207
(Diff revision 2)
> +  const TEST_EVENT_CATEGORY = "telemetry.test.hct";
> +  const RECORDED_TEST_EVENTS = [
> +    [TEST_EVENT_CATEGORY, "test1", "object1"],
> +    [TEST_EVENT_CATEGORY, "test2", "object1", null, {"key1": "foo", "key2": "bar"}],
> +    [TEST_EVENT_CATEGORY, "test2", "object1", "some value"],
> +    [TEST_EVENT_CATEGORY, "test1", "object1", undefined, null],

Add another one with a falsey-but-valid value (like "")?
Attachment #8933244 - Flags: review?(chutten) → review-
Comment on attachment 8933245 [details]
Bug 1417473 - Add the docs for hybrid content telemetry.

https://reviewboard.mozilla.org/r/204190/#review211922

Another field of nits. One larger question:

Do we have to wait all the way to document "load" for the API to be available? I would think it would be available immediately.

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:20
(Diff revision 3)
> +
> +The API
> +=======
> +The hybrid content API is available to the web content through the inclusion of the `HybridContentTelemetry-lib.js <https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/hybrid_content/HybridContentTelemetry-lib.js>`_ library.
> +
> +

Extra line of whitespace. Intentional?

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:37
(Diff revision 3)
> +
> +These functions will not throw. If an unsupported operation is performed (e.g. writing to an unkown event) an error will be logged to the browser console if the Telemetry logging :doc:`preferences <../internals/preferences>` are set.
> +
> +.. note::
> +
> +    Data collected using this API will always respect the user Telemetry preferences: if a user has chosen to not send Telemetry data to Mozilla servers, telemetry from hybrid content pages will not be sent as well.

lowercase "telemetry" detected.

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:38
(Diff revision 3)
> +These functions will not throw. If an unsupported operation is performed (e.g. writing to an unkown event) an error will be logged to the browser console if the Telemetry logging :doc:`preferences <../internals/preferences>` are set.
> +
> +.. note::
> +
> +    Data collected using this API will always respect the user Telemetry preferences: if a user has chosen to not send Telemetry data to Mozilla servers, telemetry from hybrid content pages will not be sent as well.
> +    As for other Telemetry data, it will still be recorded locally and available through ``about:telemetry``.

"As for" conversationally gains meaning along the lines of "This other class will be treated differently.

"You three are free to go. As for _you_ Mr. Phelps, you must stay behind."

In this case I think we want "Like"

"Like other Telemetry data these events will be recorded locally and available through ``about:telemetry``."

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:47
(Diff revision 3)
> +
> +.. code-block:: js
> +
> +  Mozilla.ContentTelemetry.canUpload();
> +
> +This function returns true if the browser is allowed to send collected data to Mozilla servers (i.e. ``datareporting.healthreport.uploadEnabled`` is ``true``, see :doc:`preferences <../internals/preferences>`), false otherwise.

Put the "see :doc:" part at the end, for flow.

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:68
(Diff revision 3)
> +
> +.. code-block:: js
> +
> +  Mozilla.ContentTelemetry.registerEvents(category, eventData);
> +
> +Register new dynamic events from the content. This accepts the same parameters and is subject to the same limitation of ``Services.telemetry.registerEvents()``. See the `events` documentation for a comprehensive discussion.

*same limitations as

Link to the events documentation.

s/a comprehensive discussion/the definitive reference/

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:96
(Diff revision 3)
> +
> +.. code-block:: js
> +
> +  Mozilla.ContentTelemetry.recordEvent(category, method, object, value, extra);
> +
> +Record a registered event. This accepts the same parameters and is subject to the same limitation of ``Services.telemetry.recordEvent()``. See the `events` documentation for a comprehensive discussion.

Same comments as before

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:115
(Diff revision 3)
> +
> +Adding content data collection
> +==============================
> +Telemetry can be sent from web content by:
> +
> +1. granting the host privileges in the Firefox codebase;

Which host? Recommend "the web content's host".

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:118
(Diff revision 3)
> +Telemetry can be sent from web content by:
> +
> +1. granting the host privileges in the Firefox codebase;
> +2. including the ``HybridContentTelemetry-lib.js`` file in the page;
> +3. registering the probes after the library is loaded;
> +4. using the API to send telemetry.

lowercase Telemetry detected

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:122
(Diff revision 3)
> +3. registering the probes after the library is loaded;
> +4. using the API to send telemetry.
> +
> +Granting the privileges
> +-----------------------
> +For security/privacy reasons `Mozilla.ContentTelemetry` will only work on a list of allowed secure origins. The list of allowed origins can be found in `browser/app/permissions <https://dxr.mozilla.org/mozilla-central/source/browser/app/permissions>`_ . An host needs to be given the ``hc_telemetry`` permission in order to be whitelisted.

"host" contains a voiced 'h' so using "An" looks archaic. Modern usage is "A host"
(In reply to Alessio Placitelli [:Dexter] from comment #11)
> @Chris, would you kindly review the telemetry bits? You might also want to
> give an high level look at the rest of the patch, if interested :) What do
> you think about having the validation/data sanitization just in the
> Telemetry core (current state)? Do you think we're robust enough to handle
> garbage being injected without the risk of exploding?

I'm fine with sanitizing in a single place. We already use rigid typing and strict length checks in C++, so to my knowledge we're not opening up a new class of attack here by not checking the types of parameters.
Comment on attachment 8933245 [details]
Bug 1417473 - Add the docs for hybrid content telemetry.

https://reviewboard.mozilla.org/r/204190/#review211922

We require `HybridContentTelemetry-lib.js` to be fully loaded and the `document` object to be ready. I'm not sure if there's anything else that we can suggest using that happens before `load`. Gijs might have suggestions there.

> "As for" conversationally gains meaning along the lines of "This other class will be treated differently.
> 
> "You three are free to go. As for _you_ Mr. Phelps, you must stay behind."
> 
> In this case I think we want "Like"
> 
> "Like other Telemetry data these events will be recorded locally and available through ``about:telemetry``."

Oh! Great point, thanks!
Comment on attachment 8933245 [details]
Bug 1417473 - Add the docs for hybrid content telemetry.

https://reviewboard.mozilla.org/r/204190/#review212448
Attachment #8933245 - Flags: review?(chutten) → review+
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review213112

I haven't read the test yet, but there's probably enough feedback for now, plus I'm a little confused that there's no new patch since chutten's r- ?

::: toolkit/components/telemetry/hybrid_content/HybridContentTelemetry-lib.js:1
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

We don't use underscores in any non-test filenames or folder names in JS land. We use dashes everywhere. Please do the same. :-)

::: toolkit/components/telemetry/hybrid_content/HybridContentTelemetry-lib.js:46
(Diff revision 2)
> +    document.dispatchEvent(event);
> +  }
> +
> +  /**
> +   * This internal function is used to register the policy handler. This is
> +   * needed by pages that do not want to use Telemetry but still needs to

I'm confused that these issues have been marked as resolved but no new patch is online...

::: toolkit/components/telemetry/hybrid_content/HybridContentTelemetry-lib.js:54
(Diff revision 2)
> +  function _registerInternalPolicyHandler() {
> +    // Register the handler that will update the policy boolean.
> +    function policyChangeHandler(updatedPref) {
> +      if (!("detail" in updatedPref) ||
> +          !("canUpload" in updatedPref.detail) ||
> +          typeof(updatedPref.detail.canUpload) != "boolean") {

Nit: `typeof` is an operator in JS, you don't need the method-style `()`. Prevailing style elsewhere in browser JS is to omit them (and you do this in some other places in this file, but not everywhere). So please omit them. :-)

::: toolkit/components/telemetry/hybrid_content/HybridContentTelemetry-lib.js:61
(Diff revision 2)
> +    // Catch the unload event to clean up the policy handler as well.
> +    window.addEventListener("unload", function () {
> +      document.removeEventListener("mozTelemetryPolicyChange", policyChangeHandler);
> +    }, {once: true});

This runs inside webpages, right? Why would this be necessary? When the page is removed, this script will be removed too and so there's no need to have a manual unload handler. Instead, the unload handler will break the bfcache...

::: toolkit/components/telemetry/hybrid_content/HybridContentTelemetry-lib.js:71
(Diff revision 2)
> +    // Make sure the chrome is initialized.
> +    _sendMessageToChrome("init");
> +  };
> +
> +  Mozilla.ContentTelemetry.canUpload = function() {
> +    return Mozilla.ContentTelemetry._canUpload;

This can still be overridden by other scripts on the page. Why not make it completely private?

::: toolkit/components/telemetry/hybrid_content/HybridContentTelemetry.jsm:33
(Diff revision 2)
> +  /**
> +   * Lazily initialized observer for the Telemetry upload preference. This is
> +   * only ever executed if a page uses hybrid content telemetry and has enough
> +   * privileges to run it.
> +   */
> +  _lazyObserverInit() {
> +    if (this._observerInstalled) {
> +      // We only want to install the observer once, if needed.
> +      return;
> +    }
> +    this._log.trace("_lazyObserverInit - installing the pref observer.");
> +    this._uploadEnabled =
> +      Services.prefs.getBoolPref(TelemetryUtils.Preferences.FhrUploadEnabled, false);
> +    Services.prefs.addObserver(TelemetryUtils.Preferences.FhrUploadEnabled, this, true);
> +    this._observerInstalled = true;
> +
> +    // Catch shutdown, this is needed to unwatch the preference changes.
> +    Services.obs.addObserver(this, "profile-before-change");

Don't do this, use XPCOMUtils.defineLazyPreferenceGetter instead.

::: toolkit/components/telemetry/hybrid_content/HybridContentTelemetry.jsm:73
(Diff revision 2)
> +  },
> +
> +  /**
> +   * Handle the preference changes and shutdown topics.
> +   */
> +  observe(aSubject, aTopic, aData) {

All of this can go away by just using the XPCOMUtils utility instead.

::: toolkit/components/telemetry/hybrid_content/content-HybridContentTelemetry.js:45
(Diff revision 2)
> +    let uri = content.document.documentURIObject;
> +    if (uri.schemeIs("chrome")) {
> +      return true;
> +    }

Don't do this, use `content.document.nodePrincipal` instead, and check whether or not it is system principal.

::: toolkit/components/telemetry/hybrid_content/content-HybridContentTelemetry.js:57
(Diff revision 2)
> +      return false;
> +    }
> +
> +    // If this is not a chrome page, it needs to have the
> +    // HCT_PERMISSION to use this API.
> +    let permission = Services.perms.testPermission(uri, HCT_PERMISSION);

Test the permission with a principal instead of a uri, please.

::: toolkit/components/telemetry/hybrid_content/content-HybridContentTelemetry.js:83
(Diff revision 2)
> +    if (!Services.prefs.getBoolPref(TelemetryPrefs.HybridContentEnabled, false)) {
> +      this._log.trace("handleEvent - hybrid content telemetry is disabled.");
> +      return;
> +    }

Instead of manually checking the pref here, where it could potentially be manipulated if the content process was compromised anyway, could we just unload the process scripts from the parent process when the pref is flipped?

::: toolkit/components/telemetry/hybrid_content/content-HybridContentTelemetry.js:110
(Diff revision 2)
> +    // message, the manager is smart enough to ignore all the registration
> +    // after the first one. See the MDN docs for
> +    // nsIMessageListenerManager.addMessageListener()
> +    // Note that the name of the message must match the name of the one
> +    // in HybridContentTelemetry.jsm.
> +    addMessageListener(HCT_POLICY_CHANGE_MSG, this);

This doesn't make much sense to me. We never remove the message listener, from what I can tell, and it seems pretty ineffecient to just re-add it every single time, even if the message manager is OK with that. Can we fix that?

::: toolkit/components/telemetry/hybrid_content/content-HybridContentTelemetry.js:145
(Diff revision 2)
> +    let doc = content.document;
> +    doc.dispatchEvent(new doc.defaultView.CustomEvent("mozTelemetryPolicyChange", {

You could just omit the temp and do:

```js
content.document.dispatchEvent(new content.CustomEvent(...));
```

as `content.document.defaultView` is equivalent to just `content`.
Attachment #8933244 - Flags: review?(gijskruitbosch+bugs)
(In reply to Alessio Placitelli [:Dexter] from comment #15)
> Comment on attachment 8933245 [details]
> Bug 1417473 - Add the docs for hybrid content telemetry.
> 
> https://reviewboard.mozilla.org/r/204190/#review211922
> 
> We require `HybridContentTelemetry-lib.js` to be fully loaded and the
> `document` object to be ready. I'm not sure if there's anything else that we
> can suggest using that happens before `load`.

What does "ready" mean here?

Off-hand, if you just need document.nodePrincipal, that's available from the creation of the document object, which will be alive before the script is loaded, so you could just start using things from the point where HybridContentTelemetry-lib.js has fully loaded. Does that help?
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review211904

> Should these be ("value" in aData) ? aData.value : null so we support the values false, "", and 0?

Yes, good point. We should also add a special check for "undefined" though.
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review213112

> We don't use underscores in any non-test filenames or folder names in JS land. We use dashes everywhere. Please do the same. :-)

Ah, I didn't know that, thank you for mentioning it!

> I'm confused that these issues have been marked as resolved but no new patch is online...

I think that's because of the way I'm using Mozreview: I "Fix" or "Drop" issues while working on the patch to keep track of them. I usually push the updated patch once I've done with all the issues. Sorry for the confusion, I'll be pushing the updated patch shortly.

> This runs inside webpages, right? Why would this be necessary? When the page is removed, this script will be removed too and so there's no need to have a manual unload handler. Instead, the unload handler will break the bfcache...

Ah, great catch, I didn't know that.

> Don't do this, use XPCOMUtils.defineLazyPreferenceGetter instead.

Ah, good, I didn't know about that!

> Instead of manually checking the pref here, where it could potentially be manipulated if the content process was compromised anyway, could we just unload the process scripts from the parent process when the pref is flipped?

This makes sense. However, I'd like to be able to switch hybrid content telemetry on without restarting Firefox. Let's check for the pref in the .jsm file, without unloading it when it's flipped off. Does it make sense to you? It could also be reasonable to still check the pref in the content script as well: this way we can prevent messages from going to the parent in the context of a non compromised content process.

> This doesn't make much sense to me. We never remove the message listener, from what I can tell, and it seems pretty ineffecient to just re-add it every single time, even if the message manager is OK with that. Can we fix that?

I'm not entirely sure what's the best place to remove this listener. Suggestions? As far as I can tell, UITour doesn't seem to remove the similar listener.
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review213112

> This can still be overridden by other scripts on the page. Why not make it completely private?

Is this the right way to fix this?
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review213926

::: toolkit/components/telemetry/hybrid-content/HybridContentTelemetry.jsm:53
(Diff revision 4)
> +    if (!Services.prefs.getBoolPref(TelemetryUtils.Preferences.HybridContentEnabled, false)) {
> +      this._log.trace("onTelemetryMessage - hybrid content telemetry is disabled.");
> +      return;
> +    }

Can you add a lazy pref getter for this, too?

::: toolkit/components/telemetry/hybrid-content/content-HybridContentTelemetry.js:42
(Diff revision 4)
> +   * from a trusted origin.
> +   * @return {Boolean} true if we are on a trusted page, false otherwise.
> +   */
> +  isTrustedOrigin() {
> +    // Only allow top-level browsing context to use the API.
> +    if (content.top != content) {

This doesn't actually test what it says it tests. `content` is always the top frame, I believe. You would need to pass the event and check it's come from the right document.

Sorry for noticing this before. If you copied this from elsewhere, or know of other code that does the check this way, please file separate follow-up bugs.

::: toolkit/components/telemetry/hybrid-content/content-HybridContentTelemetry.js:85
(Diff revision 4)
> +    if (!Services.prefs.getBoolPref(TelemetryPrefs.HybridContentEnabled, false)) {
> +      this._log.trace("handleEvent - hybrid content telemetry is disabled.");
> +      return;
> +    }

Sadly, checking prefs isn't free. Can you cache the pref?

::: toolkit/components/telemetry/hybrid-content/content-HybridContentTelemetry.js:128
(Diff revision 4)
> +    if (!Services.prefs.getBoolPref(TelemetryPrefs.HybridContentEnabled, false)) {
> +      this._log.trace("receiveMessage - hybrid content telemetry is disabled.");
> +      return;
> +    }

We trust the parent process, so checking the pref here isn't useful.

::: toolkit/components/telemetry/tests/browser/browser_HybridContentTelemetry.js:122
(Diff revision 4)
> +  // and makes the test fail. We don't expect any message from non whitelisted pages.
> +  const messageName = "HybridContentTelemetry:onTelemetryMessage";
> +  let makeTestFail = () => ok(false, `Received an unexpected ${messageName}.`);
> +  Services.mm.addMessageListener(messageName, makeTestFail);
> +
> +  // Try to use the API on asecure host but don't give the page enough privileges.

nit: a secure

::: toolkit/components/telemetry/tests/browser/browser_HybridContentTelemetry.js:160
(Diff revision 4)
> +  // Install a custom handler that intercepts hybrid content telemetry messages
> +  // and makes the test fail. We don't expect any message from non secure contexts.

This comment is confusing. I think this is testing that we don't get messages when the API is disabled?
Attachment #8933244 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review213112

> Is this the right way to fix this?

Yes, although I've just realized that people could still themselves emit the custom event that you're relying on. I don't see a way of fixing that, though - and presumably because this is whitelisted to pages that we kind of trust, and because getting it wrong isn't really that bad, this is OK?

> This makes sense. However, I'd like to be able to switch hybrid content telemetry on without restarting Firefox. Let's check for the pref in the .jsm file, without unloading it when it's flipped off. Does it make sense to you? It could also be reasonable to still check the pref in the content script as well: this way we can prevent messages from going to the parent in the context of a non compromised content process.

A compromised parent process is worse than a compromised content one, and could just spawn its own compromised content processes.

As for unloading the scripts, presumably you could just re-load the content process script from the parent process when the pref is flipped?

> I'm not entirely sure what's the best place to remove this listener. Suggestions? As far as I can tell, UITour doesn't seem to remove the similar listener.

OK, this is unfortunate, but let's leave that then.
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review213964

Fine by me with :Gijs' comments addressed.
Attachment #8933244 - Flags: review?(chutten) → review+
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review213112

> Yes, although I've just realized that people could still themselves emit the custom event that you're relying on. I don't see a way of fixing that, though - and presumably because this is whitelisted to pages that we kind of trust, and because getting it wrong isn't really that bad, this is OK?

Yeah, I think we should be ok now.

> A compromised parent process is worse than a compromised content one, and could just spawn its own compromised content processes.
> 
> As for unloading the scripts, presumably you could just re-load the content process script from the parent process when the pref is flipped?

Do you have any suggestion on where to find an example for this in our codebase?
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review213926

> This doesn't actually test what it says it tests. `content` is always the top frame, I believe. You would need to pass the event and check it's come from the right document.
> 
> Sorry for noticing this before. If you copied this from elsewhere, or know of other code that does the check this way, please file separate follow-up bugs.

No problem, thanks for catching this. I saw this line in two locations: [here](https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/browser/components/uitour/content-UITour.js#62) and [here](https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/ipc/manifestMessages.js#80). During the security review, this was requested as well. However, I see what you mean: the [docs](https://searchfox.org/mozilla-central/rev/f0ab0b072ae090f11d6b02b7cf1e0829b4f08882/dom/base/nsIMessageManager.idl#400,402) mention that `content` is "The current top level window in the frame or null.".

Discussing this with :freddyb (#contentsecurity)/:pauljt, while you're right that if we're sure to always respond to the right windows that check isn't useful, this statement might still be useful:

> freddyb:  ...if you're sure you always respond to the right windows, it's not a big deal. but that's harder to check for a review at a later point in time. this if-statement helps making assumptions and reasoning about scenarios in which messages go around (i.e., excluding any kind of frame scenario). to summarize: I find the statement useful

What do you mean by "come from the right document"? Should I check the URI? And, if so, shouldn't this already be covered by the permission part below?
ni?Gijs - ensuring this is on your radar :)
Flags: needinfo?(gijskruitbosch+bugs)
I think we should definitely check that events come from the toplevel frame.

But `content` is always the toplevel frame.

In the current implementation, if the toplevel frame is system-privileged `about:foo` and it has a subframe to `evil.com`, `evil.com` could send an event that the current code will catch and then consider OK because the toplevel frame is OK.

Instead, we should pass `event` to isTrustedOrigin, and use `event.ownerGlobal` to figure out which window the event came from.
Flags: needinfo?(gijskruitbosch+bugs)
Or maybe `event.view`, egh, sorry I should be cooking not trying to write code in bug comments.
Comment on attachment 8933245 [details]
Bug 1417473 - Add the docs for hybrid content telemetry.

https://reviewboard.mozilla.org/r/204190/#review214822

This looks good high-level!
Most of the below are nits or improvement suggestions.
My main question is how this affects our go-faster initiative.

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:5
(Diff revision 8)
> +Hybrid content is web content that is designed to follow the look and feel of the
> +Firefox chrome. This can be either a page that ships with Firefox or that can be

I think our definition is not necessarily about the look at feel. How about something like:
"Hybrid content is web content that is loaded as part of Firefox, appears as part of Firefox to the user and is primarily intended to be used in Firefox."

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:7
(Diff revision 8)
> +loaded dynamically from our hosted services. Hybrid content telemetry allows these
> +pages to submit Telemetry data.

Let's make clear that this is limited to speficic Mozilla properties:
"Hybrid content Telemetry allows Mozilla pages to ..."

And make clear that there are two goals here:
"... to check the whether data collection is enabled and to submit Telemetry data."

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:32
(Diff revision 8)
> +
> +  Mozilla.ContentTelemetry.canUpload();
> +  Mozilla.ContentTelemetry.registerEvents(category, eventData);
> +  Mozilla.ContentTelemetry.recordEvent(category, method, object, value, extra);
> +
> +These functions will not throw. If an unsupported operation is performed (e.g. writing to an unkown event) an error will be logged to the browser console if the Telemetry logging :doc:`preferences <../internals/preferences>` are set.

Nit: "recording an unknown event"
Nit: Do we need to mention the prefs here? Error logging is on by default.

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:36
(Diff revision 8)
> +
> +These functions will not throw. If an unsupported operation is performed (e.g. writing to an unkown event) an error will be logged to the browser console if the Telemetry logging :doc:`preferences <../internals/preferences>` are set.
> +
> +.. note::
> +
> +    Data collected using this API will always respect the user Telemetry preferences: if a user has chosen to not send Telemetry data to Mozilla servers, Telemetry from hybrid content pages will not be sent as well.

Nit: "not be sent either"

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:50
(Diff revision 8)
> +
> +This function returns true if the browser is allowed to send collected data to Mozilla servers (i.e. ``datareporting.healthreport.uploadEnabled`` is ``true``), false otherwise. See :doc:`preferences <../internals/preferences>`.
> +
> +.. note::
> +
> +    The page should use this function to check if it is allowed to collect data. This is only needed in case the Telemetry system is not be being used for collection. If Telemetry is used, then this is taken care of internally by the Telemetry API. The page should not cache the returned value: user can opt in or out from the Data Collection at any time and so the returned value may change.

Nit: "userS can ..."?

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:71
(Diff revision 8)
> +
> +Register new dynamic events from the content. This accepts the same parameters and is subject to the same limitation as ``Services.telemetry.registerEvents()``. See the `events` documentation for the definitive reference.
> +
> +.. note::
> +
> +    While is possible to call this function anywhere in content and at any time, doing that as soon as the library is loaded (e.g. `window load event <https://developer.mozilla.org/en-US/docs/Web/Events/load>`_) will make sure that the definition will be ready when recording.

Nit: "Make sure to call this before ..." is the main point here?

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:110
(Diff revision 8)
> +Adding content data collection
> +==============================

This section is nice and accessible.
Let's have this section first, before the API reference.
Also add a section on "checking upload is enabled".

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:121
(Diff revision 8)
> +3. registering the probes after the library is loaded;
> +4. using the API to send Telemetry.
> +
> +Granting the privileges
> +-----------------------
> +For security/privacy reasons `Mozilla.ContentTelemetry` will only work on a list of allowed secure origins. The list of allowed origins can be found in `browser/app/permissions <https://dxr.mozilla.org/mozilla-central/source/browser/app/permissions>`_ . A host needs to be given the ``hc_telemetry`` permission in order to be whitelisted.

Does this require riding the trains?
Is there any way for new go-faster content to ship new collections?
Attachment #8933245 - Flags: review?(gfritzsche)
Attachment #8933245 - Attachment is obsolete: true
Comment on attachment 8933244 [details]
Bug 1417473 - Implement the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/204188/#review214872

::: toolkit/components/telemetry/hybrid-content/content-HybridContentTelemetry.js:39
(Diff revisions 5 - 6)
>    },
>  
>    /**
>     * Verifies that the hybrid content telemetry request comes
>     * from a trusted origin.
> +   * @oaram {nsIDomEvent} event Optional object containing the data for the event coming from

Heh, 'oaram' - I think you want `@param`
Comment on attachment 8938441 [details]
Bug 1417473 - Add the docs for hybrid content telemetry.

https://reviewboard.mozilla.org/r/209150/#review215548

LGTM
Attachment #8938441 - Flags: review?(chutten) → review+
Comment on attachment 8938441 [details]
Bug 1417473 - Add the docs for hybrid content telemetry.

https://reviewboard.mozilla.org/r/209150/#review215748

From a high-level look, this looks good to me.
I like the extensive guide section and going through the various edge cases and concerns.

I just have a few small things i commented on.

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:13
(Diff revision 3)
> +services. Hybrid content telemetry allows Mozilla pages to check whether data
> +collection is enabled and to submit Telemetry data.
> +
> +.. important::
> +
> +    Every new data collection in Firefox (including hybrid content) needs a `data collection review <https://wiki.mozilla.org/Firefox/Data_Collection#Requesting_Approval>`_ from a data collection peer. Just set the feedback? flag for one of the data peers. We try to reply within a business day.

"We try to reply" -> "They try to reply" or similar.

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:56
(Diff revision 3)
> +    Services.perms.remove(hostURI, "hc_telemetry");
> +  }
> +
> +.. important::
> +
> +    Granted permissions do not disapear when a "go-faster" add-on is uninstalled. They need to be cleaned up manually. Moreover, permissions are keyed by origin: ``http://mozilla.com`` and ``https://mozilla.com`` are different things.

They do not stay active after Firefox was closed though, right?

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:138
(Diff revision 3)
> +        </button>
> +      </div>
> +    </body>
> +  </html>
> +
> +Checking upload is enabled

"... if upload is enabled"

::: toolkit/components/telemetry/docs/collection/hybrid-content.rst:140
(Diff revision 3)
> +    </body>
> +  </html>
> +
> +Checking upload is enabled
> +--------------------------
> +Mozilla pages can check if data upload is enabled, as reported by Telemetry prefs :doc:`preferences <../internals/preferences>`. This is useful for pages which are not using Telemetry to collect data, but

I think this renders as "Telemetry prefs preferences".
One "pref" or "preferences" should be enough.
Attachment #8938441 - Flags: review?(gfritzsche) → review+
Attachment #8939762 - Flags: review?(chutten)
Comment on attachment 8939762 [details]
Bug 1417473 - Enable the hybrid content telemetry API.

https://reviewboard.mozilla.org/r/210066/#review215926
Attachment #8939762 - Flags: review?(chutten) → review+
Attachment #8939762 - Flags: review?(gfritzsche)
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/443747e39abe
Implement the hybrid content telemetry API. r=chutten,Gijs
https://hg.mozilla.org/integration/autoland/rev/a365a5a59ad1
Add the docs for hybrid content telemetry. r=chutten,gfritzsche
https://hg.mozilla.org/integration/autoland/rev/27db7bf978be
Enable the hybrid content telemetry API. r=chutten
(In reply to Natalia Csoregi [:nataliaCs] from comment #53)
> Backed out for failing browser_HybridContentTelemetry.js.

Thanks Natalia!
Flags: needinfo?(alessio.placitelli)
hg error in cmd: hg rebase -s 961519f2e94a -d f8a1aafc3703: abort: can't rebase public changeset 961519f2e94a
(see 'hg help phases' for details)
Pushed by alessio.placitelli@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/2445ab084f3d
Implement the hybrid content telemetry API. r=chutten,Gijs
https://hg.mozilla.org/integration/autoland/rev/50d298fe8fb6
Add the docs for hybrid content telemetry. r=chutten,gfritzsche
https://hg.mozilla.org/integration/autoland/rev/e8c01aa93781
Enable the hybrid content telemetry API. r=chutten
(In reply to Alessio Placitelli [:Dexter] from comment #0)
> This bug is about implementing the HCT API as described in the design
> document that lives here:
> https://docs.google.com/document/d/1yaTNs-OuttHWuHSTM4nH9n1HOmqSiBtEQkrge-
> SvACo/edit#heading=h.l7t1rh78yzzj

Probably my fault, is this document supposed to be readable
by those without account at the used hosting provider?

I'm only seeing a phone number harvesting login page when following the link.
(In reply to pubkeypin from comment #64)
> (In reply to Alessio Placitelli [:Dexter] from comment #0)
> > This bug is about implementing the HCT API as described in the design
> > document that lives here:
> > https://docs.google.com/document/d/1yaTNs-OuttHWuHSTM4nH9n1HOmqSiBtEQkrge-
> > SvACo/edit#heading=h.l7t1rh78yzzj
> 
> Probably my fault, is this document supposed to be readable
> by those without account at the used hosting provider?
> 
> I'm only seeing a phone number harvesting login page when following the link.

The documentation is now available to everyone along with the rest of the Telemetry docs here: https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/telemetry/collection/hybrid-content.html
(In reply to Alessio Placitelli [:Dexter] from comment #65)
[...]
> The documentation is now available to everyone along with the rest of the
> Telemetry docs here:
> https://firefox-source-docs.mozilla.org/toolkit/components/telemetry/
> telemetry/collection/hybrid-content.html

Thank you Alessio!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: