670194 - Startup numbers don't account for interactive startup interruptions

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[(dormant account)]

Since startup numbers do not make sense when interrupted by interactive processes, we either need to add a .interactive component or a flag so we can track these.

Given how startups take minutes due to interruptions, it might even make sense to change when those prompts appear(ie make them happen after sessionrestore).

Interruptions:
* profile manager
* extensions update manager
* windows updater UAC
* update process is currently counted as part of startup

Fixing this should allow us to get a useful startup metric

Comment 1

[(dormant account)]

Margaret, do you have an ETA on this?

Comment 2

[:Margaret Leibovic]

I am starting to look into this today. Do we want to start by just excluding these interrupted startups from our startup metrics, or jump right into trying to change when we present these prompts? It seems like we wouldn't be able to move some of them, like the profile manager, so I guess we would still need a way to exclude this from startup metrics.

Taras, do you have any pointers to where/how we measure startup time? I'm unfamiliar with this, so any random resources would be helpful.

Comment 3

[(dormant account)]

Actually this is easier than that. We just need to set a flag some where *startup was interrupted*. No need to try to compensate for it.

Comment 4

[:Margaret Leibovic]

Okay, that seems straight forward enough. Do we want to set this flag on the telemetry service? Or is there another place you were thinking of storing it?

Comment 5

[(dormant account)]

(In reply to Margaret Leibovic [:margaret] from comment #4)
> Okay, that seems straight forward enough. Do we want to set this flag on the
> telemetry service? Or is there another place you were thinking of storing it?

If you cant think of a better place nsITelemetry sounds good.

Comment 6

[(dormant account)]

(In reply to Taras Glek (:taras) from comment #5)
> (In reply to Margaret Leibovic [:margaret] from comment #4)
> > Okay, that seems straight forward enough. Do we want to set this flag on the
> > telemetry service? Or is there another place you were thinking of storing it?
> 
> If you cant think of a better place nsITelemetry sounds good.

Sorry, I forgot the obvious interface for this: nsIAppStartup.interrupted = true would be a good way to go here

Comment 7

[:Margaret Leibovic]

Attachment: possible patch for extensions update dialog

I started with the extensions update dialog. Is this the right idea?

I'm also unsure how I should test this. I was looking around for a possible place to add a test in the existing extensions update test code, but I didn't see an obvious place. Dave, do you know if there's an easy way to add a test for this?

Comment 8

[(dormant account)]

Comment on attachment 554475 [details] [diff] [review]
possible patch for extensions update dialog

This is exactly what I'm looking for. Please use Cc/Ci.

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 554475 [details] [diff] [review]
possible patch for extensions update dialog

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

::: toolkit/components/startup/public/nsIAppStartup.idl
@@ +138,5 @@
> +
> +    /**
> +     * True if startup was interrupted by an interactive process.
> +     */
> +    attribute boolean interrupted;

Seem to be missing an implementation for this method, however I'm not sure I agree that nsIAppStartup is the right place for this. This seems like something very specific to the telemetry system so I'd be much more inclined to put it there. Was there some other reason you thought of putting it here Taras?

::: toolkit/mozapps/extensions/content/update.js
@@ +85,5 @@
>        document.documentElement.currentPage = document.getElementById("versioninfo");
> +
> +    let appService = Components.classes["@mozilla.org/toolkit/app-startup;1"].
> +                     getService(Components.interfaces.nsIAppStartup);
> +    appService.interrupted = true;

I would probably do this in XPIProvider.jsm where we open this as there are now two different dialogs there that can be opened on startup.

Comment 10

[(dormant account)]

(In reply to Dave Townsend (:Mossop) from comment #9)
> Comment on attachment 554475 [details] [diff] [review]
> possible patch for extensions update dialog
> 
> Review of attachment 554475 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/startup/public/nsIAppStartup.idl
> @@ +138,5 @@
> > +
> > +    /**
> > +     * True if startup was interrupted by an interactive process.
> > +     */
> > +    attribute boolean interrupted;
> 
> Seem to be missing an implementation for this method, however I'm not sure I
> agree that nsIAppStartup is the right place for this. This seems like
> something very specific to the telemetry system so I'd be much more inclined
> to put it there. Was there some other reason you thought of putting it here
> Taras?

It makes sense to me because .interrupted is needed to make sure of .firstPaint and .sessionRestored numbers provided nsIAppStartup.

Comment 11

[Dave Townsend [:mossop]]

(In reply to Taras Glek (:taras) from comment #10)
> (In reply to Dave Townsend (:Mossop) from comment #9)
> > Comment on attachment 554475 [details] [diff] [review]
> > possible patch for extensions update dialog
> > 
> > Review of attachment 554475 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/components/startup/public/nsIAppStartup.idl
> > @@ +138,5 @@
> > > +
> > > +    /**
> > > +     * True if startup was interrupted by an interactive process.
> > > +     */
> > > +    attribute boolean interrupted;
> > 
> > Seem to be missing an implementation for this method, however I'm not sure I
> > agree that nsIAppStartup is the right place for this. This seems like
> > something very specific to the telemetry system so I'd be much more inclined
> > to put it there. Was there some other reason you thought of putting it here
> > Taras?
> 
> It makes sense to me because .interrupted is needed to make sure of
> .firstPaint and .sessionRestored numbers provided nsIAppStartup.

Ah I see, that makes more sense. I'm fine with that then.

Comment 12

[:Margaret Leibovic]

Attachment: patch attempt 2

I addressed the comments. I still don't have any tests, so if you have ideas about that, I'd love to hear :)

Comment 13

[(dormant account)]

Do any of the dialogs that interrupt startup have tests?

Comment 14

[Dave Townsend [:mossop]]

Comment on attachment 555543 [details] [diff] [review]
patch attempt 2

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

Isn't this patch also meant to be doing something when nsIAppStartup.interrupted is true, like not including the perf numbers?

Comment 15

[Dave Townsend [:mossop]]

(In reply to Taras Glek (:taras) from comment #13)
> Do any of the dialogs that interrupt startup have tests?

Yes, the xpcshell tests check that the add-ons manager attempts to open those dialog so they could also test that the startup timing numbers no longer exist in those cases

Comment 16

[(dormant account)]

(In reply to Dave Townsend (:Mossop) from comment #15)
> (In reply to Taras Glek (:taras) from comment #13)
> > Do any of the dialogs that interrupt startup have tests?
> 
> Yes, the xpcshell tests check that the add-ons manager attempts to open
> those dialog so they could also test that the startup timing numbers no
> longer exist in those cases

The plan is to still report startup numbers(at least for telemetry). We'd also send a .startupInterrupted metric along that would reflect value of nsIAppStartup.interrupted. I was planning to do that patch in a follow up.

Margaret, please add do_check_true(nsIAppStartup.interrupted) to xpcshell tests for addon manager.

Comment 17

[:Margaret Leibovic]

Attachment: patch w/ test

Gavin suggested I add startup to Services.jsm, so I did that.

Comment 18

[(dormant account)]

Comment on attachment 555794 [details] [diff] [review]
patch w/ test

By the way, this only takes care of addon manager, what about profile chooser?

There is also the windows UAC upgrade prompt(I can add that).

Comment 19

[Dave Townsend [:mossop]]

Looks basically ok, one thought I'd like to hear about though is whether we can make this more automatic. If nsAppStartup listened for the window observer notificatons (I think it already does) then it could detect windows opening during startup and just set the interrupted flag manually. This would guarantee we caught every case straight off the bat and allow us to make that property readonly. Makes testing marginally more difficult but not by much.

Comment 20

[Dave Townsend [:mossop]]

Comment on attachment 555794 [details] [diff] [review]
patch w/ test

Discussed over IRC, probably not worth the automated approach for the small number of dialogs we have. Maybe we'll revisit that if there are many that we're forgetting.

Comment 21

[:Margaret Leibovic]

(In reply to Taras Glek (:taras) from comment #18)
> By the way, this only takes care of addon manager, what about profile
> chooser?

Sorry, I was just trying to start with the most common dialog, then lap back to covering others. I can do that now.

Comment 22

[:Margaret Leibovic]

Attachment: patch for profile manager

I wrote this patch for the profile manager, but I'm not sure if it's what we want.

I used dump statements near my changes to make sure they were actually running, but when I manually checked the value of Services.startup.interrupted after the browser launched, it wasn't set to true. I think the browser restarts after a profile is selected, so I don't think that time spent interacting with this dialog would be counted in startup time. Am I right about this? However, is the startup of just the profile manager still being tracked by some telemetry probes?

Comment 23

[:Margaret Leibovic]

I landed my first patch in fx-team, since it's ready:
http://hg.mozilla.org/integration/fx-team/rev/8ef08b003e21

Taras, any insight into what I ran into in comment 22?

Comment 24

[(dormant account)]

(In reply to Margaret Leibovic [:margaret] from comment #22)
> Created attachment 557626 [details] [diff] [review]
> patch for profile manager
> 
> I wrote this patch for the profile manager, but I'm not sure if it's what we
> want.
> 
> I used dump statements near my changes to make sure they were actually
> running, but when I manually checked the value of
> Services.startup.interrupted after the browser launched, it wasn't set to
> true. I think the browser restarts after a profile is selected, so I don't
> think that time spent interacting with this dialog would be counted in
> startup time. Am I right about this? However, is the startup of just the
> profile manager still being tracked by some telemetry probes?

Yeah, I suspect you are right. The way I deal with this is set MOZ_APP_RESTART in nsAppStartup. You could add another env variable or commandline flag or something.

Comment 25

[:Margaret Leibovic]

Landed the first patch on mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/8ef08b003e21

Comment 26

[Daniel C]

Mozilla/5.0 (X11; Linux i686; rv:9.0a1) Gecko/20110915 Firefox/9.0a1

(In reply to Taras Glek (:taras) from comment #24)
> (In reply to Margaret Leibovic [:margaret] from comment #22)
> > Created attachment 557626 [details] [diff] [review]
> > patch for profile manager
> > 
> > I wrote this patch for the profile manager, but I'm not sure if it's what we
> > want.
> > 
> > I used dump statements near my changes to make sure they were actually
> > running, but when I manually checked the value of
> > Services.startup.interrupted after the browser launched, it wasn't set to
> > true. I think the browser restarts after a profile is selected, so I don't
> > think that time spent interacting with this dialog would be counted in
> > startup time. Am I right about this? However, is the startup of just the
> > profile manager still being tracked by some telemetry probes?
> 
> Yeah, I suspect you are right. The way I deal with this is set
> MOZ_APP_RESTART in nsAppStartup. You could add another env variable or
> commandline flag or something.

1. At the profile manager, pick a profile really quickly.
2. Check about:startup.
3. Quit and start again.
4. Wait 30 seconds at the profile manager and then pick a profile.
5. Check about:startup.

Step 2 gives:

main    sessionRestored firstPaint      version appBuildID
1534    4597            6348            9.0a1   20110915030845

Step 5 gives:

main    sessionRestored firstPaint      version appBuildID
29569   32474           32750           9.0a1   20110915030845

Comment 27

[Nickolay_Ponomarev]

https://developer.mozilla.org/en/nsIAppStartup should be updated.

Comment 28

[:Margaret Leibovic]

I haven't had time to work on this lately, so I'm just going to close this bug. I filed bug 691774 as a follow-up.

Comment 29

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIAppStartup

Also mentioned on Firefox 9 for developers.