1514044 - macro-ize GeckoProcessType enum values and associated code

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nathan Froyd [:froydnj]]

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

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - macro-ify process enums, strings, and XRE functions

Having this information all defined in a single header will make life
easier if we ever add new process types.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - change CrashReporterHost to use GeckoProcessTypes.h

It's not the most elegant code, but at least we can mostly avoid adding code
here for new process types.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3 - update nsICrashService process constants and add checks for them

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?

Comment 4

[Nathan Froyd [:froydnj]]

Should note that the first part depends on bug 1514043.

Comment 5

[Andrew McCreight [:mccr8]]

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?

Comment 6

[Gabriele Svelto [:gsvelto]]

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

[Gabriele Svelto [:gsvelto]]

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.

Comment 8

[Gabriele Svelto [:gsvelto]]

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.

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 1 - macro-ify process enums, strings, and XRE functions

Updated typos and comments.  Carrying over r+.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: part 2 - change CrashReporterHost to use GeckoProcessTypes.h

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.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: part 3 - update nsICrashService process constants and add checks for them

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.

Comment 12

[Gabriele Svelto [: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!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/1abd03bcbfa3
https://hg.mozilla.org/mozilla-central/rev/65f142863701
https://hg.mozilla.org/mozilla-central/rev/de7c04ff6001

Comment 15

[Julien Cristau [:jcristau]]

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?

Comment 16

[Gabriele Svelto [:gsvelto]]

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

Comment 17

[Julien Cristau [:jcristau]]

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.

Comment 18

[Nathan Froyd [:froydnj]]

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