1002318 - Add UI Telemetry for sanitizing private data

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: uitelemetry-sanitize v0.1

Simple patch to add an Event for sanitizing private data.

Notes:
1. The event is fired after the sanitizing has completed. If we want to capture just opening the dialog, we can do that too, but is there value?
2. I added a "dialog" Method thinking that there might be other entry points for clearing data. Maybe through add-ons? I could be eaisly swayed to remove the "dialog" Method.

Are there other entry points we should care about?

Chenxia for the Event and Brian for the sanitize coverage.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: uitelemetry-sanitize-other v0.1

This code was not removed when it could have been in bug 771036

Comment 2

[Brian Nicholson (:bnicholson)]

Comment on attachment 8413523 [details] [diff] [review]
uitelemetry-sanitize v0.1

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

(In reply to Mark Finkle (:mfinkle) from comment #0)
> Created attachment 8413523 [details] [diff] [review]
> uitelemetry-sanitize v0.1
> 
> Simple patch to add an Event for sanitizing private data.
> 
> Notes:
> 1. The event is fired after the sanitizing has completed.

Note that it's fired after the the Java-side sanitization, but not necessarily before the Gecko sanitization. If you want to wait until everything is complete, the telemetry event should be fired here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/preferences/GeckoPreferences.java#304.

Not sure if there's much value capturing right when the dialog is opened, but there may be value in waiting until after we get Sanitize:Finished since we also have a success boolean we could pass along.

Comment 3

[Chenxia Liu [:liuche]]

Comment on attachment 8413523 [details] [diff] [review]
uitelemetry-sanitize v0.1

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

I'm kind of okay with having the sending of the Java message trigger the UI telemetry event, because technically, this is UI Telemetry, where we care about what the user is doing/tapping, but there's might be value in waiting for the Sanitize:Finished message from Gecko.

::: mobile/android/base/TelemetryContract.java
@@ +33,5 @@
>          // Sharing content.
>          public static final String SHARE = "share.1";
> +
> +        // Sanitizing private data.
> +        public static final String SANITIZE = "sanitize.1";

My first impression is that sanitize is what we do when we remove user-specific data from logs, not clearing private data. But I guess "Sanitize" is what we use for the Gecko message, so that's probably fine.

@@ +48,5 @@
>          // Action triggered from a button.
>          public static final String BUTTON = "button";
> +
> +        // Action triggered from a dialog.
> +        public static final String DIALOG = "dialog";

We could also reuse "dialog" when we launch contextmenus too.

Comment 4

[Brian Nicholson (:bnicholson)]

(In reply to Brian Nicholson (:bnicholson) from comment #2)
> Note that it's fired after the the Java-side sanitization

Actually, the Java-side sanitization is triggered by Gecko [1], so the Java-side sanitization may not have happened yet either. But if all we want to track is whether the user clicked the button without caring about the sanitization itself, it doesn't really matter.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/Sanitizer.jsm#141

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/6fcc6dae5409
remote:   https://hg.mozilla.org/integration/fx-team/rev/ce243803cab7

Chenxia's point about UI Telemetry being used to watch what the user taps, not what functionality really does, hit home to to me. I moved the Event to occur before any sanitizing actually completed, to the place where we know the dialog had a "positive" button press. So as soon as the user decided to start the sanitization.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6fcc6dae5409
https://hg.mozilla.org/mozilla-central/rev/ce243803cab7

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8413523 [details] [diff] [review]
uitelemetry-sanitize v0.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: We miss out on getting some "before changes" telemetry
Testing completed (on m-c, etc.): Working and being analyzed
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8413525 [details] [diff] [review]
uitelemetry-sanitize-other v0.1

See above

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/d2fd047a368b
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b3046dfbac4f

Comment 10

[Flaviu Cos]

The following telemetry probe is logged when clearing private data:
SendUIEvent: action = sanitize.1 method = dialog timestamp = 3040083 extras = null


Verified as fixed in builds:
- 32.0a1 (2014-05-25);
- 31.0a2 (2014-05-25);

Device: Alcatel One Touch 8008D (Android 4.1.2).