Cert annotation performance and reliability improvements

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: aklotz, Assigned: aklotz)

Tracking

unspecified
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox60 fixed)

Details

(Whiteboard: inj+)

Attachments

(3 attachments)

(Assignee)

Description

a year ago
In bug 1436684 I had to disable cert annotations due to perf regressions at startup. We're still going to want these annotations for telemetry, but I think in that case we'll use a pull model where the modules ping will request the cert information (since it is only sent weekly).

As for crash reports, I suspect that the perf problem is due to saturated cores during startup. Even though we're initializing off the main thread, there's just too much stuff going on.

We *could* delay cert annotations past startup, but that means that we don't have any coverage should a startup crash occur (which is a common case with injected DLLs). Even if we left this patch in as-is, there would still be a window where those annotations are missing (during initial data gathering).

I'm leaning toward moving the crash reporter bits into the crash reporter client, thus guaranteeing that we query all the modules in the minidump and completely eliminating the Firefox perf factor.
(Assignee)

Comment 1

a year ago
One problem is that we probably only have the leaf filenames in the pending minidump. If we want to do cert extraction, we need to find the full path to the binary.
(Assignee)

Comment 2

a year ago
We should be able to obtain full paths during minidump generation, however this will require some breakpad changes.
(Assignee)

Updated

a year ago
Depends on: 1437156
(Assignee)

Updated

a year ago
Priority: -- → P1
Whiteboard: inj+
(Assignee)

Comment 3

a year ago
(In reply to Aaron Klotz [:aklotz] from comment #1)
> One problem is that we probably only have the leaf filenames in the pending
> minidump. If we want to do cert extraction, we need to find the full path to
> the binary.

I take that back. Looks like we get full paths.
(Assignee)

Updated

a year ago
Blocks: 1434495
(Assignee)

Comment 7

a year ago
(I would have asked gsvelto to review this patch but he's on PTO)
Attachment #8951039 - Flags: review?(ted)
Attachment #8951032 - Flags: review?(jmathies) → review+
Comment on attachment 8951033 [details] [diff] [review]
Part 2: Link minidump-anayzer with mozglue on Windows

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

This seems fine. We primarily didn't use Gecko code in the crashreporter client because we didn't want something like a startup crash to make the crashreporter inoperable, but mozglue is fundamental enough that I don't think that worry applies.
Attachment #8951033 - Flags: review?(ted) → review+
Comment on attachment 8951039 [details] [diff] [review]
Part 3: Add ModuleSignatureInfo support to minidump-analyzer on Windows

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

It might not be bad at this point to have a short comment at the top of the file explaining what minidump-analyzer does, now that it's going to be doing additional work on Windows.

::: toolkit/crashreporter/minidump-analyzer/minidump-analyzer.cpp
@@ +223,5 @@
>      }
>  
> +#if defined(XP_WIN)
> +    auto certSubject = gDllServices.GetBinaryOrgName(
> +                         UTF8ToWide(module->code_file()).c_str());

I had a moment here before I remembered that module filenames are stored as UTF-16 in minidumps and Breakpad converts them back to UTF-8: https://chromium.googlesource.com/breakpad/breakpad/+/master/src/processor/minidump.cc#5562

@@ +228,5 @@
> +    if (certSubject) {
> +      string strSubject(WideToUTF8(certSubject.get()));
> +      Json::Value& subjectNode = aCertSubjects[strSubject];
> +      if (!subjectNode) {
> +        subjectNode = Json::Value(Json::arrayValue);

These two lines were really confusing to me until I read the jsoncpp docs and realized that:
a) Json::Value's `operator[]` will create and return a null member if the key doesn't exist
b) Json::Value's `operator!` returns `isNull`
c) This line calls `operator=` on the null object which resets its contents.

A short comment to clarify would be helpful here.

@@ +329,4 @@
>  }
>  
>  // Process the minidump file and append the JSON-formatted stack traces to
> +// the node specified in |aStackTraces|

Since you're updating the comment, can you also mention the new `aCertSubjects` parameter you're adding?

@@ +374,5 @@
>    return file;
>  }
>  
>  // Update the extra data file by adding the StackTraces field holding the
>  // JSON output of this program.

Please update this comment as well.

@@ +397,5 @@
> +    *f << "StackTraces=" << writer.write(aStackTraces);
> +
> +#if defined(XP_WIN)
> +    if (!!aCertSubjects) {
> +      *f << "ModuleSignatureInfo=" << writer.write(aCertSubjects);

At some point we should just change these .extra files to .json and stop goofing around with this stuff.
Attachment #8951039 - Flags: review?(ted) → review+

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ff7e21c630eb
https://hg.mozilla.org/mozilla-central/rev/a9c97aa6d789
https://hg.mozilla.org/mozilla-central/rev/341f20ef2627
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
While reviewing your other patch I found myself wondering if we ought to be including the serial numbers of the signing certs? That could make it easier to track down the origin of things if someone generates a signing certificate with a confusing subject name. (I'm imagining homoglyphs or simply confusingly similar subject names, here.)
You need to log in before you can comment on or make changes to this bug.