Closed Bug 1348273 Opened 7 years ago Closed 6 years ago

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

Categories

(Toolkit :: Crash Reporting, enhancement)

Product:
Toolkit
Component:
Crash Reporting
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: gsvelto, Assigned: gsvelto)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Whiteboard: [fce-active-legacy])

Attachments

(14 files, 9 obsolete files)

Bug 1348273 - Implementation of machine-generated crash annotations; .mielczarek
59 bytes, text/x-review-board-request
ted
: review+
Details
Bug 1348273 - Convert the accessibility annotations;
59 bytes, text/x-review-board-request
surkov
: review+
Details
Bug 1348273 - Convert the docshell and XPConnect-related annotations;
59 bytes, text/x-review-board-request
bzbarsky
: review+
Details
Bug 1348273 - Convert the IPC-related annotations;
59 bytes, text/x-review-board-request
jimm
: review+
Details
Bug 1348273 - Convert the Android-specific annotations and use the machine-generated whitelist in the crash reporter;
59 bytes, text/x-review-board-request
nalexander
: review+
Details
Bug 1348273 - Convert the graphics annotations;
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details
Bug 1348273 - Convert the telemetry annotations;
59 bytes, text/x-review-board-request
Dexter
: review+
Details
Bug 1348273 - Convert the XPCOM annotations;
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1348273 - Convert the network-related annotations;
59 bytes, text/x-review-board-request
mcmanus
: review+
Details
Bug 1348273 - Convert the media-related annotations;
59 bytes, text/x-review-board-request
cpearce
: review+
Details
Bug 1348273 - Convert the storage-related annotations;
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1348273 - Convert the layout annotation;
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1348273 - Convert the browser and toolkit annotations;
59 bytes, text/x-review-board-request
n.nethercote
: review+
Details
[PATCH] Convert crash annotations into a machine-readable list of constants
205.66 KB, patch
Details | Diff | Splinter Review 
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.
Highly related and on my wishlist: bug 1323002
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.
(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.
(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.
See Also: → 1370215
Whiteboard: [fce-active]
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?
Flags: needinfo?(ted)
Attached patch [PATCH] WIP (obsolete) — — Splinter Review 
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.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
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.
Flags: needinfo?(ted)
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.
Flags: needinfo?(jmuizelaar)
Attached file [PATCH v2] WIP (obsolete) — 
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.
Attachment #8882825 - Attachment is obsolete: true
Attached patch [PATCH v3] WIP (obsolete) — — Splinter Review 
First working patch; it still has a few rough edges but functionally speaking it's complete.
Attachment #8904771 - Attachment is obsolete: true
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.
Depends on: 1402153
Depends on: 1402519
Depends on: 1416078
Blocks: 1416078
No longer depends on: 1416078
Attached patch [PATCH v4] WIP (obsolete) — — Splinter Review 
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.
Attachment #8908867 - Attachment is obsolete: true
Depends on: 1428775
Attached patch [PATCH v5] WIP (obsolete) — — Splinter Review 
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.
Attachment #8934296 - Attachment is obsolete: true
Clearing the NI for now.
Flags: needinfo?(jmuizelaar)
Attached patch [PATCH v6] WIP (obsolete) — — Splinter Review 
More Windows-specific fixes.
Attachment #8941102 - Attachment is obsolete: true
Depends on: 1434944
See Also: → 1435683
Whiteboard: [fce-active] → [fce-active-legacy]
Attached patch [PATCH v7] WIP (obsolete) — — Splinter Review 
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.
Attachment #8946664 - Attachment is obsolete: true
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.
Attachment #8964019 - Attachment is obsolete: true
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.
[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.]
[er never mind - MozReview says heycam's not accepting review requests, so I'll keep that one after all.]
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 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)
Attachment #8965075 - Flags: review?(dholbert) → review+
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
Attachment #8965068 - Flags: review?(nchen)
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
Attachment #8965068 - Flags: review?(nchen)
Attachment #8965068 - Flags: review?(nchen) → review?(nalexander)
Comment on attachment 8965074 [details]
Bug 1348273 - Convert the storage-related annotations;

https://reviewboard.mozilla.org/r/228664/#review239454
Attachment #8965074 - Flags: review?(mak77) → review+
Comment on attachment 8965071 [details]
Bug 1348273 - Convert the XPCOM annotations;

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

This is great stuff, thank you.
Attachment #8965071 - Flags: review?(nfroyd) → review+
Comment on attachment 8965072 [details]
Bug 1348273 - Convert the network-related annotations;

https://reviewboard.mozilla.org/r/228660/#review239466
Attachment #8965072 - Flags: review?(mcmanus) → review+
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
Attachment #8965065 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8965073 [details]
Bug 1348273 - Convert the media-related annotations;

https://reviewboard.mozilla.org/r/228662/#review239512
Attachment #8965073 - Flags: review?(cpearce) → review+
Comment on attachment 8965066 [details]
Bug 1348273 - Convert the docshell and XPConnect-related annotations;

https://reviewboard.mozilla.org/r/228648/#review239536
Attachment #8965066 - Flags: review?(bzbarsky) → review+
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?
Attachment #8965076 - Flags: review?(n.nethercote) → review+
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
Attachment #8965070 - Flags: review?(alessio.placitelli) → review+
Comment on attachment 8965069 [details]
Bug 1348273 - Convert the graphics annotations;

https://reviewboard.mozilla.org/r/228654/#review239742
Attachment #8965069 - Flags: review?(jmuizelaar) → review+
(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)
(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 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.
Attachment #8965068 - Flags: review?(nalexander) → review-
> 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.
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 on attachment 8965067 [details]
Bug 1348273 - Convert the IPC-related annotations;

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

lgtm, awesome cleanup work, thanks.
Attachment #8965067 - Flags: review?(jmathies) → review+
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
Attachment #8965064 - Flags: review?(ted) → review-
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 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 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 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 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?
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 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 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.
Attachment #8965068 - Flags: review+
Attachment #8965068 - Flags: review?(nchen)
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.
Attachment #8965064 - Flags: review?(ted) → review+
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.
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.
Pushed by gsvelto@mozilla.com:
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
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
Flags: needinfo?(gsvelto)
Darn, I had missed the ES failure, sorry for the noise.
Flags: needinfo?(gsvelto)
Pushed by gsvelto@mozilla.com:
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
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.
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?
(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.
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.
Attachment #8964020 - Attachment is obsolete: true
Attachment #8997207 - Attachment is obsolete: true
Flags: needinfo?(gsvelto)
Pushed by gsvelto@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/42e2eeaca65d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1502977
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: