840476 - Gather telemetry about character encoding (charset) menu use on labeled vs. unlabeled pages

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Henri Sivonen (:hsivonen)]

In order to be able to make informed decisions about gradually retiring the character encoding menu, we should gather telemetry about its use.

To mitigate the risk of users being tricked into corrupting properly authored pages, it would be good to make the menu have no effect on pages that explicitly set their encoding. To determine if it's OK to make that change, we should categorize uses of the character encoding menu into two categories:
 1) Override was chosen when the top-level page was labeled.
 2) Override was chosen when the top-level page was unlabeled.

Comment 1

[Henri Sivonen (:hsivonen)]

Hmm. Actually, it would be possible to gather this info even for non-top-level pages in nsHtml5StreamParser and even for other same-origin pages loaded with the override in effect.

To measure only the user actions, the telemetry code should probably live in the docshell.

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Add the probe

(In reply to Henri Sivonen (:hsivonen) from comment #1)
> Hmm. Actually, it would be possible to gather this info even for
> non-top-level pages in nsHtml5StreamParser and even for other same-origin
> pages loaded with the override in effect.

Not sure what I was thinking when I wrote that.

This patch does what I suggested in comment 0.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 716474 [details] [diff] [review]
Add the probe

> NS_IMETHODIMP
> nsDocShell::SetCharset(const char* aCharset)
> {
>     // set the default charset
>     nsCOMPtr<nsIContentViewer> viewer;
>     GetContentViewer(getter_AddRefs(viewer));
>     if (viewer) {
>+
>+      nsIDocument* doc = viewer->GetDocument();
>+      MOZ_ASSERT(!doc->WillIgnoreCharsetOverride(), "How come we got here?");
>+      if (doc && !doc->WillIgnoreCharsetOverride()) {
Here you have null check for doc, but you don't have that in MOZ_ASSERT.



>+        int32_t charsetSource = doc->GetDocumentCharacterSetSource();
>+        switch (charsetSource) {
>+          case kCharsetFromWeakDocTypeDefault:
>+          case kCharsetFromUserDefault:
>+          case kCharsetFromDocTypeDefault:
>+          case kCharsetFromCache:
>+          case kCharsetFromParentFrame:
>+          case kCharsetFromAutoDetection:
>+          case kCharsetFromHintPrevDoc:
>+            // Changing charset on an unlabeled doc
>+            Telemetry::Accumulate(Telemetry::CHARSET_OVERRIDE_SITUATION, 0);
>+            break;
>+          case kCharsetFromMetaPrescan:
>+          case kCharsetFromMetaTag:
>+          case kCharsetFromChannel:
>+            // Changing charset on a doc that had a charset label
>+            Telemetry::Accumulate(Telemetry::CHARSET_OVERRIDE_SITUATION, 1);
>+            break;
>+          case kCharsetFromParentForced:
>+          case kCharsetFromUserForced:
>+            // Changing charset on a document that already had an override
>+            Telemetry::Accumulate(Telemetry::CHARSET_OVERRIDE_SITUATION, 2);
>+            break;
>+          case kCharsetFromIrreversibleAutoDetection:
>+          case kCharsetFromOtherComponent:
>+          case kCharsetFromByteOrderMark:
>+          case kCharsetUninitialized:
>+          default:
>+            MOZ_NOT_REACHED("Should not have come here.");
And this is really never ever possible? Since we crash here even on opt builds if this happens, I believe.
Perhaps you could use Telemetry::Accumulate(Telemetry::CHARSET_OVERRIDE_SITUATION, 3) for this. (and increase n for telemetry)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

In other words, I'd prefer to *not* use MOZ_NOT_REACHED in this kind of code which isn't critical or anything.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Add the probe, r=smaug

Not landing yet, since my understanding is that all telemetry probes need privacy review.

Comment 6

[Sid Stamm [:geekboy or :sstamm]]

Much like bug 843586, this is about measuring feature usage, so it should be in scope for telemetry privacy.  CC'ing curtisk since he's the one managing this review now.

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Better patch

Let's try this again. This patch works around the problem we discussed on IRC about extensions setting the charset property on the docshell. Also, this patch folds in the flag from the other bug. Additionally, this breaks the unlabeled cases into file URLs and non-file URLs in order to see if people are actually using the menu to work around problems with local files as opposed to working around problems with Web sites. Furthermore, this handles the cases where the heuristic detector has fired separately in order to see if our detectors are actually misfiring a lot.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 718955 [details] [diff] [review]
Better patch

Update uuid is of nsIDocShell

Comment 10

[Henri Sivonen (:hsivonen)]

Thanks. Landed with revised uuid.
https://hg.mozilla.org/integration/mozilla-inbound/rev/57179b1fea93

Edited the telemetry wiki accordingly:
https://wiki.mozilla.org/Privacy/Reviews/Telemetry/Measurements#Deployed_.28In_Nightly.2FAurora.2FBeta.2FRelease.29

Comment 11

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/57179b1fea93