The default bug view has changed. See this FAQ.

Move sessionStorage functionality to JSM

RESOLVED FIXED in Firefox 15

Status

()

Firefox
Session Restore
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: andreshm)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 611966 [details] [diff] [review]
patch v1

In preparation for bug 669603 I thought it might be beneficial to split this big patch into multiple steps. This patch moves all the sessionStorage functionality out of the session store service into its own JSM. It's still the same synchronous behavior, but cleaned up a bit.
Attachment #611966 - Flags: review?(paul)
(Reporter)

Comment 1

5 years ago
Created attachment 613589 [details] [diff] [review]
patch v2

Fixed a little typo.
Attachment #611966 - Attachment is obsolete: true
Attachment #611966 - Flags: review?(paul)
Attachment #613589 - Flags: review?(paul)
Comment on attachment 611966 [details] [diff] [review]
patch v1

(splinter trashed my in progress review when the attachment became obsolete, so hoping un-obsoleting it bring it back)
Attachment #611966 - Attachment is obsolete: false
Comment on attachment 611966 [details] [diff] [review]
patch v1

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

Looking at this patch because I had already started on it...

I think it's a good start but I have some concerns. These aren't "do what I say concerns" so let me know what you think!

::: browser/components/sessionstore/src/SessionStorage.jsm
@@ +7,5 @@
> +let EXPORTED_SYMBOLS = ["SessionStorage"];
> +
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;

You don't use Cc or Ci, so no need.

@@ +17,5 @@
> +const PRIVACY_ENCRYPTED = 1;
> +const PRIVACY_FULL = 2;
> +
> +XPCOMUtils.defineLazyServiceGetter(this, "gScriptSecurityManager",
> +  "@mozilla.org/scriptsecuritymanager;1", "nsIScriptSecurityManager");

Can we keep this SecuritySvc to match what's in nsSessionStore?

@@ +32,5 @@
> +   * @param aPrivacyLevel
> +   *        The session store privacy level to use
> +   */
> +  serialize: function SessionStorage_serialize(aTabData, aHistory, aDocShell,
> +                                               aPrivacyLevel) {

We could take this opportunity to clean this up and bring it closer to mirroring deserialize.

serialize shouldn't know anything about aTabData, it should just return an object and then in sessionstore we decide what to do with it. If it always returns an object then we can just set & be done with it.

I think we can get away with not passing in history too - it should be available on docshell.sessionHistory (quick test in error console seems to work). Or we could pass the browser into each method here and get history/docshell from it. A little bit of duplication with what's happening in sessionstore, but as we pull more of that out into modules that's ok. We should try to design the most robust API instead of pigeonholing into what sessionstore is currently doing.

@@ +63,5 @@
> +   */
> +  read: function DomStorage_read(aHistory, aDocShell, aPrivacyLevel) {
> +    let data = {};
> +
> +    History.getEntries(aHistory).forEach(function (aEntry) {

I know structurally this is cleaner, but we're now iterating over every entry twice (for every tab... often). I don't have scientific data to back this up, but I'm guessing that isn't good.

@@ +70,5 @@
> +        return;
> +
> +      // Check if we're allowed to store sessionStorage data.
> +      if (aPrivacyLevel == PRIVACY_NONE ||
> +          (aEntry.uri.schemeIs("https") && aPrivacyLevels == PRIVACY_ENCRYPTED))

I don't really like moving the privacy level checks around like this :/
It works though, but moves the logic around - if we change the rules for _checkPrivacyLevel, we need to make sure that is updated too.
Can you put a nice big XXX comment here and at _checkPrivacyLevel making it clear how it's used outside of there. The other option would be passing SSS in to here so we could duplicate _checkPrivacyLevel, but that's not great either

Also, aPrivacyLevel (no s!) if we do this (I'm guessing this was the typo fix)

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +160,5 @@
>    "@mozilla.org/scriptsecuritymanager;1", "nsIScriptSecurityManager");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "SessionStorage",
> +  "resource:///modules/sessionstore/SessionStorage.jsm");
> +

Nit: move this up with the other lazy module imports.

@@ +1872,5 @@
> +        if (!this._checkPrivacyLevel(true, aTab.pinned))
> +          privacyLevel = PRIVACY_ENCRYPTED;
> +        else if (!this._checkPrivacyLevel(false, aTab.pinned))
> +          privacyLevel = PRIVACY_NONE;
> +      }

Uhh, so this... privacy level should be determined for each entry, not a once & done thing.

(oh I see...)
Attachment #611966 - Flags: feedback+
Attachment #613589 - Flags: review?(paul)
(Assignee)

Updated

5 years ago
Assignee: ttaubert → andres
(Assignee)

Comment 4

5 years ago
Created attachment 617033 [details] [diff] [review]
patch v3

Added Paul suggestions. 

Also, I changed it to use a callback to check privacy. When Bug 745040 is ready, we can update it to call the module directly.
Attachment #617033 - Flags: review?(paul)
(Assignee)

Updated

5 years ago
Depends on: 708488
(Assignee)

Comment 5

5 years ago
Created attachment 622562 [details] [diff] [review]
Updated patch
Attachment #617033 - Attachment is obsolete: true
Attachment #617033 - Flags: review?(paul)
Attachment #622562 - Flags: review?(ttaubert)
(Assignee)

Comment 6

5 years ago
Created attachment 624239 [details] [diff] [review]
Updated patch
Attachment #622562 - Attachment is obsolete: true
Attachment #622562 - Flags: review?(ttaubert)
Attachment #624239 - Flags: review?(ttaubert)
(Reporter)

Updated

5 years ago
Blocks: 708488
No longer depends on: 708488
(Reporter)

Updated

5 years ago
Depends on: 745040
(Reporter)

Comment 7

5 years ago
Created attachment 627209 [details] [diff] [review]
Part 1 - create SessionStorage.jsm and migrate existing code

[Taking over while Andres is out for a couple of weeks.]

We should put bug 708488 on hold for a moment because it's really hard to refactor all the TabState code if it's all over the place.

As a first step we should just create a SessionStorage.jsm, move _serializeSessionStorage() and _deserializeSessionStorage() to their new place and make those internal methods accessible that we need.

The second step, a follow-up bug, should be refactoring the whole code and the API to prepare making all this async.
Attachment #611966 - Attachment is obsolete: true
Attachment #613589 - Attachment is obsolete: true
Attachment #624239 - Attachment is obsolete: true
Attachment #624239 - Flags: review?(ttaubert)
Attachment #627209 - Flags: review?(paul)
Comment on attachment 627209 [details] [diff] [review]
Part 1 - create SessionStorage.jsm and migrate existing code

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

(In reply to Tim Taubert [:ttaubert] from comment #7)
> As a first step we should just create a SessionStorage.jsm, move
> _serializeSessionStorage() and _deserializeSessionStorage() to their new
> place and make those internal methods accessible that we need.
> 
> The second step, a follow-up bug, should be refactoring the whole code and
> the API to prepare making all this async.

Ok, so you're thinking we just move wholesale & have an unadvertised SessionStorage API that we'll change later? If possible, I'd prefer to have all of that happen within a single version (much like the format changes) so we don't churn too much. SessionStorage.jsm would hopefully only be used by us, but there aren't any guarantees...

That's fine by me if you'll coordinate to make that happen sooner rather than later (and keep my comments & Andres's changes in mind).
Attachment #627209 - Flags: review?(paul) → review+
(Reporter)

Updated

5 years ago
Attachment #627209 - Attachment description: patch v4 → Part 1 - create SessionStorage.jsm and migrate existing code
(Reporter)

Comment 9

5 years ago
(In reply to Paul O'Shannessy [:zpao] from comment #8)
> Ok, so you're thinking we just move wholesale & have an unadvertised
> SessionStorage API that we'll change later? If possible, I'd prefer to have
> all of that happen within a single version (much like the format changes) so
> we don't churn too much. SessionStorage.jsm would hopefully only be used by
> us, but there aren't any guarantees...

I agree, let's make it all happen in this bug.
(Reporter)

Comment 10

5 years ago
Created attachment 628298 [details] [diff] [review]
Part 2 - Refactor SessionStorage.jsm and its API

This patch basically contains all the changes from attachment 624239 [details] [diff] [review]. I simplified the API even more and did a little cleanup.
Attachment #628298 - Flags: review?(paul)
(Reporter)

Comment 11

5 years ago
Created attachment 628346 [details] [diff] [review]
Part 2 - Refactor SessionStorage.jsm and its API

Small cleanup.
Attachment #628298 - Attachment is obsolete: true
Attachment #628298 - Flags: review?(paul)
Attachment #628346 - Flags: review?(paul)
(Reporter)

Comment 12

5 years ago
Looks good on try, btw.

https://tbpl.mozilla.org/?tree=Try&rev=af5bde28b750
Comment on attachment 628346 [details] [diff] [review]
Part 2 - Refactor SessionStorage.jsm and its API

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

::: browser/components/sessionstore/src/SessionStorage.jsm
@@ +143,5 @@
> +   *        That tab's session history
> +   * @param aIndex
> +   *        The history entry's index
> +   */
> +  getUriForEntry: function History_getUriForEntry(aHistory, aIndex) {

super nit: I like URI all caps in there but I'm fine either way.
Attachment #628346 - Flags: review?(paul) → review+
(Reporter)

Comment 14

5 years ago
(In reply to Paul O'Shannessy [:zpao] from comment #13)
> > +  getUriForEntry: function History_getUriForEntry(aHistory, aIndex) {
> 
> super nit: I like URI all caps in there but I'm fine either way.

I do, too. Will fix.
(Reporter)

Comment 15

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/ebd52606c461
https://hg.mozilla.org/integration/fx-team/rev/329aa567fec9
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 15
https://hg.mozilla.org/mozilla-central/rev/ebd52606c461
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/329aa567fec9
You need to log in before you can comment on or make changes to this bug.