right now there are a bunch of redundant instantiations of observer service, io service, and others. we should instead load them each only once, and lazily at that. eg, something like this: http://hg.mozilla.org/labs/weave/file/42d1239d8262/source/modules/util.js#l274

I tried to catch all services instantiated more than three times.

How about: > [ > ["gPrefService", "preferences-service;1", "nsIPrefBranch2"], > ["gPermissionManager", "permissionmanager;1", "nsIPermissionManager"], > ... > ].forEach(function (service) { > var [name, cid, iface] = service; > window.__defineGetter__(name, function () { > delete window[name]; > return window[name] = Cc["@mozilla.org/" + cid].getService(Ci[iface]); > }); > });

Comment on attachment 390747 [details] [diff] [review] Patch (v1) >+ if (gPermissionManager.testPermission(uri, "popup") == this._kIPM.ALLOW_ACTION) { remove _kIPM from gPopupBlockerObserver, use gPermissionManager instead >+ var rv = gPromptService.confirmEx(window, promptTitle, promptMessage, >+ Ci.nsIPromptService.STD_YES_NO_BUTTONS, >+ null, null, null, checkboxLabel, checkEveryTime); gPromptService instead of Ci.nsIPromptService > } else { >- var ss = Cc["@mozilla.org/browser/search-service;1"]. >- getService(Ci.nsIBrowserSearchService); >- var searchForm = ss.defaultEngine.searchForm; >+ var searchForm = gSearchService.defaultEngine.searchForm; > loadURI(searchForm, null, null, false); > } > }, use let instead of var > try { >- var ios = Cc["@mozilla.org/network/io-service;1"]. >- getService(Ci.nsIIOService); >- >- var contentURI = ios.newURI(aWindow.location.href, null, null); >- return ios.newURI(attr, aWindow.document.characterSet, contentURI); >+ var contentURI = gIOService.newURI(aWindow.location.href, null, null); >+ return gIOService.newURI(attr, aWindow.document.characterSet, contentURI); ditto > function undoCloseWindow(aIndex) { >- let ss = Cc["@mozilla.org/browser/sessionstore;1"]. >- getService(Ci.nsISessionStore); > let window = null; >- if (ss.getClosedWindowCount() > (aIndex || 0)) >- window = ss.undoCloseWindow(aIndex || 0); >+ if (gSessionStoreService.getClosedWindowCount() > (aIndex || 0)) >+ window = gSessionStoreService.undoCloseWindow(aIndex || 0); > > return window; > } please rename the "window" variable to something sane You're using Components.classes and Components.interfaces a bunch of times. Use Cc and Ci instead. Did you take browser-places.js, browser-tabPreviews.js and browser-textZoom.js into account?

I'm also not sure that gFooService is the best naming scheme for this. Maybe FooService without the g is more appropriate, except for gPrefService.

Addressed all review comments.

(In reply to comment #4) > I'm also not sure that gFooService is the best naming scheme for this. Maybe > FooService without the g is more appropriate, except for gPrefService. I think a common prefix for these global accessors is useful (to make them easy to identify as browser.js-scope globals). It may have been useful to distinguish the element references from the service references, but we already have gPrefService as a constraint, so I think I would favor keeping the "g" prefix here.

(In reply to comment #6) > (In reply to comment #4) > > I'm also not sure that gFooService is the best naming scheme for this. Maybe > > FooService without the g is more appropriate, except for gPrefService. > > I think a common prefix for these global accessors is useful (to make them easy > to identify as browser.js-scope globals). It may have been useful to > distinguish the element references from the service references, but we already > have gPrefService as a constraint, so I think I would favor keeping the "g" > prefix here. I'm fine either way, but I tend to lean a bit on the side of having the "g" prefix.

Comment on attachment 391033 [details] [diff] [review] Patch (v2) Fine by me, let's use the g prefix. Nit: No need to replace let with var if you're not in a nested block, especially if you'd be introducing inconsistency because var is used a few lines above.

(In reply to comment #8) > Nit: No need to replace let with var if you're not in a nested block, > especially if you'd be introducing inconsistency because var is used a few > lines above. Quite obviously, I meant "replace var with let"...

Use the "g" prefix, and addressed the nit.

Comment on attachment 391049 [details] [diff] [review] Patch (v3) >+ ["gPrefService", "preferences-service;1", "nsIPrefBranch2"], >+ ["gPermissionManager", "permissionmanager;1", "nsIPermissionManager"], >+ ["gObserverService", "observer-service;1", "nsIObserverService"], >+ ["gWindowMediator", "appshell/window-mediator;1", "nsIWindowMediator"], >+ ["gPromptService", "embedcomp/prompt-service;1", "nsIPromptService"], >+ ["gSearchService", "browser/search-service;1", "nsIBrowserSearchService"], >+ ["gSessionStoreService", "browser/sessionstore;1", "nsISessionStore"], >+ ["gIOService", "network/io-service;1", "nsIIOService2"], Calling gSessionStoreService gSessionStore would be more consistent, I think. Should we use ContentAreaUtils.ioService rather than adding gIOService? r=me either way, with this nit: >+ gPermissionManager.add(aDocument.documentURIObject, "offline-app", >+ Ci.nsIPermissionManager.DENY_ACTION); gPermissionManager.DENY_ACTION

(In reply to comment #11) > (From update of attachment 391049 [details] [diff] [review]) > Should we use ContentAreaUtils.ioService rather than adding gIOService? That is _currently_ nsIIOService - manageOfflineStatus is in nsIIOService2

(In reply to comment #11) > Calling gSessionStoreService gSessionStore would be more consistent, I think. > Should we use ContentAreaUtils.ioService rather than adding gIOService? gIOService is an nsIIOService2. Is it OK to modify ContentAreaUtils.ioService to be an nsIObserver2?

There's no reason not to make it nsIObserver2, at least if it's intended for external use, which seems to be the case.

You both mean nsIIOService2 rather than nsIObserver2, I'm guessing?

(In reply to comment #15) > You both mean nsIIOService2 rather than nsIObserver2, I'm guessing? Yes. /me wishes bugzilla supported comment edits... :-)

Gavin writes: >diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js > // XXX: duplicated in preferences/advanced.js > _getOfflineAppUsage: function (host, groups) We avoided touching this function in the other bug because it is duplicated. We should get a bug on file on fixing that and avoid making this change too, I think.

Comment on attachment 391117 [details] [diff] [review] For check-in >+[ >+ ["gPrefService", "preferences-service;1", "nsIPrefBranch2"], >+].forEach(function (service) { >+ let [name, cid, iface] = service; You can actually do this instead.. :) ["gPrefService", "preferences-service;1", "nsIPrefBranch2"], ].forEach(function ([name, cid, iface]) {

Comment on attachment 391117 [details] [diff] [review] For check-in > // don't bother showing UI if the user has already made a decision >- if (pm.testExactPermission(currentURI, "offline-app") != >+ if (gPermissionManager.testExactPermission(currentURI, "offline-app") != > Ci.nsIPermissionManager.UNKNOWN_ACTION) >+ gPermissionManager.add(aDocument.documentURIObject, "offline-app", >+ Ci.nsIPermissionManager.ALLOW_ACTION); I replaced Ci.nsIPermissionManager with gPermissionManager.

Following Mardak's suggestion...

Comment on attachment 391117 [details] [diff] [review] For check-in > function getMeOutOfHere() { > // Get the start page from the *default* pref branch, not the user's >- var prefs = Cc["@mozilla.org/preferences-service;1"] >- .getService(Ci.nsIPrefService).getDefaultBranch(null); > var url = "about:blank"; > try { >- url = prefs.getComplexValue("browser.startup.homepage", >- Ci.nsIPrefLocalizedString).data; >+ url = gPrefService.getComplexValue("browser.startup.homepage", >+ Ci.nsIPrefLocalizedString).data; That doesn't look right to me - wrong branch.

Backed out, as this caused or triggered bug 507092 somehow. (In reply to comment #23) Which branch?

This is what got landed and backed out.

(In reply to comment #24) > (In reply to comment #23) > Which branch? No longer the default branch: >- .getService(Ci.nsIPrefService).getDefaultBranch(null);

> nsIPrefBranch nsIPrefService::getDefaultBranch( > in string aPrefRoot) > [...] aPrefRoot: [...] nsnull or "" may be used to access to the entire > preference "tree". This is what gPrefService does by default.

(In reply to comment #27) > > nsIPrefBranch nsIPrefService::getDefaultBranch( > > in string aPrefRoot) > > [...] aPrefRoot: [...] nsnull or "" may be used to access to the entire > > preference "tree". > > This is what gPrefService does by default. Sorry I seem to be getting the terminology wrong! Presumably I meant wrong *tree* - this code wants the value from the default preferences not from the users preferences. > When using a Get method a default value will always be returned.

I see. So gPrefService can't be used there.

(In reply to comment #29) > I see. So gPrefService can't be used there. + url = gPrefService.getDefaultBranch(null).getComplexValue("browser.startup.homepage", should be OK.

(In reply to comment #30) > + url = > gPrefService.getDefaultBranch(null).getComplexValue("browser.startup.homepage", > > should be OK. Now I need an edit a previous comment. gPrefService is nsIPrefBranch2 not the nsIPrefService that its name suggests.

gPrefService.getDefaultBranch(null) would depend on other consumers, but gPrefService.QueryInterface(Ci.nsIPrefService).getDefaultBranch(null) should work reliably.

gPrefService should just also be an nsIPrefService, IMO.

Ehsan, any idea what was going on with bug 507092? Should we try to re-land this piece by piece?

This patch makes us keep longer-living references to these services... perhaps we should clear them onunload? I suppose that might break stuff, though.

(In reply to comment #35) > I suppose that might break stuff, though. We could re-define the getters. ;)

(In reply to comment #34) > Ehsan, any idea what was going on with bug 507092? Should we try to re-land > this piece by piece? I've read this patch over and over, and I couldn't think of anything which might potentially go wrong. And since this is OS X only, I can't even debug it to figure out which service is causing the assertion. Regarding your getter patch, I think this may solve the problem, and I doubt if that would break other things, as nothing should be run in the context of a browser window after BrowserShutdown, right? Have you tested this patch on the try server? I'm going to do that now...

I forgot to mention that if the new patch doesn't help, I'm in favor of landing this piece by piece and see which part is the culprit.

(In reply to comment #39) > Regarding your getter patch, I think this may solve the problem, and I doubt if > that would break other things, as nothing should be run in the context of a > browser window after BrowserShutdown, right? There could be more unload handlers for instance. But they would still be supported with my patch, as it re-defines the getters rather than just deleting the properties.

(In reply to comment #39) > Have you tested this patch on the try server? I'm going to do that now... Ah, forget it. I just remembered that try server does not run leak tests (which was failing after this check-in.) Can you test it locally please?

I don't run OS X.

In that case, let's wait for Gavin's review and try to land the new getters alongside the old patch and see if the problem persists.

PING!

Comment on attachment 391667 [details] [diff] [review] like this... on top of the previous patches Hmm, I forgot about this bug before starting work on bug 512784 :( The patch in that bug largely overlaps with the patch in this one, I think, which is unfortunate. I think I prefer that bug's patch because it is slightly more generic, though. This followup makes sense, and we can apply it to the work in that bug as well (by calling Services.release() in BrowserShutdown()).