Store _scheme & _host in _serializeHistoryEntry




Session Restore
6 years ago
6 years ago


(Reporter: zpao, Assigned: zpao)


Firefox 8
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)



(2 attachments)

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, 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.
Created attachment 544104 [details] [diff] [review]
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).
Assignee: nobody → paul
Attachment #544104 - Flags: review?(dietrich)
Comment on attachment 544104 [details] [diff] [review]
Patch v0.1

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


::: 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?
Attachment #544104 - Flags: review?(dietrich) → review+
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
Created attachment 548280 [details] [diff] [review]
Patch v1.0

Updated to what I'll land.
Whiteboard: [good first bug][mentor=zpao] → [fixed-in-fx-team]
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8
Flags: in-testsuite?
Depends on: 690992


6 years ago
Blocks: 729071
You need to log in before you can comment on or make changes to this bug.