403885 - handler service instantiation recurses on fresh profile

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect, P2)

Description

[Myk Melez [:myk] [@mykmelez]]

Starting with a fresh profile and then doing something to prompt the initialization of the handler service (f.e. trying to load the URL "foo:bar") causes the HandlerService constructor in nsHandlerService.js to get called ~117 times, chewing up a bunch of time and causing handler service initialization to be the most expensive operation by an order of magnitude in a Tp test run that first creates a profile and then loads a few hundred pages:

Inclusive function elapsed times (us),
   FILE                 TYPE       NAME                                TOTAL
...
   browser.js           func       onLocationChange                 27121365
   nsSessionStore.js    func       sss_serializeHistoryEntry        27296999
   nsSessionStore.js    func       sss_getCurrentState              28275317
   nsSessionStore.js    func       sss_saveState                    29002207
   hp.js@16             func       scanTreeForChangeUrls            39065753
   nsHandlerService.js  func       getProtocolHandlerInfo           672815734
   nsHandlerService.js  func       HS__injectNewDefaults            678839771
   nsHandlerService.js  func       HS__updateDB                     680224555
   nsHandlerService.js  func       HS__init                         680650173

I'm guessing the culprit is the nsIExternalProtocolService::getProtocolHandlerInfo call in injectNewDefaults, which calls nsExternalHelperAppService::GetProtocolHandlerInfo, which retrieves the handler service in order to call its FillHandlerInfo method.

Subsequent runs don't have this problem, as they don't prompt injectNewDefaults, so this only affects the first run with a new profile.  Nevertheless, it seems pretty serious, given that it is a significant cost that affects the first-run experience for new users, so marking as major severity and requesting blocking-1.9.

I'm also marking this as affecting all platforms, although I've only tested on Mac OS X so far, because I expect this to happen the same way across platforms.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

I've just confirmed that it happens on Linux as well.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

This may not be as severe as I thought at first, given that it doesn't seem to happen on startup but rather the first time the handler service is retrieved.  Nevetheless, it seems like it's still worth fixing for Firefox 3.

Comment 3

[Damon Sicore (:damons)]

-'ing.  If this can be shown to have a more significant impact, please renom.

Comment 4

[Florian Quèze [:florian]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007112118 Minefield/3.0b2pre

On Linux (Ubuntu 7.10) when I start with a new profile, I get this error if I try to load foo:bar or open the pref dialog on the Applications tab:

Error: too much recursion
Source File: file:///home/florian/moz/mozilla/obj-i686-pc-linux-gnu/dist/bin/components/nsHandlerService.js
Line: 119

And every time I open the Applications tab, I get:
Error: uncaught exception: [Exception... "Component returned failure code: 0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE) [nsIJSCID.getService]"  nsresult: "0x80570016 (NS_ERROR_XPC_GS_RETURNED_FAILURE)"  location: "JS frame :: chrome://browser/content/preferences/applications.js :: <TOP_LEVEL> :: line 442"  data: no]

Error: gApplicationsPane is undefined
Source File: chrome://global/content/bindings/preferences.xml
Line: 670

The Applications tab is empty.

Comment 5

[Florian Quèze [:florian]]

I tried again with a new nightly: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b2pre) Gecko/2007113004 Minefield/3.0b2pre

I still get this error:
Error: too much recursion
Source File: file:///home/florian/Desktop/firefox/components/nsHandlerService.js
Line: 119

But I don't have the other errors any more.

The Applications tab is not completely broken but it seems like some rows are missing.  I only have "webcal" and "Web Feed".  I would expect to see "mailto" and maybe others.

Renominating for blocking because this issue has a visible effect.

Comment 6

[Florian Quèze [:florian]]

Hmm... Apparently there's no mailto handler in the default list of handlers added the first time the service is started so the bug may not be visible after all.

It doesn't seem hard to fix so I still think we should fix it for Firefox 3.

Comment 7

[Florian Quèze [:florian]]

Attachment: patch v1

This will ensure _injectNewDefaults is called only once. However there is still a recursion and _init is called twice.
I thought we could prevent this by calling _updateDB in a setTimeout from _init but in this case the first time the service is used it would not have the new default values yet. Don't know if this would be acceptable.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 291270 [details] [diff] [review]
patch v1

+        this._datastoreDefaultHandlersVersion =
         this._prefsDefaultHandlersVersion;

can you put these on the same line, or if that gets too long at least indent the second line?

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 291270 [details] [diff] [review]
patch v1

Looks good; r=dmose.

Comment 10

[Florian Quèze [:florian]]

Attachment: patch v2 (ready for checkin)

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Checked in.