1436845 - Cert annotation performance and reliability improvements

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement, P1)

Description

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

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.

Comment 2

[(No longer employed by Mozilla) Aaron Klotz]

We should be able to obtain full paths during minidump generation, however this will require some breakpad changes.

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

(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.

Comment 4

[(No longer employed by Mozilla) Aaron Klotz]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e61d7d4d534b0f0f4ba2878bc948c2735bb217a4

Comment 5

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 1: Add BasicDllServices for non-XPCOM programs

Comment 6

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 2: Link minidump-anayzer with mozglue on Windows

Comment 7

[(No longer employed by Mozilla) Aaron Klotz]

Attachment: Part 3: Add ModuleSignatureInfo support to minidump-analyzer on Windows

(I would have asked gsvelto to review this patch but he's on PTO)

Comment 8

[(not currently active) Ted Mielczarek]

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.

Comment 9

[(not currently active) Ted Mielczarek]

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.

Comment 10

[(No longer employed by Mozilla) Aaron Klotz]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff7e21c630eb634a2d59910d8f9b95388fc35c0d
Bug 1436845: Part 1 - Add BasicDllServices; r=jimm

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9c97aa6d78903db203f80a1d6d78b1547f15297
Bug 1436845: Part 2 - Link minidump-analyzer with mozglue on Windows; r=ted

https://hg.mozilla.org/integration/mozilla-inbound/rev/341f20ef2627e0b19a324de14f9b32ed743bda92
Bug 1436845: Part 3 - Add support for ModuleSignatureInfo field in .extra file on Windows builds; r=ted

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/ff7e21c630eb
https://hg.mozilla.org/mozilla-central/rev/a9c97aa6d789
https://hg.mozilla.org/mozilla-central/rev/341f20ef2627

Comment 12

[(not currently active) Ted Mielczarek]

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.)