macro-ize GeckoProcessType enum values and associated code

RESOLVED FIXED in Firefox 66

Status

()

enhancement
RESOLVED FIXED
7 months ago
6 months ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

(Blocks 1 bug)

unspecified
mozilla66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

We have multiple places you have to touch to update XPCOM's idea of what the process type enumeration is:

https://searchfox.org/mozilla-central/source/xpcom/build/nsXULAppAPI.h#367-386,434-454

We should hide all of this behind a ProcessTypes.h header with a GECKO_PROCESS_TYPE macro or similar that can be used to codegen all the appropriate bits.  Once we have that, we can even add other data-driven bits, like whether chrome manifests should be loaded in your process:

https://searchfox.org/mozilla-central/source/xpcom/components/nsComponentManager.cpp#341-344
Having this information all defined in a single header will make life
easier if we ever add new process types.
Attachment #9034425 - Flags: review?(continuation)
It's not the most elegant code, but at least we can mostly avoid adding code
here for new process types.
Attachment #9034426 - Flags: review?(gsvelto)
This code is kind of an experiment in helping people to discover what code they
need to modify when a new process type is added.

I guess asserting these equivalences could also be used to simplify the switch
in NotifyCrashService: once you know that the constants are equivalent, you can
say:

  switch(aProcessType) {
    case GeckoProcessType_IPDLUnitTest:
    case GeckoProcessType_Default:
    // Other process types we don't care about...
      NS_ERROR("Unknown process type");
      return;
    default:
      // All the process types we do care about
      processType = (int)aProcessType;
      break;
  }

and then you don't need to add explicit cases for new process types.  WDYT?
Attachment #9034428 - Flags: review?(gsvelto)
Should note that the first part depends on bug 1514043.
Depends on: 1514043
Comment on attachment 9034425 [details] [diff] [review]
part 1 - macro-ify process enums, strings, and XRE functions

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

::: xpcom/build/GeckoProcessTypes.h
@@ +4,5 @@
> + * 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/. */
> +
> +// GECKO_PROCESS_TYPE(enum-name, string-name, XRE_Is${NAME}Process)
> +// Note that string-name is exposed to various things like Telemtry

Telemetry. Also, maybe it should be a lower case t?

::: xpcom/build/nsXULAppAPI.h
@@ -373,5 @@
> -  GeckoProcessType_IPDLUnitTest,
> -
> -  GeckoProcessType_GMPlugin,  // Gecko Media Plugin
> -
> -  GeckoProcessType_GPU,  // GPU and compositor process

Can these descriptions be added to the new header?
Attachment #9034425 - Flags: review?(continuation) → review+
(In reply to Nathan Froyd [:froydnj] from comment #3)
> I guess asserting these equivalences could also be used to simplify the
> switch
> in NotifyCrashService: 
> [...]
> and then you don't need to add explicit cases for new process types.  WDYT?

I like it. This code is the only user of the crash service so if changes to the crash service can make it even simpler then we should just do them.
Comment on attachment 9034426 [details] [diff] [review]
part 2 - change CrashReporterHost to use GeckoProcessTypes.h

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

Macroization always makes the code harder to read, but in this case it will also cause less people to have to look at it so I'm very happy with the change!

::: ipc/glue/CrashReporterHost.cpp
@@ +77,5 @@
>    CrashReporter::AnnotationTable annotations;
>  
>    nsAutoCString type;
> +  // The gecko media plugin and normal plugin processes are lumped together
> +  // as a historical artifact.

It might be worth opening a bug to remove this special case even though it would require some changes on Socorro's side.

@@ +141,5 @@
>  
>    int32_t processType;
>    nsCString telemetryKey;
>  
>    switch (aProcessType) {

Go with the int cast here, the simpler the better.

::: toolkit/crashreporter/CrashAnnotations.yaml
@@ +554,5 @@
>  
>  ProcessType:
>    description: >
> +    Type of the process that crashed, can hold string values defined in
> +    GeckoProcessTypes.h currently.

Maybe the second part could simply be reworded as "the possible values are defined in GeckoProcessTypes.h". After all that should always be accurate from this point on.
Attachment #9034426 - Flags: review?(gsvelto) → review+
Comment on attachment 9034428 [details] [diff] [review]
part 3 - update nsICrashService process constants and add checks for them

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

I'm not entirely comfortable with adding new entries to the nsICrashService interface without handling them in the implementation, comments below.

::: toolkit/components/crashes/nsICrashService.idl
@@ +23,5 @@
>  
>    const long PROCESS_TYPE_MAIN = 0;
> +  const long PROCESS_TYPE_PLUGIN = 1;
> +  const long PROCESS_TYPE_CONTENT = 2;
> +  const long PROCESS_TYPE_IPDLUNITTEST = 3;

I don't think we'll ever send crash reports for this so addCrash() should explicitly ignore it. As it is it would throw instead.

@@ +26,5 @@
> +  const long PROCESS_TYPE_CONTENT = 2;
> +  const long PROCESS_TYPE_IPDLUNITTEST = 3;
> +  const long PROCESS_TYPE_GMPLUGIN = 4;
> +  const long PROCESS_TYPE_GPU = 5;
> +  const long PROCESS_TYPE_VR = 6;

I'm OK with not adding this to the implementation of addCrash() but instead we should file a bug to add crash reporting for VR processes. This way whoever is involved with that will be made aware that they need to act if they want to get crash reports.

On this topic, I should probably write some documentation on what's needed to add new processes to the crash reporting flow. It's complex, involves external components (Telemetry, Socorro) so a guide would be helpful.
Updated typos and comments.  Carrying over r+.
Attachment #9035425 - Flags: review+
Attachment #9034425 - Attachment is obsolete: true
Carrying over r+ here.  The VR crash reporting is bug 1518895.  The int cast
for the switch discussed in comment 3 has been placed in part 3.
Attachment #9035426 - Flags: review+
Attachment #9034426 - Attachment is obsolete: true
Updated to make use of the equivalences we were asserting, and to bail if we
ever try to add a crash for the ipdl unit test process.
Attachment #9035427 - Flags: review?(gsvelto)
Attachment #9034428 - Attachment is obsolete: true
Attachment #9034428 - Flags: review?(gsvelto)
Comment on attachment 9035427 [details] [diff] [review]
part 3 - update nsICrashService process constants and add checks for them

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

Looking good!
Attachment #9035427 - Flags: review?(gsvelto) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1abd03bcbfa3
part 1 - macro-ify process enums, strings, and XRE functions; r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/65f142863701
part 2 - change CrashReporterHost to use GeckoProcessTypes.h; r=gsvelto
https://hg.mozilla.org/integration/mozilla-inbound/rev/de7c04ff6001
part 3 - update nsICrashService process constants and add checks for them; r=gsvelto

As far as I can tell this changed process type from "content" to "tab" for content process crashes, which socorro doesn't expect. E.g. https://crash-stats.mozilla.com/topcrashers/?product=Firefox&version=66.0a1&process_type=content&days=7&_range_type=build is now empty. Was that intended?

Flags: needinfo?(nfroyd)
Flags: needinfo?(gsvelto)

This is something that I missed during review. I'm not sure if it was intentional or not.

Flags: needinfo?(gsvelto)

OK thanks. I sent a heads-up to the stability list for now, and if this change remains in place I guess we'll need a follow-up in socorro.

(In reply to Julien Cristau [:jcristau] from comment #15)

As far as I can tell this changed process type from "content" to "tab" for content process crashes, which socorro doesn't expect. E.g. https://crash-stats.mozilla.com/topcrashers/?product=Firefox&version=66.0a1&process_type=content&days=7&_range_type=build is now empty. Was that intended?

Oh my. That was not intended, sorry about that. It will make CrashReporterHost a little bit uglier, but I think that's possibly better than fixing it on the Socorro side? Happy to do either one, but I'd need a bit of a pointer for the Socorro side of things.

Flags: needinfo?(nfroyd) → needinfo?(gsvelto)
You need to log in before you can comment on or make changes to this bug.