903388 - [Session Restore] Collect cookie hosts *after* serializing session history

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Currently, we collect cookies *while* serializing session history. While saves a couple of loop iterations it blocks moving session history serialization to a content process.

Cookies are global and the services will at least for now continue to live in the main process. Therefore we need to collect cookie hosts from the serialized session history.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Collect cookie hosts *after* serializing session history

This does what the summary says. I needed to resist the temptation to rewrite all that code for now as it's a huge mess. I didn't really want to touch the logic in there although it seems a little flawed. The good news is we got rid of the _internalWindows field and the __SS_hostSchemeData property.

Comment 2

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

Comment on attachment 788105 [details] [diff] [review]
Collect cookie hosts *after* serializing session history

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

Looks good. Thanks for yet another simplification.

::: browser/components/sessionstore/src/SessionStore.jsm
@@ -305,5 @@
>    _windows: {},
>  
> -  // internal states for all open windows (data we need to associate,
> -  // but not write to disk)
> -  _internalWindows: {},

Oh, there was such a field?

@@ +2417,5 @@
>      var _this = this;
>      // MAX_EXPIRY should be 2^63-1, but JavaScript can't handle that precision
>      var MAX_EXPIRY = Math.pow(2, 62);
>  
> +    for (let window of aWindows) {

Could you update the documentation of |updateCookies| with the correct type for |aWindows|?

@@ -2448,4 @@
>        window.cookies = [];
> -      let internalWindow = this._internalWindows[id];
> -      if (!internalWindow.hosts)
> -        return;

So what was the point of that? Just caching the list of hosts?

@@ +2425,5 @@
> +      let hosts = {};
> +      window.tabs.forEach(function(tab) {
> +        tab.entries.forEach(function(entry) {
> +          this._extractHostsForCookiesFromEntry(entry, hosts, true, tab.pinned);
> +        }, this);

Note for future version: we might wish to store some of |hosts| in metadata attached to the tabstate cache.

@@ +2522,5 @@
>        DirtyWindows.clear();
>      }
>  
>      // collect the data for all windows
> +    var total = [], ids = [];

Could you take the opportunity to document |total| and |ids|?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Could you update the documentation of |updateCookies| with the correct type
> for |aWindows|?

Oh sure. Forgot about that.

> > -      let internalWindow = this._internalWindows[id];
> > -      if (!internalWindow.hosts)
> > -        return;
> 
> So what was the point of that? Just caching the list of hosts?

Yeah that was caching the list of hosts to possibly collect cookies for. So actually the list of all hosts of the window. This patch removes some of the caching but I'm not really sure how effective it was. We should definitely look into this more in follow-up bugs.

> > +      let hosts = {};
> > +      window.tabs.forEach(function(tab) {
> > +        tab.entries.forEach(function(entry) {
> > +          this._extractHostsForCookiesFromEntry(entry, hosts, true, tab.pinned);
> > +        }, this);
> 
> Note for future version: we might wish to store some of |hosts| in metadata
> attached to the tabstate cache.

Yes. With bug 903398 implemented we can be even more intelligent when it comes to caching and updating our internal data structures.

> >      // collect the data for all windows
> > +    var total = [], ids = [];
> 
> Could you take the opportunity to document |total| and |ids|?

Sure.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/df3cf81576cc