1744135 - Add font names to crash annotations

Status: RESOLVED FIXED

(Core :: Graphics: Text, task, P3)

Description

[Jim Mathies [:jimm]]

For diagnosing issues in BigSur and more generally.

Comment 1

[Brad Werth [:bradwerth]]

I'll take this on.

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1744135 Part 1: Add a 'FontName' key for crash reports.

Comment 3

[Brad Werth [:bradwerth]]

Attachment: Bug 1744135 Part 2: Annotate macOS font loading with font name if a crash happens.

Depends on D133425

Comment 4

[Jonathan Kew [:jfkthame]]

Just to note for the record: while in most cases exposing the name of the font being loaded will be harmless, it might occasionally reveal unexpected details. If it's something like a standard system font, or a widely-used Google Fonts resource, that probably reveals very little. But if it's a webfont used by one particular site for its branding, or a company's custom logo font, it may give a strong indication of the site the user was loading. (E.g. suppose it's a webfont called "PornHub Icons", used only by that site.) Or if it's a font that is associated with a particular language community (e.g. "Noto Sans Hanifi Rohingya"), it suggests the user is connected to or interested in that community, which in some situations might be politically sensitive.

Comment 5

[Brad Werth [:bradwerth]]

Attachment: Data Review Request

Comment 6

[Gabriele Svelto [:gsvelto]]

Comment on attachment 9255057 [details]
Data Review Request

Handing over the data review to a data steward. Ask my review on the patch again once the data review is in.

Comment 7

[Chris H-C :chutten]

Comment on attachment 9255057 [details]
Data Review Request

PRELIMINARY NOTE:
Please confirm that "Font name" is unlikely to be something that a user could reasonably be expected to set to something sensitive? For instance, this isn't a full file path, or the contents of a text area?

For CrashAnnotations we (temporarily) accept CrashAnnotations.yaml as a source of documentation for Crash Reports (since they are opt-in). But if this annotation is included in "crash" pings (which are opt-out), then please document the annotation in the "crash" ping docs.

DATA COLLECTION REVIEW RESPONSE:

Is there or will there be documentation that describes the schema for the ultimate data set available publicly, complete and accurate?

Yes.

Is there a control mechanism that allows the user to turn the data collection on and off?

Yes. This collection is Telemetry so can be controlled through Firefox's Preferences.

If the request is for permanent data collection, is there someone who will monitor the data over time?

Yes, Jonathan Kew is responsible.

Using the category system of data types on the Mozilla wiki, what collection type of data do the requested measurements fall under?

Category 1, Technical.

Is the data collection request for default-on or default-off?

Default off for all channels (Crash Reports). (( If this annotation is sent in "crash" pings as well, "default on for all channels" with an opt-out in the usual part of about:preferences#privacy ))

Does the instrumentation include the addition of any new identifiers?

No.

Is the data collection covered by the existing Firefox privacy notice?

Yes.

Does the data collection use a third-party collection tool?

No.

---

Result: datareview+

Comment 8

[Brad Werth [:bradwerth]]

(In reply to Chris H-C :chutten from comment #7)

Comment on attachment 9255057 [details]
Data Review Request

PRELIMINARY NOTE:
Please confirm that "Font name" is unlikely to be something that a user could reasonably be expected to set to something sensitive? For instance, this isn't a full file path, or the contents of a text area?

The FontName key will be filled with a font family name. No path, very similar to a file name minus the extension. It's like something you'd see in a Font menu in an app.

For CrashAnnotations we (temporarily) accept CrashAnnotations.yaml as a source of documentation for Crash Reports (since they are opt-in). But if this annotation is included in "crash" pings (which are opt-out), then please document the annotation in the "crash" ping docs.

I will add a Part 3 that does this, and mark you as the reviewer.

Comment 9

[Brad Werth [:bradwerth]]

Attachment: docs: Bug 1744135 Part 3: Update crash-ping documentation.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18fe281d3b21
Part 1: Add a 'FontName' key for crash reports. r=gfx-reviewers,gsvelto,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/f5c2b2e49d21
Part 2: Annotate macOS font loading with font name if a crash happens. r=gfx-reviewers,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/3ddaff848ded
docs: Bug 1744135 Part 3: Update crash-ping documentation. r=chutten

Comment 11

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/18fe281d3b21
https://hg.mozilla.org/mozilla-central/rev/f5c2b2e49d21
https://hg.mozilla.org/mozilla-central/rev/3ddaff848ded

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Here's a crash with the annotation:
https://crash-stats.mozilla.org/report/index/d491596a-e1cc-435b-b2e1-132b40211221#tab-annotations

Weirdly the FontName is "1"

Comment 13

[Jeff Muizelaar [:jrmuizel]]

I guess this is caused by psname implicitly converting to an int here:
https://searchfox.org/mozilla-central/rev/b605f01915c5704a55e9f485101b7be7d20a55df/gfx/thebes/gfxMacPlatformFontList.mm#397

Comment 14

[Gabriele Svelto [:gsvelto]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #13)

I guess this is caused by psname implicitly converting to an int here:
https://searchfox.org/mozilla-central/rev/b605f01915c5704a55e9f485101b7be7d20a55df/gfx/thebes/gfxMacPlatformFontList.mm#397

Yes, that has already happened before. Unfortunately I haven't found a way to avoid those implicit conversions; it's a bit of a footgun.

Comment 15

[Brad Werth [:bradwerth]]

I'll find a fix and get it posted.

Comment 16

[Jonathan Kew [:jfkthame]]

You probably just want to pass mName instead.

Comment 17

[Brad Werth [:bradwerth]]

Attachment: Bug 1744135 Part 4: Avoid implicit int conversion for font names.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ea19a575611
Part 4: Avoid implicit int conversion for font names. r=jfkthame

Comment 19

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/3ea19a575611