ExtensionService: Delay non-critical IO until after startup has finished.

ExtensionService: Delay non-critical IO until after startup has finished.

Delay the calls to CheckForExternalUpdates() and GarbageCollectExtensions() until onload fires for the first frame to reduce IO at startup.

For newly created profiles we don't delay CheckForExternalUpdates().

BUG=136623
TEST=All unit tests should pass, external extension updates should continue to work.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170960

[jeremy, 2012-09-04 14:34:32]

Hi Antony,

This CL implements the change we discussed.  CheckForExternalUpdates() will
notably be called twice for newly created profiles, is that an issue?

Thanks!,
Jeremy

[asargent_no_longer_on_chrome, 2012-09-04 16:45:42]

Overall approach seems fine, so LGTM. I don't think it's a huge deal to call
CheckForExternalUpdates twice on new profiles, but it would be nice to avoid it.
Maybe just add a boolean flag member variable like
|checked_for_external_updates_| or something? Alternatively, could you just
perform the same "is this a new profile?" check?

Looks like you might need to arrange to have that notification sent to fix a
couple of tests that likely are expecting the current behavior. 

Also nit on CL description - you say "...until unload fires..." but it's clear
from the code changes you mean "...until *onload* fires...".

[asargent_no_longer_on_chrome, 2012-09-04 16:48:40]

Sorry, I just thought of a potential problem with the approach: external
extensions can often have their source .crx files somewhere in the local
filesystem, and we probably want to install them relatively quickly even if
there is no network connection. In the case where the network is unavailable,
will we not get an onload event firing if the set of startup pages are all
internet resources that get connection errors? Or do the error pages generate
that onload notification?

[jeremy, 2012-09-05 08:33:05]

Yes, I've just double-checked, the error pages do indeed trigger
NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME .

[asargent_no_longer_on_chrome, 2012-09-05 17:09:05]

On 2012/09/05 08:33:05, jeremy wrote:
> Yes, I've just double-checked, the error pages do indeed trigger
> NOTIFICATION_LOAD_COMPLETED_MAIN_FRAME .

Ok, I suppose then the only minor issue is that if all the startup pages
happened to be very slow to load and none of them completed in a resonable time
frame, then external extension update checks would be delayed. That's probably
not a big deal though, so I don't know that it's worth the complexity of doing
something like setting up a timer to alleviate this (and indeed, if a more
generic "run non-essential startup tasks after some delay" mechanism is
implemented then we'd just want to hook into that anyway).

[jeremy, 2012-09-06 09:03:49]

Thanks! All review comments addressed, all unit tests pass.

[asargent_no_longer_on_chrome, 2012-09-06 22:58:15]

LGTM

[commit-bot: I haz the power, 2012-09-08 20:02:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/10914076/1002

[commit-bot: I haz the power, 2012-09-08 22:05:10]

Try job failure for 10914076-1002 (retry) on mac_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-09 08:02:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/10914076/1002

[commit-bot: I haz the power, 2012-09-09 11:18:39]

Try job failure for 10914076-1002 (retry) (retry) on linux_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-11-13 15:08:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/10914076/25001

[commit-bot: I haz the power, 2012-11-13 16:44:52]

Retried try job too often for step(s) sync_integration_tests

[akalin, 2012-11-14 20:14:18]

I'll be trying to repro the integration test failure.

[akalin, 2012-11-14 23:07:47]

On 2012/11/14 20:14:18, akalin wrote:
> I'll be trying to repro the integration test failure.

Hunh, still can't repro locally.  Tried Debug/Release x Linux/OS X.

I guess I'll see if I can upload my own patch and repro the trybot failures. 
Then I'll add logging

[akalin, 2012-11-15 05:23:12]

On 2012/11/14 23:07:47, akalin wrote:
> On 2012/11/14 20:14:18, akalin wrote:
> > I'll be trying to repro the integration test failure.
> 
> Hunh, still can't repro locally.  Tried Debug/Release x Linux/OS X.
> 
> I guess I'll see if I can upload my own patch and repro the trybot failures. 
> Then I'll add logging

Okay, I found the problem (finally).  I will file a bug.

[akalin, 2012-11-15 05:32:48]

bug: https://code.google.com/p/chromium/issues/detail?id=161203

[akalin, 2012-11-16 18:04:59]

On 2012/11/15 05:32:48, akalin wrote:
> bug: https://code.google.com/p/chromium/issues/detail?id=161203

Landed my patch in r168142 -- see if syncing after that fixes the failures!

[akalin, 2012-11-17 01:25:01]

On 2012/11/16 18:04:59, akalin wrote:
> On 2012/11/15 05:32:48, akalin wrote:
> > bug: https://code.google.com/p/chromium/issues/detail?id=161203
> 
> Landed my patch in r168142 -- see if syncing after that fixes the failures!

Nevermind, I have to roll back that patch. See 161581

[commit-bot: I haz the power, 2012-11-28 19:31:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/10914076/25001

[commit-bot: I haz the power, 2012-11-28 20:23:53]

Retried try job too often on linux_rel for step(s) unit_tests

[commit-bot: I haz the power, 2012-11-28 20:53:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/10914076/25001

[commit-bot: I haz the power, 2012-11-28 21:46:41]

Retried try job too often on linux_chromeos for step(s) unit_tests

[akalin, 2012-11-28 21:56:03]

On 2012/11/28 21:46:41, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_chromeos for step(s) unit_tests

Test failures are ExtensionServiceTest.ExternalInstallGlobalError -- possibly a
legit (but separate) failure?

[jeremy, 2012-12-03 10:49:14]

Antony: I've changed the approach slightly to not fire fake notifications in the
updated test files, could you take another look please?

[asargent_no_longer_on_chrome, 2012-12-03 18:13:47]

LGTM

https://chromiumcodereview.appspot.com/10914076/diff/40003/chrome/browser/ext...
File chrome/browser/extensions/extension_service.h (right):

https://chromiumcodereview.appspot.com/10914076/diff/40003/chrome/browser/ext...
chrome/browser/extensions/extension_service.h:386: void
ResetExternalUpdateCheckGuard() {
optional suggestion: consider adding "ForTests" or something along those lines
to this name

[commit-bot: I haz the power, 2012-12-03 18:45:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/10914076/40003

[commit-bot: I haz the power, 2012-12-03 19:26:55]

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[akalin, 2012-12-03 20:06:26]

On 2012/12/03 19:26:55, I haz the power (commit-bot) wrote:
> Step "update" is always a major failure.
> Look at the try server FAQ for more details.

I hate to do this, but can you hold off on this again?  My 2nd attempt actually
introduced yet another regression (
https://code.google.com/p/chromium/issues/detail?id=163706 ) that's not easy to
fix, so I will try to revert and try again. :/

[akalin, 2012-12-04 06:33:39]

Okay, my third attempt landed as 170880.  This time I think it's sticking. :)

[commit-bot: I haz the power, 2012-12-04 11:16:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jeremy@chromium.org/10914076/32005

[commit-bot: I haz the power, 2012-12-04 15:33:58]

Change committed as 170960