1191351 - CrashService.js takes up 85ms on startup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

On my Nexus 4, CrashService.js takes up 85ms loading CrashManager.jsm in the profile-after-change observer.

Comment 1

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

AFAICT, CrashService/CrashManager is not used in Fennec; do you know if that's correct, gps?

If so, we should not include CrashService/CrashManager in Fennec startup.

Comment 2

[Gregory Szorc [:gps]]

I don't know.

Comment 3

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

rnewman? Does Fennec FHR use CrashService/CrashManager?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

I see CrashManager in CrashSubmit.jsm

I also wonder if it's used by about:crashes

Comment 5

[Richard Newman [:rnewman]]

We shouldn't be shipping CrashService (or CrashManager, IMO) on Fennec, let alone initing it. This should be exploding, not succeeding and impacting our startup time.

All of this is in our package-manifest, and should not be:

; [Crash Reporter]
;
#ifdef MOZ_CRASHREPORTER
@BINPATH@/components/CrashService.manifest
@BINPATH@/components/CrashService.js
@BINPATH@/crashreporter@BIN_SUFFIX@
@BINPATH@/crashreporter.crt
@BINPATH@/crashreporter.ini
#ifdef XP_UNIX
@BINPATH@/Throbber-small.gif
#endif
@BINPATH@/crashreporter-override.ini
#endif



We should probably not be initing it during startup on desktop, either:

/**
 * This component makes crash data available throughout the application.
 *
 * It is a service because some background activity will eventually occur.
 */

is a pretty poor excuse.

CrashManager is only used by the Gecko CrashSubmit.jsm and by Gecko FHR, which we don't ship on Fennec.

The accessor is lazy, so the culprit is service init.

about:crashes in toolkit has a lazy dependency on CrashSubmit (and thus CrashManager and thus CrashService) in order to submit pending crashes. That won't be the cause of this initialization, but it will break if we remove CrashSubmit (perhaps it's already broken!). I think we should remove or (runtime) conditionalize that part of toolkit/crashreporter/content/crashes.js.

Comment 6

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

Eugen, since you've been doing startup work, want to take this one?

Comment 7

[Eugen Sawin [:esawin]]

Looks interesting, I'll look into it.

Comment 8

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1191351-Don-t-package-crash-service-on-Android.patch

Since we don't seem to depend on the CrashService at all, we should remove it.

However, with submitPendingReport remaining an active path, we should keep crashreporter*.ini around for strings (loaded in CrashSubmit.getL10NStrings).

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1191351-Disable-pending-crash-report-submitting-.patch

I'm not sure if I'm interpreting comment 5 correctly, but by disabling submitPendingReport on Android, we also get rid of the CrashSubmit and CrashManager dependencies.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8666182 [details] [diff] [review]
0001-Bug-1191351-Don-t-package-crash-service-on-Android.patch

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

Add a comment to explain why the .ini is left, that we do crashes differently on Android, and a pointer to this bug.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8666184 [details] [diff] [review]
0002-Bug-1191351-Disable-pending-crash-report-submitting-.patch

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

302 rhelmer (and please skim the history in this bug!).

Eugen: I expect we want to keep this functionality, rather than remove it, but it'd need to be a different implementation.

Comment 12

[Eugen Sawin [:esawin]]

OK, but let's move it out of this bug since it's not impacting startup time then.

Comment 13

[Eugen Sawin [:esawin]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e646ad706833 (unrelated bustage on chunk 13)

Comment 14

[Robert Helmer [:rhelmer]]

Comment on attachment 8666184 [details] [diff] [review]
0002-Bug-1191351-Disable-pending-crash-report-submitting-.patch

I don't think we want to disable sending crashes from about:crashes completely as it's the only way to submit a crash if the breakpad client fails (offline, server unreachable etc) and folks use it frequently AFAIK.

The actual submit function in CrashSubmit.jsm is pretty simple, it's just a multi-part XHR form post (https://dxr.mozilla.org/mozilla-central/rev/6256ec9113c115141aab089c45ee69438884b680/toolkit/crashreporter/CrashSubmit.jsm#253)

Looks like B2G is also using this JSM, otherwise I might suggest moving it into about:crashes if it wasn't shared code.

I'm not sure why this isn't just being lazy-loaded as-needed by about:crashes - sorry but I am going to 302 as well, to Ted who wrote most of this in the first place.

Comment 15

[Eugen Sawin [:esawin]]

> I'm not sure why this isn't just being lazy-loaded as-needed by
> about:crashes - sorry but I am going to 302 as well, to Ted who wrote most
> of this in the first place.

The lazy-loading of CrashSubmit.jsm in about:crashes works fine, we've only had an issue with CrashService.jsm importing CrashManager.jsm on startup, which is fixed in the first patch.

rnewman just mentioned that we might want to refactor the about:crashes submitPendingReport code path to reduce some dependencies.

I'd be happy to just land the first patch, we can look into the refactoring in a follow-up.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a31d5b708202

Comment 17

[Robert Helmer [:rhelmer]]

(In reply to Eugen Sawin [:esawin] from comment #15)
> > I'm not sure why this isn't just being lazy-loaded as-needed by
> > about:crashes - sorry but I am going to 302 as well, to Ted who wrote most
> > of this in the first place.
> 
> The lazy-loading of CrashSubmit.jsm in about:crashes works fine, we've only
> had an issue with CrashService.jsm importing CrashManager.jsm on startup,
> which is fixed in the first patch.
> 
> rnewman just mentioned that we might want to refactor the about:crashes
> submitPendingReport code path to reduce some dependencies.
> 
> I'd be happy to just land the first patch, we can look into the refactoring
> in a follow-up.

Makes sense.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a31d5b708202

Comment 19

[(not currently active) Ted Mielczarek]

Not packaging CrashService.js is fine as that's only for desktop FHR apparently. Disabling about:crashes resubmission would not be cool, we don't have any other UI for that.