530207 - Use lazy getters for services in sessionstore

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

We end up getting a lot of services numerous times throughout sessionstore that we could do lazily and have memoized.

This would essentially be what Dietrich did in attachment #407626 [details] [diff] [review].

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1 (WIP)

Dietrich - I think part of the problem you had before might have been related to the crash reporter not being defined. defineLazyServiceGetter doesn't actually check if the component/interface exists before doing it's thing. I think you also missed changing a local service use before.

So I used #ifdef around that lazy getter, and changed _updateCrashReportURL to use an #ifdef as well instead of changing the function at first use. I could do that in a different bug if desired.

There are a couple services I didn't get lazily:
* nsIPrefService (used once at startup to create pref branch)
* nsISessionStartup (used once at startup)
* nsIBrowserHandler (I should actually do this one, it can get used multiple times - when restoring the not actually quit case)
* nsIXULRuntime (used once at startup)

I might as well just make them all use lazy getters. And then defineLazyGetter for NetUtil as well instead of __define___. I'll post a new version with those changes (and sort the services by variable name).

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

I realized I should probably do this in SessionStartup while I'm here, so I'll do that in the next rev.

Anybody have thoughts on naming conventions (eg. PrefSvc vs gPrefSvc vs ...)? I just stuck with what Dietrich had done previously. I don't really have any strong feelings about it, but maybe others do.

Comment 3

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.2

Comment 4

[Simon Bünzli]

Comment on attachment 415956 [details] [diff] [review]
Patch v0.2

Do we really want to do this for services we use exactly once (CLHSvc, PBSvc, DirectorySvc, SessionStartupSvc, XULRuntime)? IMO these only clutter the code and eat memory (keeping a reference we're not going to use again) and thus aren't worthy.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #4)
> (From update of attachment 415956 [details] [diff] [review])
> Do we really want to do this for services we use exactly once (CLHSvc, PBSvc,
> DirectorySvc, SessionStartupSvc, XULRuntime)? IMO these only clutter the code
> and eat memory (keeping a reference we're not going to use again) and thus
> aren't worthy.

That would probably eat memory but I would argue they make the code cleaner. Memory use is more important though, so I'll undo the one time uses.

Comment 6

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.3

Now with less lazy.

Comment 7

[Simon Bünzli]

Comment on attachment 416604 [details] [diff] [review]
Patch v0.3

>-        cookieManager.add(cookie.host, cookie.path || "", cookie.name || "", cookie.value, !!cookie.secure, !!cookie.httponly, true, "expiry" in cookie ? cookie.expiry : MAX_EXPIRY);
>+        CookieSvc.add(cookie.host, cookie.path || "", cookie.name || "", cookie.value, !!cookie.secure, !!cookie.httponly, true, "expiry" in cookie ? cookie.expiry : MAX_EXPIRY);

Touch it - own it: Please break this line at at most 80 characters.

>-    this._observerService.notifyObservers(stateString,
>+    ObserverSvc.notifyObservers(stateString,
>                                           "sessionstore-state-write", "");

Nit: This should fit on one line (else, the second parameter must be realigned).

>-    var windowsEnum = wm.getZOrderDOMWindowEnumerator("navigator:browser", true);
>+    var windowsEnum = WindowMediator.getZOrderDOMWindowEnumerator("navigator:browser", true);

Nit: Overlong line.

>+    var window = WindowWatcher.openWindow(
>+                   null, this._prefBranch.getCharPref("chromeURL"), "_blank",
>+                   features, argString);

Nit: Please align the parameters more reasonable, maybe something like

>+    var window =
>+      WindowWatcher.openWindow(null,
>+                               this._prefBranch.getCharPref("chromeURL"),
>+                               "_blank", features, argString);

r+=me with these fixed.

Comment 8

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.4 (for check-in)

Nits addressed. FYI, this is on top of my patch for bug 529674, so there's one bit that won't apply cleanly. I'll either reorder before checking in, or maybe by the time the tree reopens, I'll have that r+ed too.

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.5 (updated; for check-in)

Updated to tip so it will apply cleanly & took out CLHSvc (which I thought I'd done already) - it's only used once in the usual case.

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/fe82d5826318

Comment 11

[u279076]

What can QA do to verify this bug?

Comment 12

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #11)
> What can QA do to verify this bug?

The fact that tests pass (and there have been no bugs saying "session restore broke") should be good enough.

Comment 13

[u279076]

(In reply to comment #12)
> (In reply to comment #11)
> > What can QA do to verify this bug?
> 
> The fact that tests pass (and there have been no bugs saying "session restore
> broke") should be good enough.

Good enough!  Marking verified.

Comment 14

[[e-mail]]

Comment on attachment 417585 [details] [diff] [review]
Patch v0.5 (updated; for check-in)

>+XPCOMUtils.defineLazyServiceGetter(this, "CookieSvc",
>+  "@mozilla.org/cookiemanager;1", "nsICookieManager2");
The cookie manager tends to get shortened to cm or cs (oddly the latter can also mean nsICookieService; maybe they used to be one interface); people not abbreviating name the variable consistently after the interface.

>+XPCOMUtils.defineLazyServiceGetter(this, "SecuritySvc",
>+  "@mozilla.org/scriptsecuritymanager;1", "nsIScriptSecurityManager");
This one probably belongs on Services, but everyone else calls it SecMan.