568921 - setTimeouts in delayedStartup should be on idleService

Status: NEW

(Firefox :: General, defect)

Description

[Johnathan Nightingale [:johnath]]

Looking at delayedStartup as a result of startup timeline pieces, it made me uncomfortable to see that we have several setTimeouts which call code a few seconds after startup, without a really clear sense of why those are the correct values.

Starting around here:

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1345

we have timeouts for 4, 5, 10 and 15 seconds after startup. While that is probably outside the straight "paint a window on the screen" startup, and certainly outside Ts, I think it stands a high chance of landing right in the middle of the user trying to do whatever the first thing was they wanted to launch their browser for, and hence hurting perceived performance by making us momentarily unresponsive.

Full disclosure: I haven't profiled each of these pieces to understand whether they are likely to hurt responsiveness individually. Nevertheless, I can't see a really good reason to be doing these here, versus hanging them off the idle service. Can they all be moved?

Copying some people implicated by hg blame as having added these, in case I'm missing a reason why idle service is inappropriate (even then, I'd like to know why these are the right values, aside from ordering the timeouts sequentially).

Comment 1

[:Gavin Sharp [email: [e-mail]]]

I don't think we want these to be dependent on user idle time, which is what the idle service tracks, AFAIK. A user who starts his browser and uses it continuously should still get download statusbar notifications and livemark updates at some point, right?

It looks like (based only on its IDL documentation) the smallest interval the idle service supports is 5 seconds, which doesn't seem small enough to be useful here.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

On the other hand, I think it would likely be fine for startPlacesDBUtils(), if that really only cares about being run once per day (though in that case, I wonder why it isn't just using something like nsIUpdateTimerManager?).

Comment 3

[Johnathan Nightingale [:johnath]]

Well, in the download case my understanding is that he/she will get download notifications if they spawn any new downloads since that would cause the dlmgr to init itself, this is just for resuming paused downloads - and maybe that's okay to wait until they are idle?

Comment 4

[Ed Lee :Mardak]

At least part of the download timeout block is to initialize the download manager, and this will trigger auto-resume as well as show the state of paused/resumed in the status bar.

So it's not just a background downloading thing as it will show information to the user. There seems to be a windows7-specific code for I'm assuming updating the download progress in the taskbar.

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #2)
> On the other hand, I think it would likely be fine for startPlacesDBUtils(), if
> that really only cares about being run once per day (though in that case, I
> wonder why it isn't just using something like nsIUpdateTimerManager?).

PlacesDBUtils uses the updateTimerManager, but something has to register the timer somewhen. I had set that to 15 seconds because it was long enough after startup, We could just call it on the browserGlue idle timer, sure, but users doing short sessions with rare idles won't even run it, we have a similar issue with backups.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #5)
> PlacesDBUtils uses the updateTimerManager, but something has to register the
> timer somewhen.

Can't one of the places components register it using the category manager method?

(Offtopic for this bug, but what calls the PlacesDBUtils getter that actually instantiates the service? startPlacesDBUtils does the Cu.import(), but I don't see an invocation of it anywhere, unless it's somehow implicit in the import()...).

Comment 7

[Marco Bonardo [:mak]]

(In reply to comment #6)
> (In reply to comment #5)
> > PlacesDBUtils uses the updateTimerManager, but something has to register the
> > timer somewhen.
> 
> Can't one of the places components register it using the category manager
> method?

Sure, but it would not save anything, instead it would start earlier making Ts worse for real. We don't have anything that implements a delayed startup in Places, adding it would mean registering a delay startup timer, we would practically just move code from browser.js to another file, with no win.

> (Offtopic for this bug, but what calls the PlacesDBUtils getter that actually
> instantiates the service? startPlacesDBUtils does the Cu.import(), but I don't
> see an invocation of it anywhere

When it's imported the object is instantiated and that registers the timer

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #7)
> Sure, but it would not save anything, instead it would start earlier making Ts
> worse for real. We don't have anything that implements a delayed startup in
> Places, adding it would mean registering a delay startup timer, we would
> practically just move code from browser.js to another file, with no win.

I don't think you understood what I'm proposing - I'm suggesting using the technique mentioned at http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsIUpdateTimerManager.idl#64 . It avoids the need to invoke the nsIUpdateTimerManager to register the timer, and also means we can delay loading the PlacesDBUtils code in the startup path. I'll file a bug.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

I filed bug 570387.

Comment 10

[Dietrich Ayala (:dietrich)]

WRT to the download manager - what if we set a pref on shutdown if there are active downloads, so that on startup we can check and see if we need to init the service immediately or if we wait and do it lazily.

Comment 11

[Marco Bonardo [:mak]]

(In reply to comment #10)
> WRT to the download manager - what if we set a pref on shutdown if there are
> active downloads, so that on startup we can check and see if we need to init
> the service immediately or if we wait and do it lazily.

I like this idea, I think all the other stuff (win7 taskbar) could easily be moved inside the download manager init or to browserglue, with a notification fired on download manager init.

Comment 12

[Marco Bonardo [:mak]]

the livemarks service init for now could be delayed more, I don't think waiting idle is fine since after I open the browser I would like to see the latest news without going to have a coffee for 5 minutes. But waiting 10 seconds rather than 5 would be fine, the 5 seconds timeout sometimes is fighting with slow startups.  And actually, we could move this to the PlacesCategoriesStarter component and stop relying on browser. Will file a bug to do this move.