Closed
Bug 584946
Opened 14 years ago
Closed 14 years ago
e10s: localStorage
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
fennec | 2.0b3+ | --- |
People
(Reporter: jdm, Assigned: jdm)
References
()
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.
Comment 1•14 years ago
|
||
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
Updated•14 years ago
|
tracking-fennec: --- → ?
Updated•14 years ago
|
tracking-fennec: ? → 2.0+
Assignee | ||
Comment 2•14 years ago
|
||
The popular blog commenting system DISQUS uses localStorage, and is therefore completely busted: http://disqus.com/, click the live demo to test.
Assignee | ||
Comment 3•14 years ago
|
||
Honza's busy, so this reverts back to me.
Assignee: honzab.moz → josh
Assignee | ||
Comment 4•14 years ago
|
||
Comment 5•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #479602 -
Attachment is obsolete: true
Assignee | ||
Comment 6•14 years ago
|
||
Assignee | ||
Comment 7•14 years ago
|
||
This version gives correct results when tested against a simple visit counter using globalStorage.
Assignee | ||
Comment 8•14 years ago
|
||
sessionStorage and localStorage also appear to work correctly. Private browsing is not testable as it doesn't exist in Fennec.
Assignee | ||
Updated•14 years ago
|
Attachment #479844 -
Flags: feedback?(honzab.moz)
Comment 9•14 years ago
|
||
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-
Comment 10•14 years ago
|
||
Adding dwitt because of the problem with serializing principals from content to chrome.
Updated•14 years ago
|
OS: Windows CE → All
Hardware: x86 → All
Comment 14•14 years ago
|
||
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.
Assignee | ||
Comment 16•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #479844 -
Attachment is obsolete: true
Assignee | ||
Comment 17•14 years ago
|
||
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
Updated•14 years ago
|
tracking-fennec: 2.0+ → 2.0b3+
Comment 18•14 years ago
|
||
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
Comment 19•14 years ago
|
||
(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..
Assignee | ||
Comment 21•14 years ago
|
||
Assignee | ||
Comment 22•14 years ago
|
||
Assignee | ||
Comment 23•14 years ago
|
||
Assignee | ||
Comment 24•14 years ago
|
||
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.
Comment 25•14 years ago
|
||
Are your last 3 patches (WIP Part [1-3]) dependent on the first one (New WIP)?
Assignee | ||
Comment 26•14 years ago
|
||
Comment on attachment 486753 [details] [diff] [review] New WIP Nope!
Attachment #486753 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #489588 -
Attachment is obsolete: true
Assignee | ||
Comment 27•14 years ago
|
||
Forgot to add StorageChild.cpp
Comment 28•14 years ago
|
||
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!
Assignee | ||
Comment 29•14 years ago
|
||
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.
Comment 30•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Attachment #489613 -
Attachment is obsolete: true
Assignee | ||
Comment 31•14 years ago
|
||
Unbittrotted patch #3 so it can apply to m-c.
Comment 32•14 years ago
|
||
Josh, I cannot even apply Part 1.
Comment 33•14 years ago
|
||
(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.
Assignee | ||
Updated•14 years ago
|
Attachment #489586 -
Attachment is obsolete: true
Assignee | ||
Comment 34•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #489587 -
Attachment is obsolete: true
Assignee | ||
Comment 35•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #490466 -
Attachment is obsolete: true
Assignee | ||
Comment 36•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #490726 -
Attachment is obsolete: true
Assignee | ||
Comment 37•14 years ago
|
||
Assignee | ||
Comment 38•14 years ago
|
||
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.
Assignee | ||
Comment 39•14 years ago
|
||
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;|
Assignee | ||
Updated•14 years ago
|
Attachment #490731 -
Attachment is obsolete: true
Assignee | ||
Comment 40•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #491720 -
Attachment is obsolete: true
Assignee | ||
Comment 41•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Attachment #491725 -
Attachment is obsolete: true
Assignee | ||
Comment 42•14 years ago
|
||
Assignee | ||
Comment 43•14 years ago
|
||
Assignee | ||
Comment 44•14 years ago
|
||
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)
Assignee | ||
Comment 46•14 years ago
|
||
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
Comment 47•14 years ago
|
||
(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.
Assignee | ||
Comment 51•14 years ago
|
||
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.
Assignee | ||
Comment 52•14 years ago
|
||
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
Assignee | ||
Comment 53•14 years ago
|
||
Updated for test_bug463000.html.
Attachment #492506 -
Attachment is obsolete: true
Attachment #492527 -
Flags: review?(honzab.moz)
Comment 54•14 years ago
|
||
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 55•14 years ago
|
||
Comment on attachment 492527 [details] [diff] [review] Changes to make tests pass r=honzab
Attachment #492527 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 56•14 years ago
|
||
Rebased, folded, nits addressed. This is ready to land.
Assignee | ||
Updated•14 years ago
|
Attachment #492711 -
Attachment description: Implement e10s localstorage. * * → Implement e10s localstorage.
Assignee | ||
Updated•14 years ago
|
Attachment #492527 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #491728 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #491726 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #490724 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #490723 -
Attachment is obsolete: true
Comment 57•14 years ago
|
||
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+
Updated•14 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•