903398 - [Session Restore] Use a 'cookie-changed' observer instead of asking the cookie service for every host

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Currently, we build a list of all hosts found in session histories and then we go and ask the cookie service for cookies for every host. This seems silly and inefficient.

We should instead use the "cookie-changed" observer notification and keep a live map of session cookies. That should be a little faster.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Use a 'cookie-changed' observer instead of asking the cookie service for every host

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 788126 [details] [diff] [review]
Use a 'cookie-changed' observer instead of asking the cookie service for every host

Review of attachment 788126 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/SessionCookies.jsm
@@ +48,5 @@
> +  observe: function (subject, topic, data) {
> +    switch (data) {
> +      case "added":
> +      case "changed":
> +        this._updateCookie(subject.QueryInterface(Ci.nsICookie2));

Nit: I would be less nervous if the QueryInterface took place inside |_updateCookie|, |_removeCookie|, etc.

@@ +66,5 @@
> +        CookieStore.clear();
> +        this._reloadCookies();
> +        break;
> +      default:
> +        throw new Error("Unhandled cookie-changed notification.");

And now, this should even be visible somewhere :)

@@ +117,5 @@
> + * The internal cookie storage that keeps track of every active session cookie.
> + * These are stored using maps per host, path, and cookie name.
> + */
> +let CookieStore = {
> +  _hosts: new Map(),

Do I understand correctly that it maps
 (host) string -> (path) string -> (name) string -> cookie
?

This deserves a better documentation.

@@ +120,5 @@
> +let CookieStore = {
> +  _hosts: new Map(),
> +
> +  /**
> +   * Returns the list of stored session cookies for a given host.

Could you add type information to the various arguments used in these methods?
Also, given that I'm a nervous guy, I'd add dynamic type checks.

@@ +129,5 @@
> +    }
> +
> +    let cookies = [];
> +
> +    for (let path of this._hosts.get(host).values()) {

Nit: Not really a |path|, is it?

@@ +144,5 @@
> +    let jscookie = {host: cookie.host, value: cookie.value};
> +
> +    // Only add properties with non-default values to save a few bytes.
> +    for (let prop of ["path", "name", "isSecure", "isHttpOnly"]) {
> +      if (prop in cookie && cookie[prop]) {

Can we have values of |cookie| for which |prop in cookie| is false?

@@ +145,5 @@
> +
> +    // Only add properties with non-default values to save a few bytes.
> +    for (let prop of ["path", "name", "isSecure", "isHttpOnly"]) {
> +      if (prop in cookie && cookie[prop]) {
> +        jscookie[prop.replace(/^is/, "").toLowerCase()] = cookie[prop];

I believe that it would be more readable with a simple switch/case.

@@ +184,5 @@
> +      if (!map.has(key)) {
> +        map.set(key, new Map());
> +      }
> +
> +      return submap(map.get(key), keys.slice(1));

That line (possibly that whole method) looks too intelligent for me. Could you make sure I understand it even when lacking sleep?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #3)
> Do I understand correctly that it maps
>  (host) string -> (path) string -> (name) string -> cookie
> ?
> 
> This deserves a better documentation.

You're right, will do.

> > +let CookieStore = {
> > +  _hosts: new Map(),
> > +
> > +  /**
> > +   * Returns the list of stored session cookies for a given host.
> 
> Could you add type information to the various arguments used in these
> methods?

Sure.

> Also, given that I'm a nervous guy, I'd add dynamic type checks.

I don't really think we need type checks here. The CookieStore is only access by the SessionCookies object whose methods all do QueryInterace() that would throw if someone passes an invalid object.

> > +    let jscookie = {host: cookie.host, value: cookie.value};
> > +
> > +    // Only add properties with non-default values to save a few bytes.
> > +    for (let prop of ["path", "name", "isSecure", "isHttpOnly"]) {
> > +      if (prop in cookie && cookie[prop]) {
> 
> Can we have values of |cookie| for which |prop in cookie| is false?

No, good catch. Not sure why I did that.

> > +    // Only add properties with non-default values to save a few bytes.
> > +    for (let prop of ["path", "name", "isSecure", "isHttpOnly"]) {
> > +      if (prop in cookie && cookie[prop]) {
> > +        jscookie[prop.replace(/^is/, "").toLowerCase()] = cookie[prop];
> 
> I believe that it would be more readable with a simple switch/case.

Yeah, I can make that simpler.

> > +      if (!map.has(key)) {
> > +        map.set(key, new Map());
> > +      }
> > +
> > +      return submap(map.get(key), keys.slice(1));
> 
> That line (possibly that whole method) looks too intelligent for me. Could
> you make sure I understand it even when lacking sleep?

No love for some functional style? Ok :'(

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Use a 'cookie-changed' observer instead of asking the cookie service for every host, v2

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Use a 'cookie-changed' observer instead of asking the cookie service for every host, v3

The subject when we're notified about "batch-deleted" is an nsIArray, not an iterator. Oops.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 788872 [details] [diff] [review]
Use a 'cookie-changed' observer instead of asking the cookie service for every host, v3

Review of attachment 788872 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/SessionCookies.jsm
@@ +150,5 @@
> +   *       "cookiename": {name: "cookiename", value: "value", etc...}
> +   *     }
> +   *   }
> +   * };
> +   */

Much, much better, thanks.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/4a4cef561232

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4a4cef561232