Closed Bug 584946 Opened 10 years ago Closed 9 years ago

e10s: localStorage

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b3+ ---

People

(Reporter: jdm, Assigned: jdm)

References

(Blocks 1 open bug, )

Details

Attachments

(1 file, 18 obsolete files)

138.00 KB, patch
mayhemer
: review+
Details | Diff | Splinter Review
Sites break because checking for the existence of localStorage causes failures.
Blocks: 516521
OS: Linux → Windows CE
The support is completely missing, let's move this forward.

-> me
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
Summary: e10s: localstorage is busted → e10s: localStorage
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
The popular blog commenting system DISQUS uses localStorage, and is therefore completely busted: http://disqus.com/, click the live demo to test.
Honza's busy, so this reverts back to me.
Assignee: honzab.moz → josh
Josh, also please take into account that your patch will probably be on top of patches from bug 527667 and bug 536544.

I'll go through your patch and give you feedback soon.
Attachment #479602 - Attachment is obsolete: true
This version gives correct results when tested against a simple visit counter using globalStorage.
sessionStorage and localStorage also appear to work correctly.  Private browsing is not testable as it doesn't exist in Fennec.
Attachment #479844 - Flags: feedback?(honzab.moz)
Comment on attachment 479844 [details] [diff] [review]
WIP Remote persistent DOMStorage databases from content to chrome

This approach is unfortunately wrong, from following reasons:
- the in-mem db lives and is filled only on the content process but it must live longer then it and must be shared between content processes
- there is lot of low level stuff done like usage count, quota checking remotely, actually the split is in the low level code ; that has sever perfomance impact and simply is not very good
- remoting GetAllKeys is insane and unnecessary ; this will carry all keys and values for a scope between processes!
- the persistent database (one for chrome and one for content) are used as shared services ; the patch seems to create a new one for each storage instance
- also there is some stuff remoted like RemoveOwner(s), RemoveAll that is called only on chrome


The approach should be:
- have two class implementations for nsIDOMStorage (local/session) and for nsIDOMStorageObsolete (global)
- these two implementations will be child sides of the protocol
- nsGlobalWindow will instead of the current implementations return these child ones to be used from content
- the parent is then responsible for getting the 'real' storage instance on the chrome process and remote to it
- the parent side needs the principal and some other small pieces of information from the child ; only problem is to carry the principal
- we have to send StorageEvents to all child processes that by the spec have to receive it, see BroadcastChangeNotification implementation in both 'real' classes and nsGlobalWindow::Observe that catches the related o.s. notifications
- parent sides of the protocol may each listen to its appropriate o.s. notification (inspired by nsGlobalWindow::Observe implementation) and push the notification to its child
Attachment #479844 - Flags: feedback?(honzab.moz) → feedback-
Adding dwitt because of the problem with serializing principals from content to chrome.
Duplicate of this bug: 605975
Duplicate of this bug: 606041
OS: Windows CE → All
Hardware: x86 → All
Duplicate of this bug: 606375
Depends on: 606538
After discussion with bz on IRC, it seems we are able to serialize/deserialize all kinds of principals but codebase carrying nsSimpleURI.  nsSimpleURIs instances are always considered by NS_SecurityCompareURIs as non-equal even carry the same URI, two instances of nsSimpleURI are securely equal only when both are the one and same instance (there is pointer-to-pointer equality).

AFAIK, DOM storage should be instantiated with nsSimpleURI codebase principal only for about: and moz-safe-about: URIs, specially :home - the Home Page.  Fennec doesn't have this kind of Home Page, AFAIK.  So, serializing the principals could be the solution here for us.

Also, I'm sure we will send a principal from content to chrome just ones.  So, after we create a principal on the chrome process, it will be, if even so, compared at most to it self.  All security comparisons will be made on the content process - no de/serialization involved.
Duplicate of this bug: 604258
Attached patch New WIP (obsolete) — Splinter Review
Attachment #479844 - Attachment is obsolete: true
Notes for the most recent WIP:
- notifications are not sent to content yet
- I haven't checked whether principals are being used correctly yet
- there is a lot of extra code for remoting nsDOMStorage2 which is not being used right now
- I just create nsDOMStorage/nsDOMStorage2 in StorageParent::StorageParent, which is incorrect
- DOMStorageItem secure values are probably wrong a lot of the time
tracking-fennec: 2.0+ → 2.0b3+
The split must not be made on level of any existing interface or class we have.  The split must be made inside the nsDOMStorage class.

To outline, for instance let's take nsDOMStorage::SetItem, it should be split like this:

nsDOMStorage::SetItem(const nsAString& aKey, const nsAString& aData)
{
  if (!CacheStoragePermissions()) 
    return NS_ERROR_DOM_SECURITY_ERR;

  rv = mStorageImpl->SetValue(IsCallerSecure(), mSessionOnly, aKey, aData);
  NS_ENSURE(rv);

  if ((oldValue != aData || mStorageType == GlobalStorage) && mEventBroadcaster)
    mEventBroadcaster->BroadcastChangeNotification(aKey, oldValue, aData);
}

mStorageImpl is then a DOMStorageBase type that is implemented by two concerete classes:

DOMStorageChild, that implements the child side of the protocol and in most cases does SendXXX or CallXXX.

DOMStorageImpl, that implements the code we ripe out of nsDOMStorage.

When we have the content process, the mStorageImpl (or whatever we call it) will be DOMStorageChild.  On chrome then there will be DOMStorageParent that wraps the DOMStorageImpl.

When we have only the chrome process, the mStorageImpl will be DOMStorageImpl directly - i.e. no IPC bridge.

To summarize:
- security checking against the principal or domain will remain on the content process ; i.e. no need to serialize the principal
- storage event notification remain on the content process, it simplifies things a lot
- all data 'physically' present on the chrome process ONLY, even just stored in memory for some cases ; one can say this is wasting, but it has reasons:
  1. chrome can modify the data e.g. by Clean Recent History, cookie setting changes, domain data clear or leaving PB mode
(hit enter too soon...)

s/ripe/rip/

  2. after we split to have more then a single content process, we have to carry data modifications among those processes anyway and holding them on the chrome process is IMO the most clear way to achieve that, also conforms to bug 600307

Following methods and members will be moved to the DOMStorageImpl (Base) class:
- UseDB() and mUseDB with it, initialized ones during construction, never changes later
- SessionOnly(), needed by the nsDOMStorageDBWrapper to decide on the DB to use, this may change during lifetime of the storage, perfect would be to move determination of mSessionOnly state to chrome, I believe we can do that
- CanUseChromePersist() and mCanUseChromePersist, value never changes after init, may also be determined on the chrome process (deps just on URI schema)
- GetCachedValue()
- GetDBValue()
- SetDBValue()
- SetSecure()
- ClearAll()
- CloneFrom() (Clone() that is using this must probably be remoted as well)
- RegisterObservers()
- MaybeCommitTemporaryTable()
- WasTemporaryTableLoaded()
- SetTemporaryTableLoaded()
- static InitDB()
- mDomain - copied only, needed on both processes
- mItemsCached and mItems
- mScopeDBKey - probably determined on content and carried to chrome as a ctor argument
- mQuotaETLDplus1DomainDBKey - as previous
- mQuotaDomainDBKey - and also here
- mLoadedTemporaryTable
- mLastTemporaryTableAccessTime
- mTemporaryTableAge
- GetScopeDBKey()
- GetQuotaDomainDBKey()

Then:
- the DB classes methods must change to take DOMStorageImpl instead of nsDOMStorage
- calls to gStorageManager->AddToStoragesHash and RemoveFromStoragesHash must be moved to the DOMStorageImpl class ctor and dtor, the nsDOMStorageManager.mStorages is used on the chrome only
- nsDOMStorageItem refers the storage, but it has to refer or better call on the DOMStorageImpl instead
- GetDBValue is doing security check for IsCallerSecure() and the secure flag of the item, it should be moved out of that method and checked by all consumers of GetDBValue() (this is leftover from globalStorage, will be removed in next major release)


That's hopefully all.. Sorry, this code is really a mess..
Duplicate of this bug: 608002
Just uploading my current WIP as a snapshot in case of hard drive failure.  Currently passes the basic set/get tests at http://www.joshmatthews.net/localstorage.html for all of globalStorage/localStorage/sessionStorage.  The next thing to do is either recreate the localStorage mochitests somewhere else, or find some way to run mochitests in fennec.
Are your last 3 patches (WIP Part [1-3]) dependent on the first one (New WIP)?
Comment on attachment 486753 [details] [diff] [review]
New WIP

Nope!
Attachment #486753 - Attachment is obsolete: true
Attachment #489588 - Attachment is obsolete: true
Forgot to add StorageChild.cpp
Josh, your patched no longer applies to the latest m-c default.  Could you please update them and resubmit?  In whatever state they are..  I would like to take a look and give you feedback.  When split this way it is not easy to stare at them in bugzilla diff view and try to figure out what is where.  Thanks!
I'll do so on Monday, since I've made a bunch of important changes (not least of which is implementing cross-process CloneFrom) which aren't in the patches on this bug.  I don't really want to qfold the patches together yet, since the current division does make the diffs clearer - patch #1 is just moving and renaming functions; patch #2 properly implements DOMStorageImpl so that existing mochitests pass in desktop Firefox; patch #3 implements the cross-process work.
Keep the patches split as you need, I can fold them my self, that is ok.  I would just ask for updated patches to be able to do that.  Thanks.

BTW, I looked a bit and it seems to be on the right way.  Will give you more feedback after I can fold.
Attachment #489613 - Attachment is obsolete: true
Unbittrotted patch #3 so it can apply to m-c.
Josh, I cannot even apply Part 1.
(In reply to comment #32)
> Josh, I cannot even apply Part 1.

Ah!  I had it in a wrong order in my series.  Sorry for that.
Attachment #489586 - Attachment is obsolete: true
Attachment #489587 - Attachment is obsolete: true
Attachment #490466 - Attachment is obsolete: true
Attachment #490726 - Attachment is obsolete: true
All patches updated to m-c tip.  They now pass all sessionStorage and storageevent mochitests, and all of the localStorage mochitests that don't rely on setting preferences or permissions from the content process.
Sorry, part 3 won't compile at the moment because of the IPDL infallible nsTArray change.  If you want to try to build with it, just replace StorageChild::GetKeys with |return nsnull;|
Attachment #490731 - Attachment is obsolete: true
Attachment #491720 - Attachment is obsolete: true
Attachment #491725 - Attachment is obsolete: true
Attached patch Folded patches. (obsolete) — Splinter Review
Comment on attachment 491728 [details] [diff] [review]
Folded patches.

All sessionStorage and storageevent tests pass, and a significant number of localstorage tests as well.  The failures I did see all had to do with attempting to set cookies or permissions from the child, which is broken in e10s land, so the tests weren't able to run to completion.
Attachment #491728 - Flags: review?(honzab.moz)
Duplicate of this bug: 606927
In further news, I ran the localstorage mochitests with a patch that allows setting cookies and permissions in content processes, but doesn't update the parent at all.  There were 48 failures across the following tests:

test_cookieSession-phase2.html: an artifact of the way the permission manager works in the child, as it notifies observers that only exist in the parent process.

test_localStorageQuota.html: an artifact of setting quota prefs in the child when they're only checked in the parent

test_localStorageQuotaSessionOnly.html: same as previous.

test_localStorageQuotaSessionOnly2.html: same as previous.

test_removeOwnersAPI.html: accesses the nsIDOMStorageManager manually and calls things like clearOfflineApps(), which will not work in the content process in its present form.  Honza, do you think this is something that needs to be addressed, or will this sort of code only be run in chrome?

test_remoteOwnersAPISessionOnly.html: same as previous
(In reply to comment #46)
> In further news, I ran the localstorage mochitests with a patch that allows
> setting cookies and permissions in content processes, but doesn't update the
> parent at all.  There were 48 failures across the following tests:
> 
> test_cookieSession-phase2.html: an artifact of the way the permission manager
> works in the child, as it notifies observers that only exist in the parent
> process.

Calling on "@mozilla.org/cookie/permission;1" doesn't work from the child process?  I would expect it doesn't.  OK, this test will need a little overwrite then.  Call to cookie setting should be done on the chrome process and the localStorage manipulation test should go on the content process.  Probably turn the test to chrome test and move the localStorage code/tests to a frame and load it with ?phase=N in the URL to perform a test step we want?  Or, maybe check how interOriginTest2.js works/is used (helper for cross-origin test control), but I'm not sure if it will work for chrome pages.

> 
> test_localStorageQuota.html: an artifact of setting quota prefs in the child
> when they're only checked in the parent

I'm afraid this is gonna be the same sort of rework.

> 
> test_localStorageQuotaSessionOnly.html: same as previous.
> 
> test_localStorageQuotaSessionOnly2.html: same as previous.
> 
> test_removeOwnersAPI.html: accesses the nsIDOMStorageManager manually and calls
> things like clearOfflineApps(), which will not work in the content process in
> its present form.  Honza, do you think this is something that needs to be
> addressed, or will this sort of code only be run in chrome?

RemoveOwners API is invoked only on the chrome process.  It is actually code in nsDOMStorageManager::Observe, "offline-app-removed" topic.  It is going to be as well split to chrome/content test.

> 
> test_remoteOwnersAPISessionOnly.html: same as previous

In general, I believe that there are more tests that have already been split that way or there may be potentially bugs to have some generic solution fot this.  If not, you may think of something yourself or just rather go a specific way for each test.  Up to you.
Duplicate of this bug: 613933
Duplicate of this bug: 613450
Duplicate of this bug: 613990
I ran the patch through try today and discovered that I broke test_bug463000.html, along with test_localStorageBasePrivateBrowsing.html, localstorage/test_localStorageBaseSessionOnly.html, and test_localStorageCookieSettings.html.  I'm investigating the failures tonight.
Attached patch Changes to make tests pass (obsolete) — Splinter Review
With this pass, there are no more localstorage mochitest failures.  I'm still looking into the test_bug463000.html failure, but I can't seem to reproduce it locally.  Try shows this:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1290467340.1290468299.3799.gz&fulltext=1#err0

Thread 0 (crashed)
 0  0x0
    eip = 0x00000000   esp = 0x001db204   ebp = 0x0055da00   ebx = 0x0000000c
    esi = 0x031e394c   edi = 0x0055d9b8   eax = 0x04087c80   ecx = 0x090f3160
    edx = 0x001db260   efl = 0x00010202
    Found by: given as instruction pointer in context
 1  xul.dll!nsDOMStorage::ClearAll() [nsDOMStorage.h:16b832819ac3 : 420 + 0xb]
    eip = 0x68f0ae97   esp = 0x001db208   ebp = 0x0055da00
    Found by: stack scanning
 2  xul.dll!ClearStorage [nsDOMStorage.cpp:16b832819ac3 : 313 + 0xb]
    eip = 0x68f61bbf   esp = 0x001db20c   ebp = 0x0055da00
    Found by: call frame info
 3  xul.dll!nsTHashtable<nsCertOverrideEntry>::s_EnumStub(PLDHashTable *,PLDHashEntryHdr *,unsigned int,void *) [nsTHashtable.h:16b832819ac3 : 420 + 0xc]
    eip = 0x68ee546b   esp = 0x001db214   ebp = 0x0055da00
    Found by: call frame info
 4  xul.dll!PL_DHashTableEnumerate [pldhash.c:16b832819ac3 : 754 + 0x35]
    eip = 0x68b38f38   esp = 0x001db220   ebp = 0x0055da00
    Found by: call frame info
 5  xul.dll!nsTHashtable<nsDOMStorageEntry>::EnumerateEntries(PLDHashOperator (*)(nsDOMStorageEntry *,void *),void *) [nsTHashtable.h:16b832819ac3 : 241 + 0x17]
    eip = 0x68fa201c   esp = 0x001db254   ebp = 0x001db268
    Found by: call frame info
 6  xul.dll!nsDOMStorageManager::Observe(nsISupports *,char const *,unsigned short const *) + 0xd9
    eip = 0x68e1208f   esp = 0x001db270   ebp = 0x001db2ec
    Found by: previous frame's frame pointer
Attached patch Changes to make tests pass (obsolete) — Splinter Review
Updated for test_bug463000.html.
Attachment #492506 - Attachment is obsolete: true
Attachment #492527 - Flags: review?(honzab.moz)
Comment on attachment 491728 [details] [diff] [review]
Folded patches.

Just nits:

>diff --git a/dom/ipc/Makefile.in b/dom/ipc/Makefile.in
> 		-I$(srcdir)/../src/base \
>+                -I$(srcdir)/../src/storage \

Indention.

>+StorageChild::GetKeys(bool aCallerSecure)
>+  //keys->MoveElementsFrom(remoteKeys);

Maybe remove this line?

>+StorageParent::CloneFrom(bool aCallerSecure,
>+                         StorageParent* aOther)
>+{
>+  
>+  mStorage->RemoteCloneFrom(aCallerSecure, aOther->mStorage, this);

Remove the empty line.

>+struct CopyArgs {
>+  DOMStorageImpl* storage;
>+  StorageParent* eventBroadcaster;
>+ bool callerSecure;

Indention.

>+static PLDHashOperator
>+CopyStorageItems(nsSessionStorageEntry* aEntry, void* userArg)
>+{

>+  if ((oldValue != value) && args->eventBroadcaster)
>+    args->eventBroadcaster->
>+        BroadcastChangeNotification(aEntry->GetKey(), oldValue, value);

You don't need to broadcast.  In the single process version the window is not attached to the new (cloned) storage yet (if there is even the window), so it cannot receive messages anyway.  Also, if it would, it would be a bug!  Just remove this, also the BroadCast protocol events.

r=honzab
Attachment #491728 - Flags: review?(honzab.moz) → review+
Comment on attachment 492527 [details] [diff] [review]
Changes to make tests pass

r=honzab
Attachment #492527 - Flags: review?(honzab.moz) → review+
Rebased, folded, nits addressed.  This is ready to land.
Attachment #492711 - Attachment description: Implement e10s localstorage. * * → Implement e10s localstorage.
Attachment #492527 - Attachment is obsolete: true
Attachment #491728 - Attachment is obsolete: true
Attachment #491726 - Attachment is obsolete: true
Attachment #490724 - Attachment is obsolete: true
Attachment #490723 - Attachment is obsolete: true
Comment on attachment 492711 [details] [diff] [review]
Implement e10s localstorage. [Check in comment 57]

http://hg.mozilla.org/mozilla-central/rev/4973e2b9a905
Attachment #492711 - Attachment description: Implement e10s localstorage. → Implement e10s localstorage. [Check in comment 57]
Attachment #492711 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 616348
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.