Add module cert info to modules ping

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

(Blocks 1 bug)

unspecified
mozilla60
Unspecified
Windows
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: inj+)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
Once bug 1430857 lands, we can expand the CertAnnotator to report info to the modules ping in telemetry.

(See https://bugzilla.mozilla.org/show_bug.cgi?id=1430857#c3)
Priority: -- → P4
(Assignee)

Updated

a year ago
No longer blocks: injecteject
(Assignee)

Updated

a year ago
Blocks: 1435773
(Assignee)

Updated

a year ago
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Priority: P4 → P2
Whiteboard: inj+
Component: Telemetry → Crash Reporting
(Assignee)

Updated

a year ago
Priority: P2 → P1
(Assignee)

Comment 5

a year ago
(Note: data review for this functionality was done as part of bug 1430857)

This patch adds the certSubject field to the modules ping. To control ping size, we just omit the field when it isn't present (instead of setting to null), as it is common for a module to lack a signature.
Attachment #8956949 - Flags: review?(chutten)
Comment on attachment 8956949 [details] [diff] [review]
Add cert subject to modules ping when available

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

Two smallish comments, only.

::: toolkit/components/telemetry/tests/unit/test_TelemetryModules.js
@@ +70,5 @@
> +      name: "ntdll.dll",
> +      // This value changes depending on OS version and is irrelevant to this test
> +      debugName: false,
> +      // This value changes depending on OS version and is irrelevant to this test
> +      version: false,

Instead of using a boolean type check, could we omit them from the struct and check `if ("version" in lib.version)`. We could render them as commented-out here to show we're doing so deliberately. Then it would line up with how most test libs don't have `certSubject`

@@ +204,5 @@
>      } else {
>        Assert.greater(test_lib.debugID.length, 0, "The " + lib.name + " module has a debug ID.");
>      }
> +
> +    if (lib.certSubject !== undefined) {

It would be slightly more precise to check `if ("certSubject" in lib)` to catch cases where it is defined as undefined. (*sigh* JavaScript)
Attachment #8956949 - Flags: review?(chutten)
(Assignee)

Comment 7

a year ago
Changes made as suggested.
Attachment #8956949 - Attachment is obsolete: true
Attachment #8956959 - Flags: review?(chutten)
Comment on attachment 8956959 [details] [diff] [review]
Add cert subject to modules ping when available (r2)

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

LGTM
Attachment #8956959 - Flags: review?(chutten) → review+
(Assignee)

Comment 9

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/22ba4f32a7afe64038a38cd2e1fc16ede661f9ab
Bug 1434489: Add optional certSubject field to modules ping; r=chutten, data-review=francois (via bug 1430857)

Comment 10

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/22ba4f32a7af
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
(Assignee)

Updated

8 months ago
Component: Crash Reporting → Telemetry
You need to log in before you can comment on or make changes to this bug.