Last Comment Bug 668865 - Store _scheme & _host in _serializeHistoryEntry
: Store _scheme & _host in _serializeHistoryEntry
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 8
Assigned To: Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
:
: Mike de Boer [:mikedeboer]
Mentors:
Depends on: 690992
Blocks: 729071
  Show dependency treegraph
 
Reported: 2011-07-01 11:56 PDT by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2012-02-21 04:17 PST (History)
3 users (show)
gavin.sharp: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v0.1 (3.59 KB, patch)
2011-07-05 17:06 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: review+
Details | Diff | Splinter Review
Patch v1.0 (3.85 KB, patch)
2011-07-25 14:06 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Splinter Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-07-01 11:56:18 PDT
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:foo@domain.com, 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 reading much bugmail, email directly) 2011-07-05 17:06:30 PDT
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).
Comment 2 Dietrich Ayala (:dietrich) 2011-07-13 18:03:33 PDT
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 reading much bugmail, email directly) 2011-07-25 11:16:32 PDT
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 reading much bugmail, email directly) 2011-07-25 14:06:27 PDT
Created attachment 548280 [details] [diff] [review]
Patch v1.0

Updated to what I'll land.
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-28 09:55:30 PDT
http://hg.mozilla.org/mozilla-central/rev/b7fef95c4eaf

Note You need to log in before you can comment on or make changes to this bug.