1348273 - Make AnnotateCrashReport() more robust by turning annotations into a well known list of constants

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[Gabriele Svelto [:gsvelto]]

I've been thinking of this for quite a while: crash annotations currently suffer from a number of issues. First of all they're just strings so we can add them anywhere in the code and there's no telling when a new one is added; we also have no protection against typos in the strings when moving them around or adding old ones in new places. Additionally, since there's no single place where we define them we have duplicate whitelists of what annotations can or cannot be sent in crash pings and finally we don't have a single place where they're documented so most of them are not.

One possible solution would be to move all the possible annotation definitions into nsICrashReporter.idl as constants. This way we would make them accessible from both C++ and JavaScript code without duplication, we'd be able to document them in one place and we could add tests to enforce that only known annotations are used in the code.

As a bonus we could make AnnotateCrashReport type-safe by adding a version that only deals with numbers so that all the consumers that are writing out numbers don't need to duplicate the number-to-string conversion logic.

Comment 1

[Benjamin Smedberg]

Highly related and on my wishlist: bug 1323002

Comment 2

[(not currently active) Ted Mielczarek]

I think this is the right way to go. We have a schema on the Socorro side ( https://github.com/mozilla/socorro/blob/master/socorro/schemas/crash_report.json ) but it's all just arbitrary text on the Firefox side.

Historically I think we just didn't know what we wanted when it all started, we could be better about this now. We can keep `appendAppNotesToCrashReport` for people to add arbitrary textual annotations.

Comment 3

[Gabriele Svelto [:gsvelto]]

(In reply to Benjamin Smedberg [:bsmedberg] from comment #1)
> Highly related and on my wishlist: bug 1323002

Yeah, that's what made me think about this.

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> Historically I think we just didn't know what we wanted when it all started,
> we could be better about this now. We can keep `appendAppNotesToCrashReport`
> for people to add arbitrary textual annotations.

I guess so, this is the kind of thing that grows over time and the early use cases are often a lot simpler than the latter uses. It's not a perfect solution but probably a good step towards it.

Comment 5

[Gabriele Svelto [:gsvelto]]

I've been working on this in the past two days by putting all the annotations in a .yaml file and generating a header from it with a strongly-typed enum, string names and so on. I've then changed AnnotateCrashReport() to use this enum instead of a generic string for the annotation key. However I've left AppendAppNotesToCrashReport() as is as we discussed previously for "free form" notes that need to be added.

That being said I was wondering what to do with CrashReporterHost::AddNote():

https://dxr.mozilla.org/mozilla-central/rev/cc903e3f61894e60c3b0efaf05e9a446d1d85888/ipc/glue/CrashReporterHost.h#142

On the one hand it's used for a few "free form" annotations which are a good fit for actual crash notes, like this one:

https://dxr.mozilla.org/mozilla-central/rev/cc903e3f61894e60c3b0efaf05e9a446d1d85888/dom/plugins/ipc/PluginModuleParent.cpp#871

On the other hand most of the other annotations we set using AddNote() are actual annotations with well known names which should IMHO be listed along the others and documented. So I'm tempted to add an "AddAnnotation()" method to the CrashReporterHost object to mirror AnnotateCrashReport() and convert most of the invocations to use it instead of AddNote().

Ted, what do you think?

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] WIP

This is a non-working WIP which puts annotations in a yaml file and then generate a header from it using a python script. AnnotateCrashReport() then uses a type-safe enum for the annotation instead of the strings. There's a bunch of things to iron out still, mostly related to the fact that annotations are now different than "free form" ones so they need to be handled separately. I've also converted and documented only a handful of annotations, there's plenty more to do.

Comment 7

[(not currently active) Ted Mielczarek]

I confess I didn't actually know about CrashReporterHost::AddNote at all. It sounds like someone just put it in as an expedient API to get annotations out of child processes? If so, I think splitting it into two APIs that mirror the in-process APIs makes sense.

Comment 8

[Gabriele Svelto [:gsvelto]]

I've been going through all our annotations to convert them and document them and I noticed that this one has a comment mentioning that it could go away once the adapter vendor ID, device ID and driver version string are recorded in Socorro:

https://dxr.mozilla.org/mozilla-central/rev/1867d7931c0a70ab90edf4aa84876525773a7139/widget/android/GfxInfo.cpp#361

Jeff, since you authored the patch, is this still needed? Socorro already records all the other annotations so I guess that one be safely removed.

Comment 9

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] WIP

This second WIP compiles correctly but is not fully working yet. All annotations in C++ code have been properly migrated but those in JS are still missing. I've modified the script that generates the annotation list to use alternate names when printing out annotations if the annotation name cannot be used directly as a C++ enum value.

Comment 10

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v3] WIP

First working patch; it still has a few rough edges but functionally speaking it's complete.

Comment 11

[Gabriele Svelto [:gsvelto]]

While working on this I've hit a very annoying issue: the crashreporter code is built conditionally but there's quite a few headers in our code that include nsExceptionHandler.h unconditionally. The reason why this doesn't normally break the build is that they usually have a plethora of #ifdef MOZ_CRASHREPORTER directives scattered around the code.

With my changes this doesn't work anymore because certain parts of those headers now require the crash annotation enum to be defined - which it isn't if the crashreporter hasn't been built.

Besides my specific problem here plenty of #ifdefs have never been a good idea in general so I'm tempted of sanitizing that stuff first. I'll open a separate bug for that though.

Comment 12

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v4] WIP

Rebased the patch and added a couple more annotations that were introduced in the meantime. This seems to be working well with crash reporting disabled too but still needs some polish and quite a bit of testing.

Comment 13

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v5] WIP

Updated patch, this has only one remaining test failing. However I will put this on hold until bug 1428775 is resolved. It would be worth re-writing this patch to put the annotations within the nsICrashReporter interface rather than spread out in two different files as we do now.

Comment 14

[Gabriele Svelto [:gsvelto]]

Clearing the NI for now.

Comment 15

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v6] WIP

More Windows-specific fixes.

Comment 16

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v7] WIP

This is a fully working patch, with all tests fixed though still dependent on bug 
1428775. I've split it up for ease of review but I can't post it to mozreview since the core changes would need to be reviewed by Ted and he's on PTO until the 3rd of April.

Comment 17

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Workaround for bug 1428775

This is a clunky workaround  for bug 1428775 that can be applied on top of attachment 8964019 [details] [diff] [review] to build. It works fine but it's an ugly contraption.

Comment 18

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Implementation of machine-generated crash annotations; .mielczarek

This patch introduces the machinery to generate crash annotations from a YAML
file. The relevant functions are updated to take a typed enum (in C++) and an
integer constant (in JavaScript). Once written out to the .extra file the
annotations are converted in string form and are no different than the
existing ones. The existing whitelists and blacklists of annotations are also
generated from the YAML file and the existing duplicate code has been
consolidated.

Review commit: https://reviewboard.mozilla.org/r/228644/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228644/

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the accessibility annotations;

Review commit: https://reviewboard.mozilla.org/r/228646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228646/

Comment 20

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the docshell and XPConnect-related annotations;

Review commit: https://reviewboard.mozilla.org/r/228648/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228648/

Comment 21

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the IPC-related annotations;

Review commit: https://reviewboard.mozilla.org/r/228650/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228650/

Comment 22

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the Android-specific annotations and use the machine-generated whitelist in the crash reporter;

Review commit: https://reviewboard.mozilla.org/r/228652/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228652/

Comment 23

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the graphics annotations;

This patch also removes some crash notes whose contents are already gathered
through annotations and surfaced via Socorro.

Review commit: https://reviewboard.mozilla.org/r/228654/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228654/

Comment 24

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the telemetry annotations;

Review commit: https://reviewboard.mozilla.org/r/228656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228656/

Comment 25

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the XPCOM annotations;

Review commit: https://reviewboard.mozilla.org/r/228658/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228658/

Comment 26

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the network-related annotations;

Review commit: https://reviewboard.mozilla.org/r/228660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228660/

Comment 27

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the media-related annotations;

Review commit: https://reviewboard.mozilla.org/r/228662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228662/

Comment 28

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the storage-related annotations;

Review commit: https://reviewboard.mozilla.org/r/228664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228664/

Comment 29

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the layout annotation;

Review commit: https://reviewboard.mozilla.org/r/228666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228666/

Comment 30

[Gabriele Svelto [:gsvelto]]

Attachment: Bug 1348273 - Convert the browser and toolkit annotations;

Review commit: https://reviewboard.mozilla.org/r/228668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/228668/

Comment 31

[Gabriele Svelto [:gsvelto]]

A few word of information to all reviewers. First of all sorry for the review-per-component request but I went through all our crash annotations and wrote descriptions for them, but since I'm not familiar with various parts of the code I'd like you to check those descriptions and amend them if I wrote something wrong (or too limited, imprecise, etc...). Additionally I removed some annotations that seemed obviously out of date, and trimmed others which were including redundant data; while I'm fairly sure about those an additional check doesn't hurt.

Last but not least because of bug 1428775 you cannot build this directly, you need to apply my workaround (attachment 8964020 [details] [diff] [review]) on top of all the other patches in order to build and run.

Comment 32

[Daniel Holbert [:dholbert]]

[I'm punting review for the "SheetLoadFailure" layout/style patch to heycam, since he's the one who added that annotation, so he's best able to offer feedback on the description. And I believe he's back & actively working on Mozilla stuff.]

Comment 33

[Daniel Holbert [:dholbert]]

[er never mind - MozReview says heycam's not accepting review requests, so I'll keep that one after all.]

Comment 34

[Code Review Bot [:reviewbot]]

Comment on attachment 8965069 [details]
Bug 1348273 - Convert the graphics annotations;

https://reviewboard.mozilla.org/r/228654/#review239438


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/thebes/gfxPlatform.cpp:219
(Diff revision 1)
>    uint32_t mMaxCapacity;
>    int32_t mIndex;
>    Mutex mMutex;
>  };
>  
> -CrashStatsLogForwarder::CrashStatsLogForwarder(const char* aKey)
> +CrashStatsLogForwarder::CrashStatsLogForwarder(CrashReporter::Annotation aKey)

Error: Bad implicit conversion constructor for 'crashstatslogforwarder' [clang-tidy: mozilla-implicit-constructor]

Comment 35

[Daniel Holbert [:dholbert]]

Comment on attachment 8965075 [details]
Bug 1348273 - Convert the layout annotation;

https://reviewboard.mozilla.org/r/228666/#review239434

::: toolkit/crashreporter/CrashAnnotations.yaml:518
(Diff revision 1)
> +    Set when failing to load a style sheet, this can contain a potentially very
> +    large amount of diagnostic information.

Two nits:
 (1) s/style sheet/built-in style sheet/
(clarifying which stylesheets can trigger this)

 (2) s/sheet, this/sheet. This/
(i.e. splitting run-on sentence into two sentences)

Comment 36

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8965068 [details]
Bug 1348273 - Convert the Android-specific annotations and use the machine-generated whitelist in the crash reporter;

https://reviewboard.mozilla.org/r/228652/#review239446

I think it's okay but :nalexander should review it.

::: mobile/android/base/Generate_CrashReporterConstants_java.py:1
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public

Lowercase file name

Comment 37

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8965068 [details]
Bug 1348273 - Convert the Android-specific annotations and use the machine-generated whitelist in the crash reporter;

https://reviewboard.mozilla.org/r/228652/#review239450

Comment 38

[Marco Bonardo [:mak]]

Comment on attachment 8965074 [details]
Bug 1348273 - Convert the storage-related annotations;

https://reviewboard.mozilla.org/r/228664/#review239454

Comment 39

[Nathan Froyd [:froydnj]]

Comment on attachment 8965071 [details]
Bug 1348273 - Convert the XPCOM annotations;

https://reviewboard.mozilla.org/r/228658/#review239456

This is great stuff, thank you.

Comment 40

[Patrick McManus [:mcmanus]]

Comment on attachment 8965072 [details]
Bug 1348273 - Convert the network-related annotations;

https://reviewboard.mozilla.org/r/228660/#review239466

Comment 41

[alexander :surkov (:asurkov)]

Comment on attachment 8965065 [details]
Bug 1348273 - Convert the accessibility annotations;

https://reviewboard.mozilla.org/r/228646/#review239468

::: toolkit/crashreporter/CrashAnnotations.yaml:22
(Diff revision 1)
> +    Set to "Active" by the accessibility service when it is active.
> +  type: string
> +
> +AccessibilityClient:
> +  description: >
> +    Set by the out-of-process accessibility client in case of failure.

it is collected when a11y is instantiated, not necessary in case of failures (see for example, https://dxr.mozilla.org/mozilla-central/source/accessible/windows/msaa/LazyInstantiator.cpp#256)

maybe simply: Accessibility client ID

::: toolkit/crashreporter/CrashAnnotations.yaml:27
(Diff revision 1)
> +    Set by the out-of-process accessibility client in case of failure.
> +  type: string
> +
> +AccessibilityInProcClient:
> +  description: >
> +    Hexadecimal mask of accessibility consumers, see

of in-process accessibility consumers

Comment 42

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 8965073 [details]
Bug 1348273 - Convert the media-related annotations;

https://reviewboard.mozilla.org/r/228662/#review239512

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8965066 [details]
Bug 1348273 - Convert the docshell and XPConnect-related annotations;

https://reviewboard.mozilla.org/r/228648/#review239536

Comment 44

[Nicholas Nethercote [inactive]]

Comment on attachment 8965076 [details]
Bug 1348273 - Convert the browser and toolkit annotations;

https://reviewboard.mozilla.org/r/228668/#review239578

Where do the descriptions in the YAML file come from -- are they pre-existing, or did you make them up?

::: toolkit/crashreporter/CrashAnnotations.yaml:229
(Diff revision 1)
>      CPU usage of the second Adobe Flash plugin process.
>    type: string
>  
> +CrashAddressLikelyWrong:
> +  description: >
> +    Set to 1 if signal handling is broken, then the crash address is likely to

Nit: s/then/in which case/

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2149
(Diff revision 1)
>          // Annotate the crash report with relevant add-on information.
>          try {
> -          Services.appinfo.annotateCrashReport("Theme", DEFAULT_SKIN);
> -        } catch (e) { }
> -        try {
> -          Services.appinfo.annotateCrashReport("EMCheckCompatibility",
> +          Services.appinfo.annotateCrashReport(
> +            Services.appinfo.Theme, DEFAULT_SKIN);
> +          Services.appinfo.annotateCrashReport(
> +            Services.appinfo.EMCheckCompatibility,

I guess it's unlikely that the second annotateCrashReport() could succeed if the first one doesn't?

Comment 45

[Alessio Placitelli [:Dexter]]

Comment on attachment 8965070 [details]
Bug 1348273 - Convert the telemetry annotations;

https://reviewboard.mozilla.org/r/228656/#review239588

::: toolkit/crashreporter/CrashAnnotations.yaml:507
(Diff revision 1)
> +    The telemetry environment in JSON format.
> +  type: string
> +
> +TelemetryServerURL:
> +  description: >
> +    Telemetry server URL, used to send telemetry pings.

nit: Can you mention that this is used by the pingsender? I think it's the only consumer, AFAIK

Comment 46

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8965069 [details]
Bug 1348273 - Convert the graphics annotations;

https://reviewboard.mozilla.org/r/228654/#review239742

Comment 47

[Gabriele Svelto [:gsvelto]]

(In reply to Alessio Placitelli [:Dexter] from comment #45)
> nit: Can you mention that this is used by the pingsender? I think it's the
> only consumer, AFAIK

Good point, it's actually the crash reporter client that uses it to generate the URL to send the ping to, not the pingsender itself. I'll add it. (IIRC we added it in bug 1341349)

Comment 48

[Gabriele Svelto [:gsvelto]]

(In reply to Nicholas Nethercote [:njn] from comment #44)
> Where do the descriptions in the YAML file come from -- are they
> pre-existing, or did you make them up?

I made them up, often reading the code to try and figure out what they meant. One of the reasons behind this is to have at least some documentation about the annotations and to ensure that new ones are also documented.

> ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2149
> (Diff revision 1)
> >          // Annotate the crash report with relevant add-on information.
> >          try {
> > -          Services.appinfo.annotateCrashReport("Theme", DEFAULT_SKIN);
> > -        } catch (e) { }
> > -        try {
> > -          Services.appinfo.annotateCrashReport("EMCheckCompatibility",
> > +          Services.appinfo.annotateCrashReport(
> > +            Services.appinfo.Theme, DEFAULT_SKIN);
> > +          Services.appinfo.annotateCrashReport(
> > +            Services.appinfo.EMCheckCompatibility,
> 
> I guess it's unlikely that the second annotateCrashReport() could succeed if
> the first one doesn't?

Actually there are two scenarios: the first one is that crash reporting has not been initialized in which case both will fail. The second one is if one of the annotations is unknown (which we can now detect) so if one removes the "Theme" annotation from the YAML file but not "EMCheckCompatibility" then the second annotation won't be added even though it should. I'll revert to the previous setup with separate try blocks.

Comment 49

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8965068 [details]
Bug 1348273 - Convert the Android-specific annotations and use the machine-generated whitelist in the crash reporter;

https://reviewboard.mozilla.org/r/228652/#review240340

I'm happy with this approach, and want to see this land smoothly.  I have many nits in the Python implementation, but these are largely just stylistic -- if you disagree, or are out of time/energy to work on this patch, we can push to follow-up.  (Perhaps even a good-first-bug for a contributor with some Python experience.)

I do have a question that either you or one of jchen or snorp might need to answer.  The way you've written this, the `CrashReporterConstants` class is exposed only to Fennec, not to GeckoView.  That's largely because a ticket tracking migrating _all_ of these constant-generation things to the new Gradle world is outstanding (https://bugzilla.mozilla.org/show_bug.cgi?id=1443202).  There's no way you could have anticipated that ticket, nor should you need to do anything in response, but we should understand whether or not these constants should be present/used in GeckoView, and adjust if that's the case.  If that _is_ the case, we should expose the constants in the GeckoView `BuildConfig` class in exactly the same manner, but we should do it in the existing Gradle/Groovy code.

I'm marking this r- only so that I get a chance to see it again and consider the responses.  Thanks for doing this large unification!

::: mobile/android/base/CrashReporterConstants.java.in:1
(Diff revision 1)
> +/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-

This file isn't preprocessed with the preprocessor, so the `.in` suffix is a little mis-leading.  I'd prefer to fold this into the Python script directly.

::: mobile/android/base/Generate_CrashReporterConstants_java.py:8
(Diff revision 1)
> +# You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +import string
> +import yaml
> +
> +class ParserError(Exception):

I personally find that wrapping Python exceptions in this way isn't very high value, since the first step when debugging is to manually unwrap the exception.  And in this case you're not preserving the actual exception, you're just extracting the message.

Did you need this wrapping in your local development, so my preference does not respect something you witnessed while getting this wired up?

If not, let's just drop the wrapping, and trust that "raw" errors will include enough details about the filename and the process that failed for debugging.

::: mobile/android/base/Generate_CrashReporterConstants_java.py:37
(Diff revision 1)
> +
> +    return whitelist
> +
> +
> +def generate_array_initializer(contents):
> +    """ generates the initializer for an array of strings """

nit: Through-out, no whitespace and capitalized full sentences for all docstrings.  So """Generates ... strings."""

An example, like "Turns `["a", "b"]` into '   \"a\"\n   '" is more useful for such things.

::: mobile/android/base/Generate_CrashReporterConstants_java.py:65
(Diff revision 1)
> +
> +def read_template(class_filename):
> +    """ Read the contents of the class template.
> +    If an error is encountered quit the program. """
> +
> +    try:

Again, if you don't worry so much about exception wrapping, this can just be `open(class_filename, "r").read()` -- Python will close and the file as it goes out of scope.  And you can avoid this file IO by folding your template into the Python string.

::: mobile/android/base/Generate_CrashReporterConstants_java.py:112
(Diff revision 1)
> +    parser.add_argument('annotations_filename',
> +                        help='YAML file holding the list of crash annotations')
> +    args = parser.parse_args()
> +    generate(sys.stdout, args.class_filename, args.annotations_filename)
> +
> +if __name__ == '__main__':

uber-nit: double line in between -- you might want to run flake8 on this file too.

::: mobile/android/base/moz.build:138
(Diff revision 1)
>  z.inputs += ['AndroidManifest.xml.in']
>  
> +# Generate CrashReporterConstants.java
> +crash_reporter_constants_files = [
> +    'CrashReporterConstants.java.in',
> +    '../../../toolkit/crashreporter/CrashAnnotations.yaml',

Is there an advantage to relative paths?  I'd prefer `TOPSRCDIR + /...` if not.

::: toolkit/crashreporter/CrashAnnotations.yaml:311
(Diff revision 1)
>    ping: true
>  
> +
> +JavaStackTrace:
> +  description: >
> +    Java stack trace, only present on Firefox for Android if we encounter an

Can we say "... present on Android ..."?  I expect we'll want to include this annotation for crashes that are handled by GeckoView, which will run in more Apps than just Firefox for Android.  That is, we might have a JavaStackTrace for, say, Focus/Klar-with-GeckoView.

Comment 50

[Nicholas Nethercote [inactive]]

> I made them up, often reading the code to try and figure out what they
> meant. One of the reasons behind this is to have at least some documentation
> about the annotations and to ensure that new ones are also documented.

Ok. I think Socorro also has definitions for some of these annotations, but not all of them. You see them on crash-stats if you hover the cursor over a label in a crash report. I can't see an obvious way to avoid any doubling up of the descriptions, though.

Comment 51

[Will Kahn-Greene [:willkg] ET needinfo? me]

Socorro has a "super_search_fields" schema that includes descriptions of fields some of which are annotations. I'm game for doing a manual pass updating Socorro descriptions and/or mulling over simple ways to update it more automatically over time.

Comment 52

[Jim Mathies [:jimm]]

Comment on attachment 8965067 [details]
Bug 1348273 - Convert the IPC-related annotations;

https://reviewboard.mozilla.org/r/228650/#review240652

lgtm, awesome cleanup work, thanks.

Comment 53

[(not currently active) Ted Mielczarek]

Comment on attachment 8965064 [details]
Bug 1348273 - Implementation of machine-generated crash annotations; .mielczarek

https://reviewboard.mozilla.org/r/228644/#review240318

Overall this looks very good! I have mostly nitpicky comments and a few questions. I'm only giving you r- because I'd like to see the two Python scripts combined into one. Otherwise this is a very nice change!

Additionally, as I mentioned in one of my comments, I think this could use a quick once-over from someone who's more familiar with modern C++, since I am definitely not.

::: ipc/glue/CrashReporterMetadataShmem.cpp:15
(Diff revision 1)
>  #include "nsISupportsImpl.h"
>  
>  namespace mozilla {
>  namespace ipc {
>  
> +using namespace CrashReporter;

Do you need the whole namespace here or could you just do `using CrashReporter::Annotation;`?

::: ipc/glue/CrashReporterMetadataShmem.cpp:134
(Diff revision 1)
>  {
>    MetadataShmemWriter writer(mShmem);
>  
> -  for (auto it = mNotes.Iter(); !it.Done(); it.Next()) {
> -    nsCString key = nsCString(it.Key());
> -    nsCString value = nsCString(it.Data());
> +  for (auto key : MakeEnumeratedRange(Annotation::Count)) {
> +    if (!mAnnotations[key].IsEmpty()) {
> +      if (!writer.WriteAnnotation(key, mAnnotations[key])) {

I confess to not being very up-to-speed with modern C++, especially as it's being used in Gecko. Could you get someone who is to give a once-over of the specifics of the typed enum?

::: toolkit/components/crashes/CrashManager.jsm:607
(Diff revision 1)
> +    let crashReporter = Cc["@mozilla.org/toolkit/crash-reporter;1"]
> +                          .getService(Ci.nsICrashReporter);
>  
>      for (let line in annotations) {
> -      if (this.ANNOTATION_WHITELIST.includes(line)) {
> +      try {
> +        if (crashReporter.isAnnotationWhitelistedForPing(line)) {

Nice!

::: toolkit/crashreporter/CrashAnnotations.yaml:47
(Diff revision 1)
> +  type: string
> +  ping: true
> +
> +BlocklistInitFailed:
> +  description: >
> +    Set to 1 if the DLL blocklist could not be initialized.

Kinda nitpicky, but this says `1` and the type is `bool`. Maybe mention at the top that `bool` values are stringified as 0 / 1 in the actual annotations?

::: toolkit/crashreporter/CrashAnnotations.yaml:128
(Diff revision 1)
> +  description: >
> +    Time in seconds since the last crash occurred.
> +  type: string
> +  ping: true
> +
> +ServerURL:

As a followup, maybe we should fix things so this isn't an annotation? It's just a weird implementation detail. We could pass it to the client in an environment variable, or just hardcode the default in the source.

::: toolkit/crashreporter/CrashAnnotations.yaml:204
(Diff revision 1)
> +  type: string
> +  ping: true
> +
> +UptimeTS:
> +  description: >
> +    Uptime in seconds.

You might mention that this has a fractional component (which is probably why it's a string and not an int).

::: toolkit/crashreporter/Generate_CrashAnnotations.py:31
(Diff revision 1)
> +
> +    whitelist = []
> +
> +    for (name, data) in annotations.iteritems():
> +        if data.get('ping', False):
> +            whitelist.append(name)

I probably would write this and the other ones as simply `return [name for (name, data) in annotations.iteritems() if data.get('ping', False)]`

...but that's just stylistic preference.

::: toolkit/crashreporter/Generate_CrashAnnotations.py:81
(Diff revision 1)
> +    """ generates the initializer for a C++ array of annotations """
> +
> +    initializer = ""
> +
> +    for name in contents:
> +        initializer += "  Annotation::" + name + ",\n"

You could use `join` to get the behavior you want here (commas between each entry), like: `',\n'.join(' Annotation::' + name for name in contents)`

::: toolkit/crashreporter/Generate_CrashAnnotations.py:107
(Diff revision 1)
> +    If an error is encountered quit the program. """
> +
> +    try:
> +        template_file = open(template_filename, "r")
> +        template = template_file.read()
> +        template_file.close()

It's more Pythonic to do:
```with open(template_filename, 'r') as template_file:
    ...
```

(the with handles closing the file for you)

::: toolkit/crashreporter/Generate_CrashAnnotations.py:123
(Diff revision 1)
> +    annotations and return it as a string """
> +
> +    whitelist = extract_crash_ping_whitelist(annotations)
> +    blacklist = extract_content_process_blacklist(annotations)
> +
> +    return "/* THIS IS AN AUTOGENERATED FILE.  DO NOT EDIT */\n\n" + \

It'd probably be nice to include the name of the script in the warning comment.

::: toolkit/crashreporter/moz.build:12
(Diff revision 1)
>  SPHINX_TREES['crashreporter'] = 'docs'
>  
>  with Files('docs/**'):
>      SCHEDULES.exclusive = ['docs']
>  
> +GENERATED_FILES = [

We generally use `+=` for all assignments in moz.build just so people don't accidentally break things when adding new entries.

::: toolkit/crashreporter/moz.build:133
(Diff revision 1)
>      UNIFIED_SOURCES += [
>          'nsDummyExceptionHandler.cpp',
>      ]
>  
> +# Generate CrashAnnotations.h
> +crash_annotations_files = [

If you don't need the list of files elsewhere I wouldn't bother using an intermediate variable for it.

::: toolkit/crashreporter/nsExceptionHandler.cpp:1967
(Diff revision 1)
>    delete gExceptionHandler;
>  
>    // do this here in the unlikely case that we succeeded in allocating
>    // our strings but failed to allocate gExceptionHandler.
> -  delete crashReporterAPIData_Hash;
> -  crashReporterAPIData_Hash = nullptr;
> +  for (auto key : MakeEnumeratedRange(Annotation::Count)) {
> +    crashReporterAPIData_Table[key] = EmptyCString();

Is this necessary to avoid leaking the string data? I guess EnumeratedArray doesn't have a clear method because it always has as many entries as enum variants?

::: toolkit/crashreporter/nsExceptionHandler.cpp:2166
(Diff revision 1)
>  
>    MutexAutoLock lock(*crashReporterAPILock);
>  
> -  crashReporterAPIData_Hash->Put(key, escapedData);
> +  crashReporterAPIData_Table[key] = escapedData;
>  
>    // now rebuild the file contents

If this enumeration code doesn't allocate we could remove this and write this out in the minidump callback instead. Do you know if that's the case?

::: toolkit/crashreporter/test/unit/test_crash_after_js_large_allocation_failure.js:10
(Diff revision 1)
>    }
>  
>    do_crash(
>      function() {
>        crashType = CrashTestUtils.CRASH_MOZ_CRASH;
> -      crashReporter.annotateCrashReport("TestingOOMCrash", "Yes");
> +      crashReporter.annotateCrashReport(crashReporter.TestingOOMCrash, "Yes");

It'd probably be fine to just use `TestKey` for all these tests as well. I don't think the annotation name has any special meaning.

::: xpcom/system/Generate_nsICrashReporterIDL.py:3
(Diff revision 1)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +# You can obtain one at http://mozilla.org/MPL/2.0/.

You should combine this script with the other script since they share so much code. You can specify which function to use in a `GENERATED_FILES` script specifically for this purpose:
https://firefox-source-docs.mozilla.org/build/buildsystem/mozbuild-symbols.html#generated-files

Comment 54

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965068 [details]
Bug 1348273 - Convert the Android-specific annotations and use the machine-generated whitelist in the crash reporter;

https://reviewboard.mozilla.org/r/228652/#review240978

Comment 55

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965068 [details]
Bug 1348273 - Convert the Android-specific annotations and use the machine-generated whitelist in the crash reporter;

https://reviewboard.mozilla.org/r/228652/#review240340

I'm glad to fix the python bits, this is probably the largest piece of Python code I wrote so it's quite rough around the edges. As for inclusion in GeckoView I see that bug 1433968 doesn't have a definitive answer yet so I'll leave it up to you. If this needs to be adapted though I'll be glad to do it.

> This file isn't preprocessed with the preprocessor, so the `.in` suffix is a little mis-leading.  I'd prefer to fold this into the Python script directly.

Sure, will do.

> I personally find that wrapping Python exceptions in this way isn't very high value, since the first step when debugging is to manually unwrap the exception.  And in this case you're not preserving the actual exception, you're just extracting the message.
> 
> Did you need this wrapping in your local development, so my preference does not respect something you witnessed while getting this wired up?
> 
> If not, let's just drop the wrapping, and trust that "raw" errors will include enough details about the filename and the process that failed for debugging.

I'll just drop that exception and use raw ones. When I started working on this patch many moons ago I just copy pasted that stuff from https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/toolkit/components/telemetry/shared_telemetry_utils.py#178. It's  just a leftover from that.

> Again, if you don't worry so much about exception wrapping, this can just be `open(class_filename, "r").read()` -- Python will close and the file as it goes out of scope.  And you can avoid this file IO by folding your template into the Python string.

I folded the file into the script, it's easier that way.

> Is there an advantage to relative paths?  I'd prefer `TOPSRCDIR + /...` if not.

No advantage, it's ugly as sin in fact, I'll use `TOPSRCDIR + /...` instead.

> Can we say "... present on Android ..."?  I expect we'll want to include this annotation for crashes that are handled by GeckoView, which will run in more Apps than just Firefox for Android.  That is, we might have a JavaStackTrace for, say, Focus/Klar-with-GeckoView.

Sure.

Comment 57

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965064 [details]
Bug 1348273 - Implementation of machine-generated crash annotations; .mielczarek

https://reviewboard.mozilla.org/r/228644/#review240318

Any suggestions for who might look into the C++ stuff? I just tried to stick to patterns that are used elsewhere in the code but I'm no C++ expert so it could use another look.

> Do you need the whole namespace here or could you just do `using CrashReporter::Annotation;`?

CrashReporter::Annotation is enough, I think I left it there accidentally.

> I confess to not being very up-to-speed with modern C++, especially as it's being used in Gecko. Could you get someone who is to give a once-over of the specifics of the typed enum?

Sure! Specifically MakeEnumeratedRange() is just a helper we have to iterate over the elements of an enum, see https://searchfox.org/mozilla-central/rev/6bfadf95b4a6aaa8bb3b2a166d6c3545983e179a/mfbt/EnumeratedRange.h#189

> As a followup, maybe we should fix things so this isn't an annotation? It's just a weird implementation detail. We could pass it to the client in an environment variable, or just hardcode the default in the source.

Asbolutely, I'll open a bug for that.

> You might mention that this has a fractional component (which is probably why it's a string and not an int).

Will do.

> I probably would write this and the other ones as simply `return [name for (name, data) in annotations.iteritems() if data.get('ping', False)]`
> 
> ...but that's just stylistic preference.

My knowledge of Python is very limited, so I went for the simplest syntax I could muster but I'm fine with your more idiomatic form. I just wasn't aware it was possible to write it like that.

> You could use `join` to get the behavior you want here (commas between each entry), like: `',\n'.join(' Annotation::' + name for name in contents)`

Thanks for the tip, it's much simpler this way.

> It's more Pythonic to do:
> ```with open(template_filename, 'r') as template_file:
>     ...
> ```
> 
> (the with handles closing the file for you)

Fixed.

> It'd probably be nice to include the name of the script in the warning comment.

I've modified the scripts to always include their name in the generated file.

> We generally use `+=` for all assignments in moz.build just so people don't accidentally break things when adding new entries.

Good catch, I hadn't thought about that.

> If you don't need the list of files elsewhere I wouldn't bother using an intermediate variable for it.

Right, I'll get rid of it.

> Is this necessary to avoid leaking the string data? I guess EnumeratedArray doesn't have a clear method because it always has as many entries as enum variants?

Yes, it's a fixed size array, though you made me realize I could clear it with std::fill() which is going to be easier on the eyes.

> It'd probably be fine to just use `TestKey` for all these tests as well. I don't think the annotation name has any special meaning.

I don't think it does either, I'll replace it.

> You should combine this script with the other script since they share so much code. You can specify which function to use in a `GENERATED_FILES` script specifically for this purpose:
> https://firefox-source-docs.mozilla.org/build/buildsystem/mozbuild-symbols.html#generated-files

OK, I'll try to compile all the scripts into one and use :nalexander suggestions too.

Comment 58

[(not currently active) Ted Mielczarek]

Comment on attachment 8965064 [details]
Bug 1348273 - Implementation of machine-generated crash annotations; .mielczarek

https://reviewboard.mozilla.org/r/228644/#review240318

My best suggestion would be froydnj.

Comment 59

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965064 [details]
Bug 1348273 - Implementation of machine-generated crash annotations; .mielczarek

https://reviewboard.mozilla.org/r/228644/#review240318

> If this enumeration code doesn't allocate we could remove this and write this out in the minidump callback instead. Do you know if that's the case?

I don't know. Maybe we can file a follow up to verify it and move the code if that's the case?

Comment 60

[Gabriele Svelto [:gsvelto]]

OK, I think I've addressed all the comments. I've consolidated the code generation into a single Python script, tried to make it more pythonic and extensively used flake8 on it to make sure it's as tidy as possible. I'm going to push again to mozreview but I had to rebase a few days ago so I hope the interdiff is readable.

Comment 61

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965064 [details]
Bug 1348273 - Implementation of machine-generated crash annotations; .mielczarek

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228644/diff/1-2/

Comment 62

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965065 [details]
Bug 1348273 - Convert the accessibility annotations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228646/diff/1-2/

Comment 63

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965066 [details]
Bug 1348273 - Convert the docshell and XPConnect-related annotations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228648/diff/1-2/

Comment 64

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965067 [details]
Bug 1348273 - Convert the IPC-related annotations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228650/diff/1-2/

Comment 65

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965068 [details]
Bug 1348273 - Convert the Android-specific annotations and use the machine-generated whitelist in the crash reporter;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228652/diff/1-2/

Comment 66

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965069 [details]
Bug 1348273 - Convert the graphics annotations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228654/diff/1-2/

Comment 67

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965070 [details]
Bug 1348273 - Convert the telemetry annotations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228656/diff/1-2/

Comment 68

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965071 [details]
Bug 1348273 - Convert the XPCOM annotations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228658/diff/1-2/

Comment 69

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965072 [details]
Bug 1348273 - Convert the network-related annotations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228660/diff/1-2/

Comment 70

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965073 [details]
Bug 1348273 - Convert the media-related annotations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228662/diff/1-2/

Comment 71

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965074 [details]
Bug 1348273 - Convert the storage-related annotations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228664/diff/1-2/

Comment 72

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965075 [details]
Bug 1348273 - Convert the layout annotation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228666/diff/1-2/

Comment 73

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965076 [details]
Bug 1348273 - Convert the browser and toolkit annotations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/228668/diff/1-2/

Comment 74

[Code Review Bot [:reviewbot]]

Comment on attachment 8965069 [details]
Bug 1348273 - Convert the graphics annotations;

https://reviewboard.mozilla.org/r/228654/#review242026


Code analysis found 1 defect in this patch:
 - 1 defect found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/thebes/gfxPlatform.cpp:219
(Diff revision 2)
>    uint32_t mMaxCapacity;
>    int32_t mIndex;
>    Mutex mMutex;
>  };
>  
> -CrashStatsLogForwarder::CrashStatsLogForwarder(const char* aKey)
> +CrashStatsLogForwarder::CrashStatsLogForwarder(CrashReporter::Annotation aKey)

Error: Bad implicit conversion constructor for 'crashstatslogforwarder' [clang-tidy: mozilla-implicit-constructor]

Comment 75

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8965068 [details]
Bug 1348273 - Convert the Android-specific annotations and use the machine-generated whitelist in the crash reporter;

https://reviewboard.mozilla.org/r/228652/#review242252

OK, I have some more nits but nothing that should slow this train down.  (I think I'm already the long pole!)  Thanks for your efforts here.

::: toolkit/crashreporter/generate_crash_reporter_sources.py:185
(Diff revision 2)
> +###############################################################################
> +
> +
> +def generate_java_array_initializer(contents):
> +    """Generates the initializer for an array of strings.
> +    Effectively turns `["a", "b"]` into '   \"a\",\n   \"b\",\n'."""

This comment isn't correct, is it?  You strip the trailing ",\n" before returning it.

::: toolkit/crashreporter/generate_crash_reporter_sources.py:190
(Diff revision 2)
> +    Effectively turns `["a", "b"]` into '   \"a\",\n   \"b\",\n'."""
> +
> +    initializer = ""
> +
> +    for name in contents:
> +        initializer += "        \"" + name + "\",\n"

You can use ' and " for strings, avoiding quoting.  Since you're producing Java, you probably want to use ' for Python strings.  (Here and below.)

::: toolkit/crashreporter/generate_crash_reporter_sources.py:200
(Diff revision 2)
> +def generate_class(template, annotations):
> +    """Fill the class template from the list of annotations."""
> +
> +    whitelist = extract_crash_ping_whitelist(annotations)
> +
> +    return "/* This file was autogenerated by " + \

These are perfect uses for triple-quote strings and `textwrap.dedent`.  See, for example, places like https://searchfox.org/mozilla-central/source/build/moz.configure/rust.configure#62.

::: toolkit/crashreporter/generate_crash_reporter_sources.py:211
(Diff revision 2)
> +
> +
> +def emit_class(output, annotations_filename):
> +    """Generate the CrashReporterConstants.java file."""
> +
> +    template = "package org.mozilla.gecko;\n" \

Ditto for triple-quote strings and `dedent`.

::: toolkit/crashreporter/generate_crash_reporter_sources.py:230
(Diff revision 2)
> +    annotations = read_annotations(annotations_filename)
> +    generated_class = generate_class(template, annotations)
> +
> +    try:
> +        output.write(generated_class)
> +

nit: no blank line.

Comment 76

[(not currently active) Ted Mielczarek]

Comment on attachment 8965064 [details]
Bug 1348273 - Implementation of machine-generated crash annotations; .mielczarek

https://reviewboard.mozilla.org/r/228644/#review244972

This looks great! Thanks for merging the Python files for code generation.

::: toolkit/crashreporter/CrashAnnotations.yaml:10
(Diff revision 2)
> +# - type: the annotation type, currently `string`, `integer` or `boolean`.
> +#   The latter are stringified to `1` for true and `0` for false.
> +#
> +# Additionally a field can have the following optional fields:
> +# - altname: A string that will be used when writing out the annotation to the
> +#   .extra file instead of the annotation name

It doesn't look like altname actually gets used anywhere here. Do you have a planned use for it or is it just a leftover?

::: toolkit/crashreporter/generate_crash_reporter_sources.py:107
(Diff revision 2)
> +    annotations and return it as a string."""
> +
> +    whitelist = extract_crash_ping_whitelist(annotations)
> +    blacklist = extract_content_process_blacklist(annotations)
> +
> +    return "/* This file was autogenerated by " + \

Python's triple-quoted strings let you include literal newlines, if you were so inclined.

Comment 77

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8965064 [details]
Bug 1348273 - Implementation of machine-generated crash annotations; .mielczarek

https://reviewboard.mozilla.org/r/228644/#review244972

> It doesn't look like altname actually gets used anywhere here. Do you have a planned use for it or is it just a leftover?

It's used by the Add-ons annotation. There was another one too but it was obsolete and I removed it.

Comment 78

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Convert crash annotations into a machine-readable list of constants

This is a rebased rollup of all the previous patches. It uses a JavaScript wrapper around the crash reporter service to store the annotations in a JS-accessible way effectively working around bug 1428775. I've tested this pretty thoroughly and I'm confident I should be able to land it soon.

Comment 79

[Gabriele Svelto [:gsvelto]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa989ab400ae78865b16a4af0f8a869f52f89526

Time to land, keeping my fingers crossed.

Comment 80

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86471a18672f
Convert crash annotations into a machine-readable list of constants; r=ted.mielczarek,njn,dholbert,mak,cpearce,mcmanus,froydnj,Dexter,jrmuizel,jchen,jimm,bz,surkov

Comment 81

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 86471a18672f (bug 1348273) for ESlint failure at toolkit/modules/WebNavigationChild.jsm

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef7547cbfdf858cb6c507e142d57fd4c3be17dac

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=86471a18672fb8b1c7edf27076f5e2964e2389f9

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=191938597&repo=mozilla-inbound&lineNumber=264

[task 2018-08-03T19:37:05.123Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python2.7
[task 2018-08-03T19:37:05.123Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python
[task 2018-08-03T19:37:06.747Z] Installing setuptools, pip, wheel...done.
[task 2018-08-03T19:37:07.853Z] running build_ext
[task 2018-08-03T19:37:07.853Z] building 'psutil._psutil_linux' extension
[task 2018-08-03T19:37:07.853Z] creating build
[task 2018-08-03T19:37:07.853Z] creating build/temp.linux-x86_64-2.7
[task 2018-08-03T19:37:07.853Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2018-08-03T19:37:07.853Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-08-03T19:37:07.853Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-08-03T19:37:07.853Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2018-08-03T19:37:07.853Z] creating build/lib.linux-x86_64-2.7
[task 2018-08-03T19:37:07.853Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2018-08-03T19:37:07.853Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2018-08-03T19:37:07.854Z] building 'psutil._psutil_posix' extension
[task 2018-08-03T19:37:07.854Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-08-03T19:37:07.854Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-08-03T19:37:07.854Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2018-08-03T19:37:07.854Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-08-03T19:37:07.854Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-08-03T19:37:07.854Z] 
[task 2018-08-03T19:37:07.854Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-08-03T19:42:38.333Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/modules/WebNavigationChild.jsm:11:1 | 'XPCOMUtils' is defined but never used. (no-unused-vars)
[taskcluster 2018-08-03 19:42:38.877Z] === Task Finished ===
[taskcluster 2018-08-03 19:42:38.877Z] Unsuccessful task run with exit code: 1 completed in 617.122 seconds

Comment 82

[Gabriele Svelto [:gsvelto]]

Darn, I had missed the ES failure, sorry for the noise.

Comment 83

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e9ecba54e7b
Convert crash annotations into a machine-readable list of constants; r=ted.mielczarek,njn,dholbert,mak,cpearce,mcmanus,froydnj,Dexter,jrmuizel,jchen,jimm,bz,surkov

Comment 84

[Noemi Erli[:noemi_erli]]

Backed out changeset 1e9ecba54e7b (Bug 1348273) for failing on widget/tests/test_bug1123480.xul

This was the initial push with the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=86471a18672fb8b1c7edf27076f5e2964e2389f9&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&filter-searchStr=872e2a52c648c79b4899fcc5e595f43a5edb101b&selectedJob=192030425
which was backed out for ES lint failures, but this particular failure reappeared after reland

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=192030425&repo=mozilla-inbound&lineNumber=1357

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4146a5857135b8542858b9e23bc5a71237e05f49

Comment 85

[Gabriele Svelto [:gsvelto]]

This is weird, the test doesn't seem to touch crash reporting code and I really don't see how it could reliably fail at that line while passing in non-ASAN builds. I'll try to investigate next week, leaving the NI for now.

Comment 86

[Kris Maglione [:kmag]]

When this landed, it caused a 23K regression in base content JS, presumably from the new JSM:

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=21cbd88d4ea1&newProject=mozilla-inbound&newRevision=86471a18672fb8b1c7edf27076f5e2964e2389f9&framework=4&filter=base%20content%20js

This is a fairly large regression, given that the majority of our JS memory usage improvements have come from the cumulative effect of 5-10K improvements.

Our JS APIs typically expect string values for their enum arguments, and convert them into integer constants internally. Can you please consider doing the same here?

Comment 87

[Gabriele Svelto [:gsvelto]]

(In reply to Kris Maglione [:kmag] from comment #86)
> Our JS APIs typically expect string values for their enum arguments, and
> convert them into integer constants internally. Can you please consider
> doing the same here?

Yes, I should be able to. What I want to ensure is that we're not using undefined constants but I can check that in native code where I have the full list already and throw if an unknown value is presented. Also note that this should shave off quite a bit of code from libxul. In my tests I've seen a ~6KiB reduction in Windows PGO builds.

Comment 88

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Convert crash annotations into a machine-readable list of constants

This is an updated patch taking into account Kris suggestion above. All JavaScript call sites have been left untouched and the API internally converts the JavaScript-provided string to one of the known constants (or throws if it doesn't correspond to a known value).

Content process memory use should now be mostly unaffected WRT JavaScript code. Since crash annotations are infrequent the performance cost of the string-to-enum conversion should also be immaterial.

Comment 89

[Gabriele Svelto [:gsvelto]]

Try is looking good this time, let's try to land this again:

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

Comment 90

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42e2eeaca65d
Convert crash annotations into a machine-readable list of constants; r=ted.mielczarek,njn,dholbert,mak,cpearce,mcmanus,froydnj,Dexter,jrmuizel,jchen,jimm,bz,surkov

Comment 91

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/42e2eeaca65d