Closed Bug 1322316 Opened 3 years ago Closed 3 years ago

Considering split localStorage and sessionStorage implementations

Categories

(Core :: DOM: Core & HTML, defect)

50 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: baku, Assigned: baku)

Details

Attachments

(11 files, 5 obsolete files)

23.62 KB, patch
asuth
: review+
Details | Diff | Splinter Review
48.86 KB, patch
asuth
: review+
Details | Diff | Splinter Review
6.35 KB, patch
asuth
: review+
Details | Diff | Splinter Review
5.76 KB, patch
asuth
: review+
Details | Diff | Splinter Review
70.92 KB, patch
asuth
: review+
Details | Diff | Splinter Review
52.54 KB, patch
asuth
: review+
Details | Diff | Splinter Review
21.39 KB, patch
asuth
: review+
Details | Diff | Splinter Review
21.41 KB, patch
asuth
: review+
Details | Diff | Splinter Review
17.31 KB, patch
asuth
: review+
Details | Diff | Splinter Review
10.83 KB, patch
asuth
: review+
Details | Diff | Splinter Review
16.56 KB, patch
asuth
: review+
Details | Diff | Splinter Review
Soon LocalStorage will be ported to QuotaManager. This will dramatically change the implementation.
What I suggest is to split localStorage and sessionStorage. SesstionStorage doesn't need any complex cache, nor IPC actors.

I would like to split sessionStorage in a simple and clean implementation. After this, localStorage can be ported to QuotaManager.
Flags: needinfo?(jvarga)
Just curious, would it be still simpler to use the same implementation for localStorage and sessionStorage, but just different storage policy.
Or have some generic implementation for Storage interface implementation but different storage backends.
SessionStorage and LocalStorage are both 'Storage' interface. This means that we must have one virtual base Storage class, but then 2 different implementations. Or, if you prefer, 2 different backends.
What I wanted to point out with this bug, is that I would like to get rid of the Storage cache object for SessionStorage and split the logic between localStorge and SessionStorage almost immediately in our code base.
and I was just curious whether it keeps the codebase simpler to have as similar implementations for localStorage and sessionStorage as possible.
LocalStorage will be probably rewritten from scratch when ported to QuotaManager.
The cache will be moved to the parent process. We don't want that for sessionStorage.
Shawn, do you want to take this bug?
Flags: needinfo?(jvarga) → needinfo?(shuang)
Assignee: amarchesini → shuang
Flags: needinfo?(shuang)
Can I take this?
Flags: needinfo?(shuang)
Assignee: shuang → amarchesini
Flags: needinfo?(shuang)
Attached patch part 1 - SessionStorage (obsolete) — Splinter Review
Attachment #8867704 - Flags: review?(bugmail)
Attached patch part 3 - SessionStorageManager (obsolete) — Splinter Review
Attachment #8867706 - Flags: review?(bugmail)
Attachment #8867708 - Flags: review?(bugmail)
Attachment #8867709 - Flags: review?(bugmail)
I would like Janv to give a +1/-1 on this approach. Note that there is some duplicated code in SessionStorageCache/LocalStorageCache but this is wanted: LocalStorage is going to be rewritten from scratch.
Flags: needinfo?(jvarga)
Attachment #8867704 - Attachment is obsolete: true
Attachment #8867704 - Flags: review?(bugmail)
Attachment #8867774 - Flags: review?(bugmail)
Attached patch part 3 - SessionStorageManager (obsolete) — Splinter Review
Attachment #8867706 - Attachment is obsolete: true
Attachment #8867706 - Flags: review?(bugmail)
Attachment #8867775 - Flags: review?(bugmail)
Attached patch part 3 - SessionStorageManager (obsolete) — Splinter Review
Wrong patch
Attachment #8867775 - Attachment is obsolete: true
Attachment #8867775 - Flags: review?(bugmail)
Attachment #8867776 - Flags: review?(bugmail)
Attachment #8867708 - Attachment is obsolete: true
Attachment #8867708 - Flags: review?(bugmail)
Attachment #8867777 - Flags: review?(bugmail)
Attachment #8867776 - Attachment is obsolete: true
Attachment #8867776 - Flags: review?(bugmail)
Attachment #8867864 - Flags: review?(bugmail)
(In reply to Andrea Marchesini [:baku] from comment #15)
> I would like Janv to give a +1/-1 on this approach. Note that there is some
> duplicated code in SessionStorageCache/LocalStorageCache but this is wanted:
> LocalStorage is going to be rewritten from scratch.

Whoops, missed this before I started my review pass.  Hopefully he likes it!  I think it's a great clean-up and really appreciate how you broke the patch out into small steps that were pretty easy to follow.  (If splinter knew how to show intra-line diffs and could identify hunks moved between files, it would have been most easy, but splinter is probably the best option we have right now for situations like this.)
Comment on attachment 8867774 [details] [diff] [review]
part 1 - SessionStorage

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

::: dom/storage/SessionStorage.cpp
@@ +60,5 @@
> +    aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
> +    return;
> +  }
> +
> +  return mCache->Key(aIndex, aResult);

nit: "return" of void result in a void function.

@@ +73,5 @@
> +    aRv.Throw(NS_ERROR_DOM_SECURITY_ERR);
> +    return;
> +  }
> +
> +  return mCache->GetItem(aKey, aResult);

nit: "return" of void result in a void function.

::: dom/storage/SessionStorage.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_Sessiontorage_h

typo: you're missing the "S" in "Storage" here and on the define and the comment closing out the ifdef at the bottom.

@@ +81,5 @@
> +  }
> +
> +  uint32_t Length() const { return mKeys.Count(); }
> +
> +  void Key(uint32_t aIndex, nsAString& aResult);

no-action digression: Interesting interaction between Mozilla naming guidelines[1] around Get() and fallibility and the spec here.  (These were fallible GetLength and GetKey under StorageCache, although the spec is "length" and "key()"; the other names are consistent with the spec.)
Attachment #8867774 - Flags: review?(bugmail) → review+
Attachment #8867705 - Flags: review?(bugmail) → review+
Comment on attachment 8867864 [details] [diff] [review]
part 3 - SessionStorageManager

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

::: dom/storage/LocalStorage.h
@@ -28,5 @@
> -    eLocalStorage = 1,
> -    eSessionStorage = 2
> -  };
> -
> -  StorageType GetType() const;

Self-note: The GetType() implementation gets removed from LocalStorage.cpp in part 4.

::: dom/storage/SessionStorageManager.cpp
@@ +24,5 @@
> +NS_IMETHODIMP
> +SessionStorageManager::PrecacheStorage(nsIPrincipal* aPrincipal,
> +                                       nsIDOMStorage** aRetval)
> +{
> +  // Nothing to reload.

nit: s/reload/preload.

@@ +35,5 @@
> +                                     const nsAString& aDocumentURI,
> +                                     bool aPrivate,
> +                                     nsIDOMStorage** aRetval)
> +{
> +  nsAutoCString origin;

suggest renaming to originKey.

@@ +71,5 @@
> +                                  nsIDOMStorage** aRetval)
> +{
> +  *aRetval = nullptr;
> +
> +  nsAutoCString origin;

suggest renaming to originKey here too.

::: dom/storage/SessionStorageManager.h
@@ +26,5 @@
> +
> +private:
> +  ~SessionStorageManager();
> +
> +  typedef nsRefPtrHashtable<nsCStringHashKey, SessionStorageCache> OriginHashTable;

Similarly to GenerateOriginKey comments, I suggest renaming this to OriginKeyHashTable.

::: dom/storage/StorageUtils.cpp
@@ +10,5 @@
> +namespace dom {
> +namespace StorageUtils {
> +
> +nsresult
> +GenerateOriginKey(nsIPrincipal* aPrincipal, nsACString& aOriginAttributes,

Reviewer note: This corresponds to AppendOriginNoSuffix in StorageManager.cpp (which becomes LocalStorageManager.cpp).  The existing implementation is removed and callers updated in part 5.

@@ +41,5 @@
> +  }
> +
> +  // Append reversed domain
> +  nsAutoCString reverseDomain;
> +  rv = CreateReversedDomain(domainOrigin, reverseDomain);

Any reason CreateReversedDomain and ReverseString weren't moved out of StorageManager.cpp (which becomes LocalStorageManager.cpp)?

::: dom/storage/StorageUtils.h
@@ +14,5 @@
> +namespace StorageUtils {
> +
> +nsresult
> +GenerateOriginKey(nsIPrincipal* aPrincipal, nsACString& aOriginAttributes,
> +                  nsACString& aOrigin);

I'd suggest argument name changes here, especially since the precursor method AppendOriginNoSuffix was much more clear about what is going on.
- s/aOriginAttributes/aOriginAttrSuffix/ like how some of the call-sites use it.  It's a suffix, not the origin attributes.
- s/aOrigin/aOriginKey/ since it's very far from the expectation of the origin term throughout the rest of the system.  (It has no suffix, the domain is reversed, there's an extra period, the scheme comes after the reversed domain with a colon, a port is appended when it's the default port.)
Attachment #8867864 - Flags: review?(bugmail) → review+
Attachment #8867707 - Flags: review?(bugmail) → review+
Attachment #8867777 - Flags: review?(bugmail) → review+
Attachment #8867709 - Flags: review?(bugmail) → review+
Comment on attachment 8867710 [details] [diff] [review]
part 7 - SessionStorageManager must be a StorageObserverSink

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

::: dom/storage/SessionStorageManager.cpp
@@ +239,5 @@
> +  }
> +
> +  // Clear all private-browsing caches
> +  if (!strcmp(aTopic, "private-browsing-data-cleared")) {
> +    ClearStorages(pattern, EmptyCString());

This case will clear all session storages, not just private browsing ones.  The precursor code worked because kUnloadPrivate specified the specific DataSet to clear, not because a pattern was provided that would only match private browsing OriginAttributes.  (The call is `Notify("private-browsing-data-cleared");` which uses a default for both.)  Also, as we know from the chrome case, the pattern would also not be sufficient because mIsPrivate can be independent from the OriginAttributes in the case of a chrome principal.

It seems like perhaps no action is required in this case?  We expect the SessionStorageManager's refcount to go to zero and all of its SessionStorage and SessionStorageCache instances too.  The broadcast really only matters for the LocalStorage instances where the manager is eternal and, at least previously, keepalives meant the LocalStorageCache instances could stay alive even without any LocalStorage references.

@@ +244,5 @@
> +    return NS_OK;
> +  }
> +
> +  // Clear localStorage data beloging to an origin pattern
> +  if (!strcmp(aTopic, "origin-attr-pattern-cleared")) {

This shouldn't be here or should be a no-op.  In part 4 you removed this code block from the LocalStorageManager's logic for this:
    // sessionStorage is expected to stay	
    if (mType == eSessionStorage) {	
      return NS_OK;	
    }

@@ +257,5 @@
> +    return NS_OK;
> +  }
> +
> +#ifdef DOM_STORAGE_TESTS
> +  if (!strcmp(aTopic, "test-reload")) {

This should be removed or no-oped.  In part 4 you removed this code block from the LocalStorageManager's logic for this:
  if (mType != eLocalStorage) {	
        return NS_OK;	
  }

(Also, it uses kTestReload which only clears the kDefaultSet, not the kSessionSet, so we would not expect this to clear sessionstorage from that perspective either.)

@@ +263,5 @@
> +    ClearStorages(pattern, EmptyCString());
> +    return NS_OK;
> +  }
> +
> +  if (!strcmp(aTopic, "test-flushed")) {

I'm not sure this case makes any sense for SessionStorage to respond to either.  The behavior is consistent with the pre-refactoring version, but it's also somewhat nonsensical.  The test infrastructure only listens for the observer to fire a single time, and it's a LocalStorage thing (about the flushing of the contents of the DB thread), so having SessionStorage redundantly announce it seems wrong.
Attachment #8867710 - Flags: review?(bugmail) → review+
Comment on attachment 8867711 [details] [diff] [review]
part 8 - Rename StorageCache to LocalStorageCache

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

In LocalStorageCache:
- CheckPrincipal() can be removed.  It was previously used in StorageManagerBase::GetStorageInternal, 
- Accordingly, mPrincipal can be removed.
Attachment #8867711 - Flags: review?(bugmail) → review+
There are another bit to fix: also sessionStorage needs 2 DataSet: SessionOnly and Default. This is needed to be totally compatible with the current code. 3 more patches are needed: 2 about moving code, 1 real stuff.
Attachment #8868131 - Flags: review?(bugmail)
Attachment #8868131 - Flags: review?(bugmail) → review+
Attachment #8868132 - Flags: review?(bugmail) → review+
Attachment #8868133 - Flags: review?(bugmail) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9957d3e64073
Split SessionStorage and LocalStorage implementation - part 1 - SessionStorage, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d959241eaf7d
Split SessionStorage and LocalStorage implementation - part 2 - Rename Storage.{cpp,h} to LocalStorage.{cpp,h}, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7d7228924b
Split SessionStorage and LocalStorage implementation - part 3 - SessionStorageManager, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d373c78235
Split SessionStorage and LocalStorage implementation - part 4 - Rename StorageManagerBase to LocalStorageManager, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc8021ded92b
Split SessionStorage and LocalStorage implementation - part 5 - Shared broadcasting of changes, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/6937455b6098
Split SessionStorage and LocalStorage implementation - part 6 - Implementation of IsForkOf(), r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/647d36b541cc
Split SessionStorage and LocalStorage implementation - part 7 - SessionStorageManager must be a StorageObserverSink, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc00341ec4a7
Split SessionStorage and LocalStorage implementation - part 8 - Rename StorageCache to LocalStorageCache, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb97abbbd158
Split SessionStorage and LocalStorage implementation - part 9 - Move SessionStorageCache in separate files, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/47c603ef87ce
Split SessionStorage and LocalStorage implementation - part 10 - Move IsSessionOnly() in Storage class, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2f47ba116b42
Split SessionStorage and LocalStorage implementation - part 11 - SessionStorageCache must have 2 DataSet: default and sessionOnly, r=asuth
Flags: needinfo?(jvarga)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.