668865 - Store _scheme & _host in _serializeHistoryEntry

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

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

Session restore currently does this:
1. iterates over each entry in shistory and serializes some parts of it
  1a. namely it grabs the entry.URI.spec (and other stuff)
2. iterates over each serialized entry to determine if we should save the cookies for that host
  2a. uses a regex to determine if it's https?: vs file:
  2b. if https?:, then creates nsIURI to figure out if it's http or https (WHHHHYYY)

I need to look closer to see what exactly the regex is doing with user:[e-mail], but we can grab those bits too.

Using uri.schemeIs("https") is apparently an optimization, but if it means creating a ton of uris, then it doesn't really seem like anything is being optimized.

Comment 1

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

Attachment: Patch v0.1

I think this should be fine. I just pushed to try in case it blows up but limited testing locally seemed ok. Logging into Gmail produced the same set of cookies and other host names were the same.

I'm a bit confused by the file:// matching, since that host ends up being empty, and match[1] in the previous code was empty too (though that doesn't really make much sense does it). I'll triple check that...

This will increase the amount of memory we use to track each tab, but I think it's probably worth it to cut down on the constant object creation & GC we have to do currently.

Dietrich: idea I can do now or file a followup bug... there is an increasing amount of data we're storing that we can wipe out for closed tabs & windows. We don't right now and that's fine - it doesn't end up on disk - but we can clear it out and gain a little bit of memory back (_host & _scheme in this case).

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 544104 [details] [diff] [review]
Patch v0.1

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

r=me

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +2165,5 @@
>     */
>    _extractHostsForCookies:
>      function sss__extractHostsForCookies(aEntry, aHosts, aCheckPrivacy, aIsPinned) {
> +
> +    if (aEntry._host && /https?/.test(aEntry._scheme)) {

is the _host test necessary, since you already tested for it when creating the property?

Comment 3

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

I guess it's not strictly necessary, just made it a bit more explicit. about: urls specifically won't have _host set here, but _scheme also won't be set, so would fail the .test check, so that should be just fine. I'll add a comment though

Comment 4

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

Attachment: Patch v1.0

Updated to what I'll land.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/b7fef95c4eaf