Closed
Bug 422526
Opened 16 years ago
Closed 15 years ago
implement localStorage
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
People
(Reporter: dcamp, Assigned: mayhemer)
References
()
Details
Attachments
(1 file, 32 obsolete files)
157.35 KB,
patch
|
Details | Diff | Splinter Review |
The html5 spec has removed globalStorage and added localStorage. We should implement localStorage.
Assignee | ||
Updated•16 years ago
|
Assignee: nobody → honzab
Assignee | ||
Comment 1•16 years ago
|
||
Just adding new localStorage attribute. I map the storage to principals origin: e.g. "http://foo.com" or "http://foo.com:8080". I will also create some mochitests for this and also adding cycle collection checking would be good.
Attachment #309461 -
Flags: review?(dcamp)
Reporter | ||
Comment 2•16 years ago
|
||
A few things: * I don't think mLocalStorages needs to be a dictionary - there should only be one localStorage per window. * However, we may want to do something like nsDOMStorageList::mStorages for these things, in the nsDOMStorage code somewhere (maybe NS_GetLocalStorage() or something?) * This one is a bit more annoying: between bugs 407839 and 367373, we're relying on the "domain" field to actually be a domain, in order to clear out all the entries for a given domain. We may be able to solve this somewhat easily by changing the key from "http://www.foo.com:8080" to something like "www.foo.com:http:80", and changing the clearing code to use "DELETE FROM storage WHERE domain = www.foo.com*" instead of "DELETE FROM storage WHERE domain=www.foo.com" (but with valid syntax). I'm not sure that's the best way to solve it, but we need to figure something out before we can do this.
Reporter | ||
Comment 3•16 years ago
|
||
Comment on attachment 309461 [details] [diff] [review] v1 r-'ing for the comments above.
Attachment #309461 -
Flags: review?(dcamp) → review-
Assignee | ||
Comment 4•16 years ago
|
||
(In reply to comment #2) > A few things: > > * However, we may want to do something like nsDOMStorageList::mStorages for > these things, in the nsDOMStorage code somewhere (maybe NS_GetLocalStorage() or > something?) > As I am thinking of it probably localStorage and globalStorage["current.domain"] should return the same object. Then it would be probably good to use the same code from nsDOMStorageList (nsGlobalWindow::gGlobalStorageList) but address the item with principal.URI.asciiHost_or_something. > * This one is a bit more annoying: between bugs 407839 and 367373, we're > relying on the "domain" field to actually be a domain, in order to clear out > all the entries for a given domain. We may be able to solve this somewhat > easily by changing the key from "http://www.foo.com:8080" to something like > "www.foo.com:http:80", and changing the clearing code to use "DELETE FROM > storage WHERE domain = www.foo.com*" instead of "DELETE FROM storage WHERE > domain=www.foo.com" (but with valid syntax). > We have probably two options: The one you propose, but I presume we want to delete also for subdomain/superdomains i.e. if there are entries for "foo.bar.baz.com" and we delete for "baz.com" then also "bar.baz.com" and "foo.bar.baz.com" have to be deleted. Then an elegant solution would be to reverse the string and add dot at the end: "moc.zab.rab.oof." and add the schema/port as you suggest: "moc.zab.rab.oof.:80:http". then we can do: DELETE FROM table WHERE domain like 'moc.zab.%'; There must be index on the column. The other more complicated solution is to add two more columns (keySchema, keyPort) with group index on it and key column. We may search by the domain to delete everything and also search by the origin data easily.
Status: NEW → ASSIGNED
Reporter | ||
Comment 5•16 years ago
|
||
(In reply to comment #4) > (In reply to comment #2) > > We have probably two options: > > The one you propose, but I presume we want to delete also for > subdomain/superdomains i.e. if there are entries for "foo.bar.baz.com" and we > delete for "baz.com" then also "bar.baz.com" and "foo.bar.baz.com" have to be > deleted. Then an elegant solution would be to reverse the string and add dot at > the end: "moc.zab.rab.oof." and add the schema/port as you suggest: > "moc.zab.rab.oof.:80:http". then we can do: DELETE FROM table WHERE domain like > 'moc.zab.%'; There must be index on the column. Yeah, that would go a long way toward helping with 399526.
Comment 6•16 years ago
|
||
Tagging that this will require a doc update when it lands.
Keywords: dev-doc-needed
Comment 7•16 years ago
|
||
I think we want to wean people off globalStorage as quickly as we can since the spec dropped it, and having the more convenient localStorage will make that easier.
Flags: wanted1.9.0.x?
Assignee | ||
Comment 8•16 years ago
|
||
This is first draft using reversed host column. I also changed some internal API from using UTF16 to UTF8 (this saves conversions on some critical places, but adds some when calling/receiving observers notifications.) I was testing on localhost but GetBaseDomain("localhost", 1) fails with NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS. Probably appending ".localnet" would solve this in case of localhost however, do want to allow localhost have access to storage?
Attachment #309461 -
Attachment is obsolete: true
Attachment #316839 -
Flags: review?(dcamp)
Comment 9•16 years ago
|
||
Folks - Could we make sure that this will work from scripts loaded from 'file://' URLs? The current globalStorage stuff does not (bug 357323), and as that stuff is being deprecated anyway, I didn't think it would be useful to add this comment / request to that bug. The HTML5 spec has a concept of 'file:' URLs in its definition of 'origin'. Some of us develop and deploy "Web / AJAX apps" that run completely off the file system, so this capability would be great! Thanks a lot! Cheers, - Bill
Reporter | ||
Comment 10•16 years ago
|
||
This patch still uses mLocalStorages in nsGlobalWindow.cpp - shouldn't nsGlobalWindow.cpp just have an mLocalStorage, and have the storage service manage the list of active storages? It might be nice to have a comment somewhere explaining the different keys (domain key, quota key). Also, I'm not sure quotas and offline apps are handled how you would expect. Right now, if I have enabled offline access for foo.bar.com, and baz.bar.com also uses localStorage, they will share a quota, but writes to foo.bar.com will be checked against a higher quota value than writes to baz.bar.com. I would kind of expect that once you have enabled an offline app at a subdomain, that it would receive its own quota. And for some purposes (such as a UI displaying storage usage), I think you'd want nsIDOMStorageManager::GetUsage() to return usage for that specific domain, not the quota domain. I'm not sure offhand the best way to solve that, but maybe we want some sort of enum describing possible usage columns. Something like enum UsageType { USAGE_DOMAIN, // checked against the domain column USAGE_ETLD1 // checked against the quota domain, ie the ETLD+1 }; nsDOMStorageDB::GetUsage() could be modified to use this and choose the right column to check. nsDOMStorage::GetQuota() would then also return a StorageType to check (_DOMAIN for offline apps, _ETLD1 for normal pages). SetKey() would need to be modified to take this, etc.
Reporter | ||
Comment 11•16 years ago
|
||
Two more things: a) This doesn't appear to change nsIDOMStorage to only deal with strings, as outlined in the new spec. We should probably add a new interface with the new semantics, and implement that for at least localStorage/sessionStorage. b) We discussed at the summit emitting a javascript warning when globalStorage is accessed. We should either do that here or file a followup bug.
Assignee | ||
Comment 12•16 years ago
|
||
(In reply to comment #10) > This patch still uses mLocalStorages in nsGlobalWindow.cpp - shouldn't > nsGlobalWindow.cpp just have an mLocalStorage, and have the storage service > manage the list of active storages? Your idea is based on fact that each nsGlobalWindow instance is always bound to its own single page (URL, what means also a domain) and therefor the storage can be held in |DOMStorage* nsGlobalWindow::mLocalStorage| and load using storage service when nsGlobalWindow is being associated with URL? Do I understand correctly? > > It might be nice to have a comment somewhere explaining the different keys > (domain key, quota key). I will do it. > > Also, I'm not sure quotas and offline apps are handled how you would expect. > Right now, if I have enabled offline access for foo.bar.com, and baz.bar.com > also uses localStorage, they will share a quota, but writes to foo.bar.com will > be checked against a higher quota value than writes to baz.bar.com. I would > kind of expect that once you have enabled an offline app at a subdomain, that > it would receive its own quota. > > And for some purposes (such as a UI displaying storage usage), I think you'd > want nsIDOMStorageManager::GetUsage() to return usage for that specific domain, > not the quota domain. > > I'm not sure offhand the best way to solve that, but maybe we want some sort of > enum describing possible usage columns. Something like > > enum UsageType { > USAGE_DOMAIN, // checked against the domain column > USAGE_ETLD1 // checked against the quota domain, ie the ETLD+1 > }; > > nsDOMStorageDB::GetUsage() could be modified to use this and choose the right > column to check. nsDOMStorage::GetQuota() would then also return a StorageType > to check (_DOMAIN for offline apps, _ETLD1 for normal pages). SetKey() would > need to be modified to take this, etc. > I agree it might be inconsistent. To get the quota key I use eTLD+1 obtained from the ETLD service. To get the quota key (the appropriate domain) we can first search a list of domains enabled for offline applications. If it is not found in this list use eTLD+1 as we do now. Then there is no need to have a flag because the domain is picked automatically. Using a new method like GetQuotaDomainForOfflineApplication, that takes URL and returns appropriate domain, instead of ETLD service where appropriate would probably solve this problem. (In reply to comment #11) > Two more things: > > a) This doesn't appear to change nsIDOMStorage to only deal with strings, as > outlined in the new spec. We should probably add a new interface with the new > semantics, and implement that for at least localStorage/sessionStorage. > Agree. I was probably writing this patch before the change was added to the spec or I simply overlooked that. > b) We discussed at the summit emitting a javascript warning when globalStorage > is accessed. We should either do that here or file a followup bug. > Not a bad idea. What about the idea to extend Firebug to have an explorer for localStorage/globalStorage and let it generate a script to copy values from globalStorage to localStorage that could be embedded to the page code? Maybe it is too much work when there are not many users of globalStorage list.
Reporter | ||
Comment 13•16 years ago
|
||
(In reply to comment #12) > (In reply to comment #10) > > This patch still uses mLocalStorages in nsGlobalWindow.cpp - shouldn't > > nsGlobalWindow.cpp just have an mLocalStorage, and have the storage service > > manage the list of active storages? > > Your idea is based on fact that each nsGlobalWindow instance is always bound to > its own single page (URL, what means also a domain) and therefor the storage > can be held in |DOMStorage* nsGlobalWindow::mLocalStorage| and load using > storage service when nsGlobalWindow is being associated with URL? Do I > understand correctly? Well, it could be created lazily, but yeah. > > > > I agree it might be inconsistent. To get the quota key I use eTLD+1 obtained > from the ETLD service. To get the quota key (the appropriate domain) we can > first search a list of domains enabled for offline applications. If it is not > found in this list use eTLD+1 as we do now. Then there is no need to have a > flag because the domain is picked automatically. Using a new method like > GetQuotaDomainForOfflineApplication, that takes URL and returns appropriate > domain, instead of ETLD service where appropriate would probably solve this > problem. But the quota domain is fixed at insert time. What if I decide to give foo.bar.com the offline-app permission after it has already added 5 megs of data to the store? Those 5 megs will forever be charged to the bar.com quota, rather than the foo.bar.com quota. > Not a bad idea. What about the idea to extend Firebug to have an explorer for > localStorage/globalStorage and let it generate a script to copy values from > globalStorage to localStorage that could be embedded to the page code? Maybe it > is too much work when there are not many users of globalStorage list. Sounds reasonable, but I don't think it's necessary to land this patch, so let's give it a different bug.
Assignee | ||
Comment 14•16 years ago
|
||
Reflecting: comment 10 except the quota issue for offline-application-enabled domains. it is huge task and not crucially important for localStorage. probably do it in a follow up. comment 11 except b), will be done in a follow up. Mochitests for this new stuff will soon be submitted in a separate patch.
Attachment #316839 -
Attachment is obsolete: true
Attachment #333831 -
Flags: review?(dcamp)
Attachment #316839 -
Flags: review?(dcamp)
Assignee | ||
Comment 15•16 years ago
|
||
And I want to start thinking what's needed for comment 9.
Comment 16•16 years ago
|
||
I don't think a change like this belongs on the stable branch. This can wait until 3.1.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
Assignee | ||
Comment 17•16 years ago
|
||
This is fix including new clear method in both old and new interfaces.
Attachment #333831 -
Attachment is obsolete: true
Attachment #334523 -
Flags: review?(dcamp)
Attachment #333831 -
Flags: review?(dcamp)
Assignee | ||
Comment 18•16 years ago
|
||
Comment on attachment 334523 [details] [diff] [review] Final v2 This is inefficient fix, correct one comes soon.
Attachment #334523 -
Attachment is obsolete: true
Attachment #334523 -
Flags: review?(dcamp)
Assignee | ||
Comment 19•16 years ago
|
||
Adding a new hash table to nsDOMStorageManager mapping origin to local storage objects. This way local storage objects are shared across documents with the same origin in all browsing contexts. There is no other solution because adding all storages to just a single hash table mapped by origin (or domain) would overlap localStorage and sessionStorage instance for just a single origin and also sessionStorages bound to a same origin but coming from different top level browsing contexts (those must be separate according to the spec and as seen from the implementation of nsGlobalWindow::OpenInternal)
Attachment #335608 -
Flags: review?(dcamp)
Assignee | ||
Comment 20•16 years ago
|
||
This is a set of tests for basic localStorage behavior and for sharing/interaction of localStoarage objects between documents with equal/different origins.
Attachment #335609 -
Flags: review?(dcamp)
Reporter | ||
Comment 21•16 years ago
|
||
Comment on attachment 335608 [details] [diff] [review] v3 >diff --git a/dom/src/base/nsGlobalWindow.cpp b/dom/src/base/nsGlobalWindow.cpp >--- a/dom/src/base/nsGlobalWindow.cpp >+++ b/dom/src/base/nsGlobalWindow.cpp >+NS_IMETHODIMP >+nsGlobalWindow::GetLocalStorage(nsIDOMStorage2 ** aLocalStorage) >+{ ... >+ if (sessionOnly) { >+ // XXX ??? is this correct? >+ return GetSessionStorage(aLocalStorage); I don't think so - I think if they try to get localStorage, and only have access to sessionStorage, we should throw here. >+ nsCOMPtr<nsIURI> innerUri = NS_GetInnermostURI(codebase); >+ if (!innerUri) >+ return NS_ERROR_UNEXPECTED; >+ >+ nsCOMPtr<nsIDOMStorageManager> storageManager = >+ do_GetService("@mozilla.org/dom/storagemanager;1", &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = storageManager->GetLocalStorageForURI(innerUri, getter_AddRefs(mLocalStorage)); >+ NS_ENSURE_SUCCESS(rv, rv); GetLocalStorageForURI should probably be in charge of getting the inner uri here. >diff --git a/dom/src/storage/nsDOMStorage.cpp b/dom/src/storage/nsDOMStorage.cpp >--- a/dom/src/storage/nsDOMStorage.cpp >+++ b/dom/src/storage/nsDOMStorage.cpp >@@ -275,21 +286,26 @@ GetOfflineDomains(nsStringArray& aDomain > > nsresult > nsDOMStorageManager::Observe(nsISupports *aSubject, > const char *aTopic, > const PRUnichar *aData) > { > if (!strcmp(aTopic, "offline-app-removed")) { > #ifdef MOZ_STORAGE > nsresult rv = nsDOMStorage::InitDB(); > NS_ENSURE_SUCCESS(rv, rv); >- return nsDOMStorage::gStorageDB->RemoveOwner(nsDependentString(aData)); >+ >+ nsCAutoString subdomainsDBKey; >+ nsDOMStorageDB::CreateDomainDBKey(NS_ConvertUTF16toUTF8(aData), subdomainsDBKey); >+ subdomainsDBKey.AppendLiteral("%"); >+ >+ return nsDOMStorage::gStorageDB->RemoveOwner(subdomainsDBKey); It seems weird to be appending the "%" here. Should nsDOMStorageDB just be taking the hostname, and appending the "%" itself if necessary? If the caller needs to be able to decide whether to accept subdomains, it should probably pass in a bool or something rather than appending %. > > NS_IMETHODIMP > nsDOMStorageManager::GetUsage(const nsAString& aDomain, > PRInt32 *aUsage) > { > nsresult rv = nsDOMStorage::InitDB(); > NS_ENSURE_SUCCESS(rv, rv); > >- return nsDOMStorage::gStorageDB->GetUsage(aDomain, aUsage); >+ nsCAutoString quotadomainDBKey; >+ rv = nsDOMStorageDB::CreateQuotaDomainDBKey(NS_ConvertUTF16toUTF8(aDomain), quotadomainDBKey); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ return nsDOMStorage::gStorageDB->GetUsage(quotadomainDBKey, aUsage); > } I'm not sure this is what we want. If I say "Give me the usage for foo.bar.com", do we really want to respond with the usage for all of bar.com? It's also not clear (at least from the usage) whether nsDOMStorageDB->GetUsage() includes subdomains. This should probably match whatever you end up doing for RemoveDomains (ie, maybe add an "aIncludeSubdomains" argument). >+NS_IMETHODIMP >+nsDOMStorageManager::GetLocalStorageForURI(nsIURI *aURI, >+ nsIDOMStorage2 **_retval NS_OUTPARAM) >+{ You don't need the NS_OUTPARAM here. >+ >+ return NS_OK; >+} >+ >+PR_STATIC_CALLBACK(PLDHashOperator) >+CheckSecure(nsSessionStorageEntry* aEntry, void* userArg) >+{ >+ PRBool* secure = (PRBool*)userArg; >+ *secure |= aEntry->mItem->IsSecure(); >+ >+ return PL_DHASH_NEXT; >+} >+ >+NS_IMETHODIMP >+nsDOMStorage::Clear() >+{ >+ PRBool foundSecureItem = PR_FALSE; >+ mItems.EnumerateEntries(CheckSecure, &foundSecureItem); >+ >+ if (foundSecureItem && !IsCallerSecure()) { >+ return NS_ERROR_DOM_SECURITY_ERR; >+ } Do you need to CacheKeysFromDB before enumerating them here? >-already_AddRefed<nsIDOMStorage> >+already_AddRefed<nsIDOMStorage2> > nsDOMStorage::Clone(nsIURI* aURI) > { > if (UseDB()) { > NS_ERROR("Uh, don't clone a global storage object."); Does this need to be updated? >diff --git a/dom/src/storage/nsDOMStorageDB.cpp b/dom/src/storage/nsDOMStorageDB.cpp >--- a/dom/src/storage/nsDOMStorageDB.cpp >+++ b/dom/src/storage/nsDOMStorageDB.cpp >+ >+ PRInt32 schemaVersion; >+ rv = mConnection->GetSchemaVersion(&schemaVersion); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (schemaVersion == 0) >+ { >+ // Perform update as described at bug 422526 Could you include a quick summary in the comments here? >+ >+ mozStorageTransaction transaction(mConnection, PR_FALSE); >+ >+ nsCOMPtr<mozIStorageFunction> function(new nsReverseStringFunction()); >+ NS_ENSURE_TRUE(function, NS_ERROR_OUT_OF_MEMORY); >+ >+ rv = mConnection->CreateFunction(NS_LITERAL_CSTRING("REVERSESTRING"), 1, function); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = mConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING( >+ "UPDATE webappsstore SET domain = REVERSESTRING(domain) || '.'")); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ // We should probably drop the owner column here but >+ // it seems reasonable to keep it with its old values for possible >+ // future use. >+ >+ rv = mConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING( >+ "CREATE INDEX domain_index ON webappsstore(domain)")); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = mConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING( >+ "CREATE INDEX domain_key_index ON webappsstore(domain, key)")); >+ NS_ENSURE_SUCCESS(rv, rv); This isn't broken, but it's a bit confusing - the rest of this clause handles only upgrades to earlier DBs, but these two indexes are created on a fresh db too. Yeah, a fresh db will hit this clause, but it seems like these two indexes should be created up with all the table creation logic. You can use CREATE INDEX IF NOT EXISTS if necessary. >diff --git a/dom/src/storage/nsDOMStorageDB.h b/dom/src/storage/nsDOMStorageDB.h >--- a/dom/src/storage/nsDOMStorageDB.h >+++ b/dom/src/storage/nsDOMStorageDB.h >- nsresult GetUsage(const nsAString &aOwner, PRInt32 *aUsage); >+ nsresult >+ GetUsage(const nsACString& aQuotaDomainDBKey, PRInt32 *aUsage); >+ >+ /** >+ * Creates a database keys used to select records belonging >+ * to a particular origin ("schema://domain:port") that when >+ * appended a '%' can be used for efficient LIKE quries to >+ * search also any subdomain. >+ */ "Creates database keys", and could you give a few examples of how this works, to give a bit more context to the later functions? (I haven't finished looking at everything yet)
Assignee | ||
Comment 22•16 years ago
|
||
It should reflect all comments from the most recent review.
Attachment #335608 -
Attachment is obsolete: true
Attachment #335609 -
Attachment is obsolete: true
Attachment #337695 -
Flags: review?(dcamp)
Attachment #335608 -
Flags: review?(dcamp)
Attachment #335609 -
Flags: review?(dcamp)
Assignee | ||
Comment 23•16 years ago
|
||
As tests v1, but added a test to check the localStorage object disappears when window location changes.
Attachment #337737 -
Flags: review?(dcamp)
Reporter | ||
Updated•16 years ago
|
Attachment #337695 -
Flags: review?(dcamp) → review-
Reporter | ||
Comment 24•16 years ago
|
||
Comment on attachment 337695 [details] [diff] [review] v4 This patch has a lot of trailing whitespace, including some DOS line endings. Please try to clean that up when submitting patches. >diff --git a/docshell/base/nsIDocShell.idl b/docshell/base/nsIDocShell.idl Can you start a thread on dev.platform about the idl changes necessary for this patch please? >diff --git a/dom/public/idl/storage/nsIDOMStorageWindow.idl b/dom/public/idl/storage/nsIDOMStorageWindow.idl >--- a/dom/public/idl/storage/nsIDOMStorageWindow.idl >+++ b/dom/public/idl/storage/nsIDOMStorageWindow.idl >@@ -39,25 +39,32 @@ > > /** > * Interface for a client side storage. See > * http://www.whatwg.org/specs/web-apps/current-work/#scs-client-side > * for more information. > * > * Allows access to contextual storage areas. > */ > > interface nsIDOMStorage; >+interface nsIDOMStorage2; > interface nsIDOMStorageList; > >-[scriptable, uuid(55E9C181-2476-47CF-97F8-EFDAAF7B6F7A)] >+[scriptable, uuid(A44581FE-DD9B-4fd7-9893-00C4AB43F12E)] > interface nsIDOMStorageWindow : nsISupports > { > /** > * Session storage for the current browsing context. > */ >- readonly attribute nsIDOMStorage sessionStorage; >+ readonly attribute nsIDOMStorage2 sessionStorage; > > /** > * Global storage, accessible by domain. > */ > readonly attribute nsIDOMStorageList globalStorage; >+ >+ /** >+ * Local storage for the current origin, what is different >+ * from the current browsing context. >+ */ >+ readonly attribute nsIDOMStorage2 localStorage; What do you mean by "what is different from..."? Isn't it the current origin taken from the current context? > >diff --git a/dom/src/base/nsGlobalWindow.h b/dom/src/base/nsGlobalWindow.h >--- a/dom/src/base/nsGlobalWindow.h >+++ b/dom/src/base/nsGlobalWindow.h > #include "nsIDOMStorageWindow.h" > #include "nsIDOMOfflineResourceList.h" > #include "nsPIDOMEventTarget.h" > #include "nsIArray.h" >+#include "nsInterfaceHashtable.h" Do you still need this include? >diff --git a/dom/src/storage/nsDOMStorage.cpp b/dom/src/storage/nsDOMStorage.cpp >--- a/dom/src/storage/nsDOMStorage.cpp >+++ b/dom/src/storage/nsDOMStorage.cpp >@@ -52,20 +52,22 @@ > #include "nsReadableUtils.h" > #include "nsIObserverService.h" > #include "nsNetUtil.h" > #include "nsIPrefBranch.h" > #include "nsICookiePermission.h" > #include "nsIPermission.h" > #include "nsIPermissionManager.h" > #include "nsCycleCollectionParticipant.h" > #include "nsIOfflineCacheUpdate.h" > #include "nsIJSContextStack.h" >+#include "nsIEffectiveTLDService.h" >+#include "nsDOMString.h" It doesn't look like you need these two #includes. > PRUint32 perm; > if (permissionManager && >- NS_SUCCEEDED(permissionManager->TestExactPermission(uri, "offline-app", &perm)) && >+ NS_SUCCEEDED(permissionManager->TestPermission(uri, "offline-app", &perm)) && I don't think we want this change. > nsresult > nsDOMStorageManager::Observe(nsISupports *aSubject, > const char *aTopic, > const PRUnichar *aData) > { > if (!strcmp(aTopic, "offline-app-removed")) { > #ifdef MOZ_STORAGE > nsresult rv = nsDOMStorage::InitDB(); > NS_ENSURE_SUCCESS(rv, rv); >- return nsDOMStorage::gStorageDB->RemoveOwner(nsDependentString(aData)); >+ >+ return nsDOMStorage::gStorageDB->RemoveOwner(NS_ConvertUTF16toUTF8(aData), PR_TRUE); I think this should be PR_FALSE - right now the offline app permission is one-host-only, not host-and-subhosts, and removal of offline apps should be treated like that. > nsDOMStorage::nsDOMStorage() >- : mUseDB(PR_FALSE), mSessionOnly(PR_TRUE), mItemsCached(PR_FALSE) >+ : mSessionOnly(PR_TRUE) >+ , mItemsCached(PR_FALSE) >+ , mUseDB(PR_FALSE) > { > mItems.Init(8); > if (nsDOMStorageManager::gStorageManager) > nsDOMStorageManager::gStorageManager->AddToStoragesHash(this); > } > >-nsDOMStorage::nsDOMStorage(nsIURI* aURI, const nsAString& aDomain, PRBool aUseDB) >- : mUseDB(aUseDB), >- mSessionOnly(PR_TRUE), >- mItemsCached(PR_FALSE), >- mURI(aURI), >- mDomain(aDomain) >+nsDOMStorage::nsDOMStorage(nsDOMStorage& aThat) >+ : mSessionOnly(PR_TRUE) >+ , mItemsCached(PR_FALSE) >+ , mURI(aThat.mURI) >+ , mDomain(aThat.mDomain) >+ , mUseDB(PR_FALSE) // Any clone is not using the database >+ , mLocalStorage(PR_FALSE) // Any clone is not a localStorage >+#ifdef MOZ_STORAGE >+ , mDomainDBKey(aThat.mDomainDBKey) >+#endif You changed the order of initializations here, which causes a warning on OSX (at least). > // now have a valid domain, so look it up in the storage table > if (!mStorages.Get(usedDomain, aStorage)) { >- nsCOMPtr<nsIDOMStorage> newstorage = new nsDOMStorage(aURI, usedDomain, PR_TRUE); >- if (!newstorage) >+ nsRefPtr<nsDOMStorage> storage; >+ storage = new nsDOMStorage(); >+ if (!storage) > return NS_ERROR_OUT_OF_MEMORY; > >- if (!mStorages.Put(usedDomain, newstorage)) >+ storage->InitAsGlobalStorage(aURI, usedDomain); >+ >+ if (!mStorages.Put(usedDomain, storage)) > return NS_ERROR_OUT_OF_MEMORY; The fact that the name mStorages is used in two different classes here is kind of confusing... >+ // true if this storage is initialized as localStorage object, it is used >+ // to distinguish placement into the managers local storage hash table >+ // to ensure always the same object for the same origin "true if this storage was initialized as a localStorage object. localStorage objects are scoped to scheme/host/port in the database, while globalStorage objects are scoped just to host." >+ PRPackedBool mLocalStorage; >+ > // true if items from the database are cached > PRPackedBool mItemsCached; > > // the URI this store is associated with > nsCOMPtr<nsIURI> mURI; > >- // domain this store is associated with >- nsString mDomain; >+ // domain key this store is associated with >+ nsCString mDomain; > > // the key->value item pairs > nsTHashtable<nsSessionStorageEntry> mItems; >+ >+ // keys are used for conditioning database queries. >+ // see comments of their getters bellow. "Keys used for database queries. See comments on the getters below." >+ // The domain this storage is associated with, >+ // e.g. "foo.bar.com" >+ nsCString& GetDomain() {return mDomain;} >+ >+ // e.g. "moc.rab.oof." or "moc.rab.oof.:http:80" depending >+ // on association with a domain (globalStorage) or >+ // an origin (localStorage). >+ nsCString& GetDomainDBKey() {return mDomainDBKey;} >+ >+ // e.g. "moc.rab.%" - reversed eTLD+1 subpart of the domain or >+ // (in future) reversed offline application allowed domain. >+ nsCString& GetQuotaDomainDBKey() {return mQuotaDomainDBKey;} You don't explain (as promised above) how these keys are used in queries. > class nsDOMStorageItem : public nsIDOMStorageItem, > public nsIDOMToString > { > public: > nsDOMStorageItem(nsDOMStorage* aStorage, > const nsAString& aKey, > const nsAString& aValue, > PRBool aSecure); >diff --git a/dom/src/storage/nsDOMStorageDB.cpp b/dom/src/storage/nsDOMStorageDB.cpp >--- a/dom/src/storage/nsDOMStorageDB.cpp >+++ b/dom/src/storage/nsDOMStorageDB.cpp >@@ -34,25 +34,66 @@ > * the provisions above, a recipient may use your version of this file under > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #include "nsCOMPtr.h" > #include "nsDOMError.h" > #include "nsDOMStorage.h" > #include "nsDOMStorageDB.h" > #include "nsIFile.h" >+#include "nsIVariant.h" >+#include "nsIEffectiveTLDService.h" > #include "nsAppDirectoryServiceDefs.h" > #include "mozStorageCID.h" > #include "mozStorageHelper.h" > #include "mozIStorageService.h" > #include "mozIStorageValueArray.h" >+#include "nsPrintfCString.h" >+#include "nsNetUtil.h" >+ >+NS_IMPL_ISUPPORTS1(nsDOMStorageDB::nsReverseStringFunction, mozIStorageFunction) You have string-reversing logic in two places, can you use a helper function? >+NS_IMETHODIMP nsDOMStorageDB::nsReverseStringFunction::OnFunctionCall( >+ mozIStorageValueArray *aFunctionArguments, nsIVariant **_retval) >+{ Newline after NS_IMETHODIMP. >+ nsresult rv; >+ >+ nsCAutoString stringToReverse; >+ rv = aFunctionArguments->GetUTF8String(0, stringToReverse); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsACString::const_iterator sb, se; >+ stringToReverse.BeginReading(sb); >+ stringToReverse.EndReading(se); Could you call these "begin" and "end" please? >+ nsCAutoString result; >+ result.SetLength(stringToReverse.Length()); >+ nsACString::iterator db; >+ result.EndWriting(db); and something better for this too. > nsresult > nsDOMStorageDB::Init() >+ >+ rv = mConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING( >+ "CREATE UNIQUE INDEX IF NOT EXISTS domain_key_index" >+ " ON webappsstore(domain, key)")); >+ >+ PRBool duplicateDomainKeyCombos = NS_FAILED(rv); >+ if (duplicateDomainKeyCombos) { >+ // if the index can't be created, there are dup domain/key combos >+ // in webappsstore, which indicates a bug elsewhere. Fail to upgrade >+ // in this case and attempt to drop this index even it failed to create. So you not only fail to upgrade, but you're left with (broken) duplicate data, and the index is never created. Is that OK? Do we need this index? If not, why do we keep it around in the common case? > nsresult >-nsDOMStorageDB::SetSecure(const nsAString& aDomain, >+nsDOMStorageDB::SetSecure(nsDOMStorage* aStorage, > const nsAString& aKey, > const PRBool aSecure) > { >- mozStorageStatementScoper scope(mGetKeyValueStatement); >+ nsresult rv; > >- nsresult rv = mGetKeyValueStatement->BindStringParameter(0, aDomain); >+ mozStorageStatementScoper scope(mSetSecureStatement); >+ >+ rv = mSetSecureStatement->BindInt32Parameter(0, aSecure ? 1 : 0); > NS_ENSURE_SUCCESS(rv, rv); >- rv = mGetKeyValueStatement->BindStringParameter(1, aKey); >+ rv = mSetSecureStatement->BindUTF8StringParameter(1, aStorage->GetDomainDBKey()); >+ NS_ENSURE_SUCCESS(rv, rv); >+ rv = mSetSecureStatement->BindStringParameter(2, aKey); > NS_ENSURE_SUCCESS(rv, rv); > >- PRBool exists; >- rv = mGetKeyValueStatement->ExecuteStep(&exists); >- NS_ENSURE_SUCCESS(rv, rv); >- >- if (exists) { >- mGetKeyValueStatement->Reset(); >- >- mozStorageStatementScoper scopeupdate(mUpdateKeyStatement); >- >- rv = mSetSecureStatement->BindInt32Parameter(0, aSecure ? 1 : 0); >- NS_ENSURE_SUCCESS(rv, rv); >- rv = mSetSecureStatement->BindStringParameter(1, aDomain); >- NS_ENSURE_SUCCESS(rv, rv); >- rv = mSetSecureStatement->BindStringParameter(2, aKey); >- NS_ENSURE_SUCCESS(rv, rv); >- >- return mSetSecureStatement->Execute(); >- } >- >- return NS_OK; >+ return mSetSecureStatement->Execute(); > } I assume here that you're relying on the newly created domain/key index to avoid duplicates (which I suppose answers my question above). If so, you'll need to update the statement to include something like OR REPLACE. > nsresult >-nsDOMStorageDB::RemoveOwners(const nsStringArray &aOwners, PRBool aMatch) >+nsDOMStorageDB::RemoveOwners(const nsStringArray &aOwners, PRBool aIncludeSubDomains, PRBool aMatch) > { > if (aOwners.Count() == 0) { > if (aMatch) { > return NS_OK; > } > > return RemoveAll(); > } > >- nsCAutoString expression; >+ // Using nsString here because it is going to be very long >+ nsCString expression; > > if (aMatch) { > expression.Assign(NS_LITERAL_CSTRING("DELETE FROM webappsstore " >- "WHERE owner IN (?")); >+ "WHERE domain IN (")); > } else { > expression.Assign(NS_LITERAL_CSTRING("DELETE FROM webappsstore " >- "WHERE owner NOT IN (?")); >+ "WHERE domain NOT IN (")); > } > >- for (PRInt32 i = 1; i < aOwners.Count(); i++) { >- expression.Append(", ?"); >+ // Using Append(NS_LITERAL_CSTRING()) because >+ // it is faster then AppendLiteral >+ for (PRInt32 i = 0; i < aOwners.Count(); i++) { >+ if (i) >+ expression.Append(NS_LITERAL_CSTRING(" UNION ")); >+ >+ expression.Append(NS_LITERAL_CSTRING( >+ "SELECT DISTINCT domain FROM webappsstore WHERE domain LIKE ?")); > } >- expression.Append(")"); >+ expression.Append(NS_LITERAL_CSTRING(");")); > > nsCOMPtr<mozIStorageStatement> statement; > > nsresult rv = mConnection->CreateStatement(expression, > getter_AddRefs(statement)); > NS_ENSURE_SUCCESS(rv, rv); > > for (PRInt32 i = 0; i < aOwners.Count(); i++) { >- rv = statement->BindStringParameter(i, *aOwners[i]); >+ nsCAutoString quotaKey; >+ rv = nsDOMStorageDB::CreateDomainDBKey(NS_ConvertUTF16toUTF8(*aOwners[i]), quotaKey); >+ >+ if (aIncludeSubDomains) >+ quotaKey.AppendLiteral("%"); >+ Does '%' work as expected in this style of query? (WHERE foo IN (?, ?)) >+nsresult >+nsDOMStorageDB::CreateQuotaDomainDBKey(const nsACString& aAsciiDomain, >+ PRBool aIncludeSubDomains, nsACString& aKey) >+{ >+ nsresult rv; >+ >+ nsCOMPtr<nsIEffectiveTLDService> eTLDService(do_GetService( >+ NS_EFFECTIVETLDSERVICE_CONTRACTID, &rv)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIURI> uri; >+ rv = NS_NewURI(getter_AddRefs(uri), NS_LITERAL_CSTRING("http://") + aAsciiDomain); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCAutoString eTLDplusOne; >+ rv = eTLDService->GetBaseDomain(uri, 0, eTLDplusOne); >+ if (NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS == rv) { >+ // XXX - what to do for localhost exactly? Add a bug number here please. >diff --git a/dom/src/storage/nsDOMStorageDB.h b/dom/src/storage/nsDOMStorageDB.h >+ class nsReverseStringFunction : public mozIStorageFunction >+ { >+ NS_DECL_ISUPPORTS >+ NS_DECL_MOZISTORAGEFUNCTION >+ }; I don't think this class needs to be in the header, it can just be fully specified in nsDOMStorageDB.cpp. >+ >+ /** >+ * Creates a database key used to select records belonging >+ * to a particular origin ("schema://domain:port") that when >+ * appended a '%' can be used for efficient LIKE quries to >+ * search also any subdomain. Please, see nsDOMStorageDB::Init >+ * update from schema version 0. >+ */ >+ >+ /** >+ * Turns "http://foo.bar.com:80" to "moc.rab.oof.:http:80", >+ * i.e. reverses the host, appends a dot, appends the schema >+ * and a port number. >+ */ >+ static nsresult CreateOriginDBKey(nsIURI* aUri, nsACString& aKey); >+ >+ /** >+ * Turns "http://foo.bar.com" to "moc.rab.oof.", >+ * i.e. reverses the host and appends a dot. >+ */ >+ static nsresult CreateDomainDBKey(nsIURI* aUri, nsACString& aKey); >+ static nsresult CreateDomainDBKey(const nsACString& aAsciiDomain, nsACString& aKey); >+ >+ /** >+ * Turns "foo.bar.com" to "moc.rab.", >+ * i.e. extracts eTLD+1 from the host, reverses the result >+ * and appends a dot. >+ */ >+ static nsresult CreateQuotaDomainDBKey(const nsACString& aAsciiDomain, >+ PRBool aIncludeSubDomains, nsACString& aKey); So yeah, I think somewhere prominent (maybe at the top of nsDOMStorage.h, but not buried in schema updating code) we need a good description of the host/quota keys, what they look like, and how they're used, with examples). >+ nsresult >+ GetUsageInrernal(const nsACString& aQuotaDomainDBKey, PRInt32 *aUsage); Should be GetUsageInternal()
Assignee | ||
Comment 25•16 years ago
|
||
> > + > > >+ rv = mConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING( > > >+ "CREATE UNIQUE INDEX IF NOT EXISTS domain_key_index" > > >+ " ON webappsstore(domain, key)")); > > >+ > > >+ PRBool duplicateDomainKeyCombos = NS_FAILED(rv); > > >+ if (duplicateDomainKeyCombos) { > > >+ // if the index can't be created, there are dup domain/key combos > > >+ // in webappsstore, which indicates a bug elsewhere. Fail to upgrade > > >+ // in this case and attempt to drop this index even it failed to create. > > So you not only fail to upgrade, but you're left with (broken) duplicate data, > and the index is never created. Is that OK? Do we need this index? > > If not, why do we keep it around in the common case? > > This preserves the same behavior as the original code. When creation of the unique index fails it indicates the data are in inconsistent state and we don't want to upgrade in that case. I need the new unique index as a search indicia so I decided to left it. > I assume here that you're relying on the newly created domain/key index to > avoid duplicates (which I suppose answers my question above). > As above. > If so, you'll need to update the statement to include something like OR > REPLACE. > Yes, if we don't want to fail on duplicate insertion (what is always better then to unexpectidly fail the process). Updated. > Does '%' work as expected in this style of query? (WHERE foo IN (?, ?)) I do exactly this: DELETE FROM webappsstore WHERE domain IN ( SELECT DISTINCT domain FROM webappsstore WHERE domain LIKE 'xyz.%' UNION SELECT DISTINCT domain FROM webappsstore WHERE domain LIKE 'abc.%' UNION ... )
Attachment #337695 -
Attachment is obsolete: true
Attachment #337942 -
Flags: review?(dcamp)
Comment 26•16 years ago
|
||
Some drive-by comments: 1) Please try to use NS_SecurityCompareURIs as little as possible; prefer to use the appropriate CAPS APIs, best ones that do principals, not URIs. Note bug 327243, which we don't want to be undoing. 2) NS_GetOriginFromURI seems to be duplicating (except incorrectly, in several different ways) existing security manager code. If we need that code elsewhere, we need to either expose it (though I thought principals already had a GetOrigin?) or refactor it into a shared location. 3) Can we _please_ stop this movement back towards using URIs (or worse yet, strings) as security contexts? It's very bug-prone. If we need better CAPS APIs, let's create better CAPS APIs instead of working around their lack by sprinkling security-bug-prone copy/paste pixie dust about?
Assignee | ||
Comment 27•16 years ago
|
||
I have spent some time trying to fix comment 26. I need an advice from BZ or someone who understand principals flow among doc shells. I suspect we will have to move the code carrying/cloning session storage to a different place. It seems that at least in global window the target principal is not know at that moment. Therefor I suggest to revert this change from the patch and submit a follow up bug where we fix session storage mapping; it is not necessary to complete this particular bug, I just tough it would be an easy fix. BZ, do you agree?
Comment 28•16 years ago
|
||
Fundamentally you do not have a principal for something you have not loaded yet (since the principal identifies what the thing's real security context is). Any already-loaded content will have a principal. If the host/port/scheme stuff is not needed to fix this bug, I'd very much support spinning it off into a different bug.
Assignee | ||
Comment 29•16 years ago
|
||
- removed session storage mapping by origin - changed database update as discussed with dcamp on irc
Attachment #337942 -
Attachment is obsolete: true
Attachment #338213 -
Flags: review?(dcamp)
Attachment #337942 -
Flags: review?(dcamp)
Reporter | ||
Comment 30•16 years ago
|
||
Can we split the changes to session storage out into a separate patch too please?
Assignee | ||
Comment 31•16 years ago
|
||
Removed sessionStorage movement to nsIDOMStorage2. I will submit a follow up.
Attachment #338213 -
Attachment is obsolete: true
Attachment #338369 -
Flags: review?(dcamp)
Attachment #338213 -
Flags: review?(dcamp)
Reporter | ||
Comment 32•16 years ago
|
||
Comment on attachment 338369 [details] [diff] [review] v7 >diff --git a/dom/public/idl/storage/nsIDOMStorage2.idl b/dom/public/idl/storage/nsIDOMStorage2.idl >new file mode 100644 >--- /dev/null >+++ b/dom/public/idl/storage/nsIDOMStorage2.idl >+ * The Initial Developer of the Original Code is >+ * Neil Deakin <enndeakin@sympatico.ca> >+ * Portions created by the Initial Developer are Copyright (C) 2006 >+ * the Initial Developer. All Rights Reserved. >+ * >+ * Contributor(s): This header information is incorrect. >+/** >+ * Interface for client side storage. See >+ * http://www.whatwg.org/specs/web-apps/current-work/multipage/structured.html#storage0 >+ * for more information. >+ * >+ * A storage object stores an arbitrary set of key-value pairs, which >+ * may be retrieved, modified and removed as needed. A key may only >+ * exist once within a storage object, and only one value may be >+ * associated with a particular key. Keys are stored in a particular >+ * order with the condition that this order not change by merely changing >+ * the value associated with a key, but the order may change when a >+ * key is added or removed. >+ */ Is ordering currently preserved as required? If not, we should file a followup. >diff --git a/dom/src/base/nsGlobalWindow.cpp b/dom/src/base/nsGlobalWindow.cpp >--- a/dom/src/base/nsGlobalWindow.cpp >+++ b/dom/src/base/nsGlobalWindow.cpp > >+NS_IMETHODIMP >+nsGlobalWindow::GetLocalStorage(nsIDOMStorage2 ** aLocalStorage) >+{ >+ NS_ENSURE_ARG(aLocalStorage); >+ >+ if (!mLocalStorage) { >+ *aLocalStorage = nsnull; >+ >+ nsresult rv; >+ >+ nsIPrincipal *principal = GetPrincipal(); >+ if (!principal) { >+ return NS_OK; >+ } >+ >+ nsCOMPtr<nsIURI> codebase; >+ rv = principal->GetURI(getter_AddRefs(codebase)); >+ if (NS_FAILED(rv) || !codebase) >+ return NS_FAILED(rv) ? rv : NS_ERROR_DOM_NOT_SUPPORTED_ERR; Maybe just an NS_ENSURE_SUCCESS() and then if (!codebase) { } ? >diff --git a/dom/src/storage/nsDOMStorage.cpp b/dom/src/storage/nsDOMStorage.cpp >+NS_IMETHODIMP >+nsDOMStorageManager::GetLocalStorageForURI(nsIURI *aURI, >+ nsIDOMStorage2 **_retval) >+{ >+ *_retval = nsnull; The style in this file would be aResult rather than _retval. Maybe AddToStoragesHash() and RemoveFromStoragesHash() should be AddStorage() and RemoveStorage(), for harmony with AddLocalStorage() ? > NS_IMETHODIMP >+nsDOMStorage::GetItem(const nsAString& aKey, nsAString &aData) >+{ >+ nsresult rv; >+ >+ nsCOMPtr<nsIDOMStorageItem> item; >+ rv = GetItem(aKey, getter_AddRefs(item)); >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ if (item) { >+ rv = item->GetValue(aData); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ else >+ SetDOMStringToNull(aData); >+ >+ return NS_OK; >+} I believe this method needs a CacheStoragePermissions() call like the one that returns an nsIDOMStorageItem. >diff --git a/dom/src/storage/nsDOMStorage.h b/dom/src/storage/nsDOMStorage.h >+ // nsIDOMStorage2 >+ // This interface is quit the same as nsIDOMStorage except GetItem >+ // method that returns DOMString instead of nsIDOMStorageItem. >+ // This way of inheritance preserves original interface unchanged, provides >+ // a new one with just a single method changed and in very cheap way implement >+ // identical methods from those two interfaces in the same class - nsDOMStorage. >+ // Therefor the only one method different from nsIDOMStorage is listed here. >+ NS_SCRIPTABLE NS_IMETHOD GetItem(const nsAString & key, nsAString & _retval NS_OUTPARAM); This comment has spelling and wrapping errors. >+ // keys are used for database queries. >+ // see comments of the getters bellow. >+ nsCString mDomainDBKey; >+ nsCString mQuotaDomainDBKey; >+ >+ nsCString mDomainHashKey; >+ >+ friend class nsDOMStorageDB; >+ >+ // The domain this storage is associated with, >+ // e.g. "foo.bar.com" >+ nsCString& GetDomain() {return mDomain;} >+ >+ // e.g. "moc.rab.oof." or "moc.rab.oof.:http:80" depending >+ // on association with a domain (globalStorage) or >+ // an origin (localStorage). >+ nsCString& GetDomainDBKey() {return mDomainDBKey;} >+ >+ // e.g. "moc.rab.%" - reversed eTLD+1 subpart of the domain or >+ // (in future) reversed offline application allowed domain. >+ nsCString& GetQuotaDomainDBKey() {return mQuotaDomainDBKey;} Any reason not to just make these public? They're still private to the module, and the db is the only other thing in the module... >diff --git a/dom/src/storage/nsDOMStorageDB.cpp b/dom/src/storage/nsDOMStorageDB.cpp >--- a/dom/src/storage/nsDOMStorageDB.cpp >+++ b/dom/src/storage/nsDOMStorageDB.cpp >+static >+void NS_ReverseString(const nsCSubstring& source, nsCSubstring& result) I don't think we want the NS_ prefix for a private static fn. >+{ >+ nsACString::const_iterator sourceBegin, sourceEnd; >+ source.BeginReading(sourceBegin); >+ source.EndReading(sourceEnd); >+ >+ result.SetLength(source.Length()); >+ nsACString::iterator destEnd; >+ result.EndWriting(destEnd); >+ >+ while (sourceBegin != sourceEnd) >+ { >+ *(--destEnd) = *sourceBegin; >+ ++sourceBegin; >+ } Rest of this file puts opening brace on the same line. This is wrong in a few places in this file. >+class nsReverseStringSQLFunction : public mozIStorageFunction >+{ >+ NS_DECL_ISUPPORTS >+ NS_DECL_MOZISTORAGEFUNCTION >+}; >+ >+NS_IMPL_ISUPPORTS1(nsReverseStringSQLFunction, mozIStorageFunction) >+ >+NS_IMETHODIMP >+nsReverseStringSQLFunction::OnFunctionCall( >+ mozIStorageValueArray *aFunctionArguments, nsIVariant **_retval) >+{ >+ nsCOMPtr<nsIWritableVariant> outVar(do_CreateInstance( >+ NS_VARIANT_CONTRACTID, &rv)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = outVar->SetAsAUTF8String(result); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ NS_ADDREF(*_retval = outVar); >+ return NS_OK; You can save a ref/addref pair here with *_retval = outVar.forget(). (also rename _retval as per the style of the file please). >+ // Check if there is storage of Gecko 1.9.0 and if so, >+ // upgrade that storage to actual webappsstore2 table >+ // and drop the obsolete table. >+ rv = mConnection->TableExists(NS_LITERAL_CSTRING("webappsstore"), >+ &exists); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ if (exists) { > // create a temporary index to handle dup checking >- rv = mConnection->ExecuteSimpleSQL( >+ rv = mConnection->ExecuteSimpleSQL( > NS_LITERAL_CSTRING("CREATE UNIQUE INDEX webappsstore_tmp " > " ON webappsstore(domain, key)")); You don't need this index anymore, the index on the new table (and the INSERT OR IGNORE) will take care of it. >- nsCAutoString expression; >+ // Using nsString here because it is going to be very long >+ nsCString expression; > > if (aMatch) { > expression.Assign(NS_LITERAL_CSTRING("DELETE FROM webappsstore " >- "WHERE owner IN (?")); >+ "WHERE domain IN (")); > } else { > expression.Assign(NS_LITERAL_CSTRING("DELETE FROM webappsstore " >- "WHERE owner NOT IN (?")); >+ "WHERE domain NOT IN (")); > } > >- for (PRInt32 i = 1; i < aOwners.Count(); i++) { >- expression.Append(", ?"); >+ // Using Append(NS_LITERAL_CSTRING()) because >+ // it is faster then AppendLiteral >+ for (PRInt32 i = 0; i < aOwners.Count(); i++) { >+ if (i) >+ expression.Append(NS_LITERAL_CSTRING(" UNION ")); >+ >+ expression.Append(NS_LITERAL_CSTRING( >+ "SELECT DISTINCT domain FROM webappsstore WHERE domain LIKE ?")); > } >- expression.Append(")"); >+ expression.Append(NS_LITERAL_CSTRING(");")); > > nsCOMPtr<mozIStorageStatement> statement; > > nsresult rv = mConnection->CreateStatement(expression, > getter_AddRefs(statement)); > NS_ENSURE_SUCCESS(rv, rv); > > for (PRInt32 i = 0; i < aOwners.Count(); i++) { >- rv = statement->BindStringParameter(i, *aOwners[i]); >+ nsCAutoString quotaKey; >+ rv = nsDOMStorageDB::CreateDomainDBKey(NS_ConvertUTF16toUTF8(*aOwners[i]), quotaKey); >+ >+ if (aIncludeSubDomains) >+ quotaKey.AppendLiteral("%"); >+ >+ rv = statement->BindUTF8StringParameter(i, quotaKey); > NS_ENSURE_SUCCESS(rv, rv); > } > >- return statement->Execute(); >+ rv = statement->Execute(); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ return NS_OK; > } OK, now that you've explained this statement - it sounds like it could be ridiculously expensive. Have you done any performance testing? Do we need the behavior (deleting subdomains) to get localStorage support? If not, can we file that behavior as a followup? >diff --git a/dom/src/storage/nsDOMStorageDB.h b/dom/src/storage/nsDOMStorageDB.h >--- a/dom/src/storage/nsDOMStorageDB.h >+++ b/dom/src/storage/nsDOMStorageDB.h >+/** >+ * Usage of webappsstore.domain database column: >+ * From now on this column has to distinguish between local storage and >+ * global storage data. On bases of this change we also managed to rethink >+ * the way how quota bound to a domain is count for data belonging to >+ * a particular domain and to all its potential subdomains. In conjuntion >+ * of these two requirements the 'domain' column now contains reversed >+ * host name (the domain) of the storage that holds the data. For example: >+ * for 'foo.bar.com' the value stored to database is 'moc.rab.oof.'. When >+ * looking up a value of key with specific name belonging to 'foo.bar.com' >+ * domain we do query like [1]: I would rewrite this something like this: ==== For the purposes of quota checking, we want to be able to efficiently reference data items that belong to a host or its subhosts. We do this by using a reversed domain name as the key for an item. For example, a storage for foo.bar.com would use a key of 'moc.rab.oof.". Additionally, globalStorage and localStorage items must be distinguished. globalStorage items are scoped to the host, and localStorage are items are scoped to the scheme/host/port. To scope localStorage data, its port and scheme are appended to its key. http://foo.bar.com is stored as moc.rab.foo.:http:80. So the following queries can be used, for http://foo.bar.com: All data owned by globalStorage["foo.bar.com"] -> SELECT * WHERE Domain = "moc.rab.foo.:" All data owned by localStorage -> SELECT * WHERE Domain = "moc.rab.foo.:http:80" All data owned by foo.bar.com, in any storage -> SELECT * WHERE Domain LIKE "moc.rab.foo.:%" All data owned by foo.bar.com or any subdomain, in any storage -> SELECT * WHERE Domain LIKE "moc.rab.foo.%". This key is called the "domain key" throughout the code. So the domain key for localStorage at http://foo.bar.com is "moc.rab.foo.:http:80". When calculating quotas, we want to lump together everything in an ETLD+1. So we use a "quota key" during lookups to calculate the quota. So the quota key for localStorage at http://foo.bar.com is "moc.rab.". ====== Note that I made one small semantic change here - adding a ":" to the end of the domain key for a globalStorage would let us reference globalStorage separately from localStorage. I don't know how important this is, so feel free to either make that change or remove that bit from the comment. >+ /** >+ * Remove all keys belonging exactly and only >+ * to this storage. >+ */ I think "Remove all keys belonging to this storage" is good enough. >+ nsresult >+ ClearStorage(nsDOMStorage* aStorage); > > /** > * Removes all keys added by a given domain. > */ > nsresult >- RemoveOwner(const nsAString& aOwner); >+ RemoveOwner(const nsACString& aOwner, PRBool aIncludeSubDomains); > > /** > * Removes keys owned by domains that either match or don't match the > * list. > */ > nsresult >- RemoveOwners(const nsStringArray& aOwners, PRBool aMatch); >+ RemoveOwners(const nsStringArray& aOwners, PRBool aIncludeSubDomains, PRBool aMatch); > > /** > * Removes all keys from storage. Used when clearing storage. > */ > nsresult > RemoveAll(); > >- nsresult GetUsage(const nsAString &aOwner, PRInt32 *aUsage); >+ /** >+ * Returns usage for a storage using its GetQuotaDomainDBKey() as a key. >+ */ >+ nsresult >+ GetUsage(nsDOMStorage* aStorage, PRInt32 *aUsage); >+ >+ /** >+ * Returns usage of the domain and optionaly by any subdomain. >+ */ >+ nsresult >+ GetUsage(const nsACString& aDomain, PRBool aIncludeSubDomains, PRInt32 *aUsage); >+ >+ /** >+ * Creates a database key used to select records belonging >+ * to a particular origin ("schema://domain:port") that when >+ * appended a '%' can be used for efficient LIKE quries to >+ * search also any subdomain. Please, see nsDOMStorageDB::Init >+ * update from schema version 0. >+ */ quries->queries, and the explanation is now in this file.
Assignee | ||
Comment 33•16 years ago
|
||
> Is ordering currently preserved as required? If not, we should file a > followup. It should be preserved. I will file a follow up. > Maybe AddToStoragesHash() and RemoveFromStoragesHash() should be AddStorage() > and RemoveStorage(), for harmony with AddLocalStorage() ? AddStorage is for any type of storage, AddLocalStorage is just for the local storage type. So, IMHO, the names are good as are now. > I believe this method needs a CacheStoragePermissions() call like the one that > returns an nsIDOMStorageItem. Added a comment to the code about it. > OK, now that you've explained this statement - it sounds like it could be > ridiculously expensive. Have you done any performance testing? Do we need the > behavior (deleting subdomains) to get localStorage support? If not, can we > file that behavior as a followup? I changed all LIKE 'XX%' to GLOB 'XX*'. Now all statements, including the above mentioned, are using index. Also there is no need to have index on just the domain (now scope) column because scope+key pair index is sufficient and used in all cases. I added the tests to this patch to have it merged together. Fixed crash in the new Clear() method.
Attachment #337737 -
Attachment is obsolete: true
Attachment #338369 -
Attachment is obsolete: true
Attachment #338885 -
Flags: review?(dcamp)
Attachment #337737 -
Flags: review?(dcamp)
Attachment #338369 -
Flags: review?(dcamp)
Reporter | ||
Updated•16 years ago
|
Attachment #338885 -
Flags: review?(dcamp) → review-
Reporter | ||
Comment 34•16 years ago
|
||
Comment on attachment 338885 [details] [diff] [review] v8 >+ * Portions created by the Initial Developer are Copyright (C) 2006 >+ * the Initial Developer. All Rights Reserved. Need to update the copyright date. >diff --git a/dom/src/storage/nsDOMStorage.cpp b/dom/src/storage/nsDOMStorage.cpp >--- a/dom/src/storage/nsDOMStorage.cpp >+++ b/dom/src/storage/nsDOMStorage.cpp > NS_IMETHODIMP >+nsDOMStorage::GetItem(const nsAString& aKey, nsAString &aData) >+{ >+ nsresult rv; >+ >+ // IMPORTANT: >+ // CacheStoragePermissions() is called inside of >+ // GetItem(nsAString, nsIDOMStorageItem) >+ // To call it particularly in this method would just duplicate >+ // the call. If the code changes, make sure that call to >+ // CacheStoragePermissions() is put here! >+ >+ nsCOMPtr<nsIDOMStorageItem> item; >+ rv = GetItem(aKey, getter_AddRefs(item)); >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ if (item) { >+ rv = item->GetValue(aData); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ else >+ SetDOMStringToNull(aData); >+ >+ return NS_OK; >+} >+ >+NS_IMETHODIMP > nsDOMStorage::GetItem(const nsAString& aKey, nsIDOMStorageItem **aItem) > { > *aItem = nsnull; > > if (!CacheStoragePermissions()) > return NS_ERROR_DOM_SECURITY_ERR; > > if (aKey.IsEmpty()) > return NS_OK; > >@@ -621,22 +765,21 @@ nsDOMStorage::GetItem(const nsAString& a > > if (entry) { > if (!IsCallerSecure() && entry->mItem->IsSecure()) { > return NS_OK; > } > NS_ADDREF(*aItem = entry->mItem); > } > else if (UseDB()) { > PRBool secure; > nsAutoString value; >- nsAutoString unused; >- nsresult rv = GetDBValue(aKey, value, &secure, unused); >+ nsresult rv = GetDBValue(aKey, value, &secure); > // return null if access isn't allowed or the key wasn't found > if (rv == NS_ERROR_DOM_SECURITY_ERR || rv == NS_ERROR_DOM_NOT_FOUND_ERR) > return NS_OK; > NS_ENSURE_SUCCESS(rv, rv); > > nsRefPtr<nsDOMStorageItem> newitem = > new nsDOMStorageItem(this, aKey, value, secure); > if (!newitem) > return NS_ERROR_OUT_OF_MEMORY; > >@@ -711,45 +854,82 @@ NS_IMETHODIMP nsDOMStorage::RemoveItem(c > return NS_ERROR_DOM_SECURITY_ERR; > } > > if (UseDB()) { > #ifdef MOZ_STORAGE > nsresult rv = InitDB(); > NS_ENSURE_SUCCESS(rv, rv); > > nsAutoString value; > PRBool secureItem; >- nsAutoString owner; >- rv = GetDBValue(aKey, value, &secureItem, owner); >+ rv = GetDBValue(aKey, value, &secureItem); > if (rv == NS_ERROR_DOM_NOT_FOUND_ERR) > return NS_OK; > NS_ENSURE_SUCCESS(rv, rv); > >- rv = gStorageDB->RemoveKey(mDomain, aKey, owner, >+ rv = gStorageDB->RemoveKey(this, aKey, > aKey.Length() + value.Length()); > NS_ENSURE_SUCCESS(rv, rv); > > mItemsCached = PR_FALSE; > > BroadcastChangeNotification(); > #endif > } > else if (entry) { > // clear string as StorageItems may be referencing this item > entry->mItem->ClearValue(); > > BroadcastChangeNotification(); > } > > if (entry) { > mItems.RawRemoveEntry(entry); > } >+ >+ return NS_OK; >+} >+ >+PR_STATIC_CALLBACK(PLDHashOperator) >+CheckSecure(nsSessionStorageEntry* aEntry, void* userArg) >+{ >+ PRBool* secure = (PRBool*)userArg; >+ *secure |= aEntry->mItem->IsSecure(); >+ >+ return PL_DHASH_NEXT; >+} >+ >+NS_IMETHODIMP >+nsDOMStorage::Clear() >+{ >+ if (UseDB()) >+ CacheKeysFromDB(); >+ >+ PRBool foundSecureItem = PR_FALSE; >+ mItems.EnumerateEntries(CheckSecure, &foundSecureItem); >+ >+ if (foundSecureItem && !IsCallerSecure()) { >+ return NS_ERROR_DOM_SECURITY_ERR; >+ } >+ >+#ifdef MOZ_STORAGE >+ if (UseDB()) { >+ nsresult rv = InitDB(); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = gStorageDB->ClearStorage(this); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+#endif >+ >+ mItems.Clear(); >+ BroadcastChangeNotification(); > > return NS_OK; > } > > nsresult > nsDOMStorage::InitDB() > { > #ifdef MOZ_STORAGE > if (!gStorageDB) { > gStorageDB = new nsDOMStorageDB(); >@@ -777,45 +957,45 @@ nsDOMStorage::CacheKeysFromDB() > nsDOMStorage::CacheKeysFromDB() > { > #ifdef MOZ_STORAGE > // cache all the keys in the hash. This is used by the Length and Key methods > // use this cache for better performance. The disadvantage is that the > // order may break if someone changes the keys in the database directly. > if (!mItemsCached) { > nsresult rv = InitDB(); > NS_ENSURE_SUCCESS(rv, rv); > >- rv = gStorageDB->GetAllKeys(mDomain, this, &mItems); >+ rv = gStorageDB->GetAllKeys(this, &mItems); > NS_ENSURE_SUCCESS(rv, rv); > > mItemsCached = PR_TRUE; > } > #endif > > return NS_OK; > } > > nsresult > nsDOMStorage::GetDBValue(const nsAString& aKey, nsAString& aValue, >- PRBool* aSecure, nsAString& aOwner) >+ PRBool* aSecure) > { > aValue.Truncate(); > > #ifdef MOZ_STORAGE > if (!UseDB()) > return NS_OK; > > nsresult rv = InitDB(); > NS_ENSURE_SUCCESS(rv, rv); > > nsAutoString value; >- rv = gStorageDB->GetKeyValue(mDomain, aKey, value, aSecure, aOwner); >+ rv = gStorageDB->GetKeyValue(this, aKey, value, aSecure); > if (NS_FAILED(rv)) > return rv; > > if (!IsCallerSecure() && *aSecure) { > return NS_ERROR_DOM_SECURITY_ERR; > } > > aValue.Assign(value); > #endif > >@@ -835,55 +1015,52 @@ nsDOMStorage::SetDBValue(const nsAString > NS_ENSURE_SUCCESS(rv, rv); > > // Get the current domain for quota enforcement > nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager(); > if (!ssm) > return NS_ERROR_FAILURE; > > nsCOMPtr<nsIPrincipal> subjectPrincipal; > ssm->GetSubjectPrincipal(getter_AddRefs(subjectPrincipal)); > >- nsAutoString currentDomain; >+ nsCAutoString currentDomain; > > if (subjectPrincipal) { > nsCOMPtr<nsIURI> uri; > rv = subjectPrincipal->GetURI(getter_AddRefs(uri)); > > if (NS_SUCCEEDED(rv) && uri) { > nsCOMPtr<nsIURI> innerUri = NS_GetInnermostURI(uri); > if (!innerUri) > return NS_ERROR_UNEXPECTED; > >- nsCAutoString currentDomainAscii; >- innerUri->GetAsciiHost(currentDomainAscii); >- currentDomain = NS_ConvertUTF8toUTF16(currentDomainAscii); >+ innerUri->GetAsciiHost(currentDomain); > } > > if (currentDomain.IsEmpty()) { > // allow chrome urls and trusted file urls to write using > // the storage's domain > if (nsContentUtils::IsCallerTrustedForWrite()) > currentDomain = mDomain; > else > return NS_ERROR_DOM_SECURITY_ERR; > } > } else { > currentDomain = mDomain; > } > > PRInt32 quota; > PRInt32 warnQuota; > GetQuota(currentDomain, "a, &warnQuota); > > PRInt32 usage; >- rv = gStorageDB->SetKey(mDomain, aKey, aValue, aSecure, >- currentDomain, quota, &usage); >+ rv = gStorageDB->SetKey(this, aKey, aValue, aSecure, quota, &usage); > NS_ENSURE_SUCCESS(rv, rv); > > mItemsCached = PR_FALSE; > > if (warnQuota >= 0 && usage > warnQuota) { > // try to include the window that exceeded the warn quota > nsCOMPtr<nsIDOMWindow> window; > JSContext *cx; > nsCOMPtr<nsIJSContextStack> stack = > do_GetService("@mozilla.org/js/xpc/ContextStack;1"); >@@ -891,38 +1068,38 @@ nsDOMStorage::SetDBValue(const nsAString > nsCOMPtr<nsIScriptContext> scriptContext; > scriptContext = GetScriptContextFromJSContext(cx); > if (scriptContext) { > window = do_QueryInterface(scriptContext->GetGlobalObject()); > } > } > > nsCOMPtr<nsIObserverService> os = > do_GetService("@mozilla.org/observer-service;1"); > os->NotifyObservers(window, "dom-storage-warn-quota-exceeded", >- currentDomain.get()); >+ NS_ConvertUTF8toUTF16(currentDomain).get()); > } > > BroadcastChangeNotification(); > #endif > > return NS_OK; > } > > nsresult > nsDOMStorage::SetSecure(const nsAString& aKey, PRBool aSecure) > { > #ifdef MOZ_STORAGE > if (UseDB()) { > nsresult rv = InitDB(); > NS_ENSURE_SUCCESS(rv, rv); > >- return gStorageDB->SetSecure(mDomain, aKey, aSecure); >+ return gStorageDB->SetSecure(this, aKey, aSecure); > } > #else > return NS_ERROR_NOT_IMPLEMENTED; > #endif > > nsSessionStorageEntry *entry = mItems.GetEntry(aKey); > NS_ASSERTION(entry, "Don't use SetSecure() with non-existing keys!"); > > if (entry) { > entry->mItem->SetSecureInternal(aSecure); >@@ -955,32 +1132,35 @@ CopyStorageItems(nsSessionStorageEntry* > newstorage->SetSecure(aEntry->GetKey(), PR_TRUE); > } > > return PL_DHASH_NEXT; > } > > already_AddRefed<nsIDOMStorage> > nsDOMStorage::Clone(nsIURI* aURI) > { > if (UseDB()) { >- NS_ERROR("Uh, don't clone a global storage object."); >+ NS_ERROR("Uh, don't clone a global or local storage object."); > > return nsnull; > } > >- nsDOMStorage* storage = new nsDOMStorage(aURI, mDomain, PR_FALSE); >+ nsDOMStorage* storage = new nsDOMStorage(*this); > if (!storage) > return nsnull; > > mItems.EnumerateEntries(CopyStorageItems, storage); > > NS_ADDREF(storage); >+ >+ if (nsDOMStorageManager::gStorageManager) >+ nsDOMStorageManager::gStorageManager->AddToStoragesHash(storage); > > return storage; > } > > struct KeysArrayBuilderStruct > { > PRBool callerIsSecure; > nsTArray<nsString> *keys; > }; > >@@ -1018,21 +1198,21 @@ nsDOMStorage::BroadcastChangeNotificatio > do_GetService("@mozilla.org/observer-service;1", &rv); > if (NS_FAILED(rv)) { > return; > } > > // Fire off a notification that a storage object changed. If the > // storage object is a session storage object, we don't pass a > // domain, but if it's a global storage object we do. > observerService->NotifyObservers((nsIDOMStorage *)this, > "dom-storage-changed", >- UseDB() ? mDomain.get() : nsnull); >+ UseDB() ? NS_ConvertUTF8toUTF16(mDomain).get() : nsnull); > } > > // > // nsDOMStorageList > // > > NS_INTERFACE_MAP_BEGIN(nsDOMStorageList) > NS_INTERFACE_MAP_ENTRY(nsISupports) > NS_INTERFACE_MAP_ENTRY(nsIDOMStorageList) > NS_DOM_INTERFACE_MAP_ENTRY_CLASSINFO(StorageList) >@@ -1100,98 +1280,100 @@ nsDOMStorageList::NamedItem(const nsAStr > > PRBool isSystem; > rv = ssm->SubjectPrincipalIsSystem(&isSystem); > NS_ENSURE_SUCCESS(rv, rv); > > // allow code that has read privileges to get the storage for any domain > if (!isSystem && nsContentUtils::IsCallerTrustedForRead()) > isSystem = PR_TRUE; > > if (isSystem || !currentDomain.IsEmpty()) { >- return GetStorageForDomain(uri, NS_ConvertUTF8toUTF16(requestedDomain), >- NS_ConvertUTF8toUTF16(currentDomain), >+ return GetStorageForDomain(uri, requestedDomain, currentDomain, > isSystem, aStorage); > } > > return NS_ERROR_DOM_SECURITY_ERR; > } > > // static > PRBool >-nsDOMStorageList::CanAccessDomain(const nsAString& aRequestedDomain, >- const nsAString& aCurrentDomain) >+nsDOMStorageList::CanAccessDomain(const nsACString& aRequestedDomain, >+ const nsACString& aCurrentDomain) > { > return aRequestedDomain.Equals(aCurrentDomain); > } > > nsresult > nsDOMStorageList::GetStorageForDomain(nsIURI* aURI, >- const nsAString& aRequestedDomain, >- const nsAString& aCurrentDomain, >+ const nsACString& aRequestedDomain, >+ const nsACString& aCurrentDomain, > PRBool aNoCurrentDomainCheck, > nsIDOMStorage** aStorage) > { > if (!aNoCurrentDomainCheck && !CanAccessDomain(aRequestedDomain, > aCurrentDomain)) { > return NS_ERROR_DOM_SECURITY_ERR; > } > >- nsStringArray requestedDomainArray; >+ nsCStringArray requestedDomainArray; > PRBool ok = ConvertDomainToArray(aRequestedDomain, &requestedDomainArray); > if (!ok) > return NS_ERROR_DOM_SECURITY_ERR; > > // now rebuild a string for the domain. >- nsAutoString usedDomain; >+ nsCAutoString usedDomain; > PRInt32 requestedPos = 0; > for (requestedPos = 0; requestedPos < requestedDomainArray.Count(); > requestedPos++) { > if (!usedDomain.IsEmpty()) > usedDomain.AppendLiteral("."); > usedDomain.Append(*requestedDomainArray[requestedPos]); > } > > // now have a valid domain, so look it up in the storage table > if (!mStorages.Get(usedDomain, aStorage)) { >- nsCOMPtr<nsIDOMStorage> newstorage = new nsDOMStorage(aURI, usedDomain, PR_TRUE); >- if (!newstorage) >+ nsRefPtr<nsDOMStorage> storage; >+ storage = new nsDOMStorage(); >+ if (!storage) > return NS_ERROR_OUT_OF_MEMORY; > >- if (!mStorages.Put(usedDomain, newstorage)) >+ storage->InitAsGlobalStorage(aURI, usedDomain); >+ >+ if (!mStorages.Put(usedDomain, storage)) > return NS_ERROR_OUT_OF_MEMORY; > >- newstorage.swap(*aStorage); >+ NS_ADDREF(*aStorage = storage); > } > > return NS_OK; > } > > // static > PRBool >-nsDOMStorageList::ConvertDomainToArray(const nsAString& aDomain, >- nsStringArray* aArray) >+nsDOMStorageList::ConvertDomainToArray(const nsACString& aDomain, >+ nsCStringArray* aArray) > { > PRInt32 length = aDomain.Length(); > PRInt32 n = 0; > while (n < length) { > PRInt32 dotpos = aDomain.FindChar('.', n); >- nsAutoString domain; >+ nsCAutoString domain; > > if (dotpos == -1) // no more dots > domain.Assign(Substring(aDomain, n)); > else if (dotpos - n == 0) // no point continuing in this case > return false; > else if (dotpos >= 0) > domain.Assign(Substring(aDomain, n, dotpos - n)); > > ToLowerCase(domain); >- aArray->AppendString(domain); >+ aArray->AppendCString(domain); > > if (dotpos == -1) > break; > > n = dotpos + 1; > } > > // if n equals the length, there is a dot at the end, so treat it as invalid > return (n != length); > } >@@ -1249,22 +1431,21 @@ nsDOMStorageItem::~nsDOMStorageItem() > > NS_IMETHODIMP > nsDOMStorageItem::GetSecure(PRBool* aSecure) > { > if (!mStorage->CacheStoragePermissions() || !IsCallerSecure()) { > return NS_ERROR_DOM_INVALID_ACCESS_ERR; > } > > if (mStorage->UseDB()) { > nsAutoString value; >- nsAutoString owner; >- return mStorage->GetDBValue(mKey, value, aSecure, owner); >+ return mStorage->GetDBValue(mKey, value, aSecure); > } > > *aSecure = IsSecure(); > return NS_OK; > } > > NS_IMETHODIMP > nsDOMStorageItem::SetSecure(PRBool aSecure) > { > if (!mStorage->CacheStoragePermissions() || !IsCallerSecure()) { >@@ -1282,22 +1463,21 @@ nsDOMStorageItem::SetSecure(PRBool aSecu > > NS_IMETHODIMP > nsDOMStorageItem::GetValue(nsAString& aValue) > { > if (!mStorage->CacheStoragePermissions()) > return NS_ERROR_DOM_INVALID_ACCESS_ERR; > > if (mStorage->UseDB()) { > // GetDBValue checks the secure state so no need to do it here > PRBool secure; >- nsAutoString unused; >- nsresult rv = mStorage->GetDBValue(mKey, aValue, &secure, unused); >+ nsresult rv = mStorage->GetDBValue(mKey, aValue, &secure); > return (rv == NS_ERROR_DOM_NOT_FOUND_ERR) ? NS_OK : rv; > } > > if (IsSecure() && !IsCallerSecure()) { > return NS_ERROR_DOM_SECURITY_ERR; > } > > aValue = mValue; > return NS_OK; > } >diff --git a/dom/src/storage/nsDOMStorage.h b/dom/src/storage/nsDOMStorage.h >--- a/dom/src/storage/nsDOMStorage.h >+++ b/dom/src/storage/nsDOMStorage.h >@@ -36,20 +36,21 @@ > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > #ifndef nsDOMStorage_h___ > #define nsDOMStorage_h___ > > #include "nscore.h" > #include "nsAutoPtr.h" > #include "nsIDOMStorage.h" >+#include "nsIDOMStorage2.h" > #include "nsIDOMStorageList.h" > #include "nsIDOMStorageItem.h" > #include "nsInterfaceHashtable.h" > #include "nsVoidArray.h" > #include "nsPIDOMStorage.h" > #include "nsIDOMToString.h" > #include "nsDOMEvent.h" > #include "nsIDOMStorageEvent.h" > #include "nsIDOMStorageManager.h" > #include "nsCycleCollectionParticipant.h" >@@ -60,20 +61,31 @@ > > class nsDOMStorage; > class nsDOMStorageItem; > > class nsDOMStorageEntry : public nsVoidPtrHashKey > { > public: > nsDOMStorageEntry(KeyTypePointer aStr); > nsDOMStorageEntry(const nsDOMStorageEntry& aToCopy); > ~nsDOMStorageEntry(); >+ >+ // weak reference so that it can be deleted when no longer used >+ nsDOMStorage* mStorage; >+}; >+ >+class nsLocalStorageEntry : public nsCStringHashKey >+{ >+public: >+ nsLocalStorageEntry(KeyTypePointer aStr); >+ nsLocalStorageEntry(const nsLocalStorageEntry& aToCopy); >+ ~nsLocalStorageEntry(); > > // weak reference so that it can be deleted when no longer used > nsDOMStorage* mStorage; > }; > > class nsSessionStorageEntry : public nsStringHashKey > { > public: > nsSessionStorageEntry(KeyTypePointer aStr); > nsSessionStorageEntry(const nsSessionStorageEntry& aToCopy); >@@ -91,50 +103,68 @@ public: > > // nsIDOMStorageManager > NS_DECL_NSIDOMSTORAGEMANAGER > > // nsIObserver > NS_DECL_NSIOBSERVER > > void AddToStoragesHash(nsDOMStorage* aStorage); > void RemoveFromStoragesHash(nsDOMStorage* aStorage); > >+ void AddLocalStorage(nsDOMStorage* aStorage); >+ void RemoveLocalStorage(nsDOMStorage* aStorage); >+ > nsresult ClearAllStorages(); > > static nsresult Initialize(); > static nsDOMStorageManager* GetInstance(); > static void Shutdown(); > > static nsDOMStorageManager* gStorageManager; > > protected: > > nsTHashtable<nsDOMStorageEntry> mStorages; >+ nsTHashtable<nsLocalStorageEntry> mLocalStorages; > }; > > class nsDOMStorage : public nsIDOMStorage, >+ public nsIDOMStorage2, > public nsPIDOMStorage > { > public: > nsDOMStorage(); >- nsDOMStorage(nsIURI* aURI, const nsAString& aDomain, PRBool aUseDB); >+ nsDOMStorage(nsDOMStorage& aThat); > virtual ~nsDOMStorage(); > > // nsISupports > NS_DECL_CYCLE_COLLECTING_ISUPPORTS > NS_DECL_CYCLE_COLLECTION_CLASS_AMBIGUOUS(nsDOMStorage, nsIDOMStorage) > > // nsIDOMStorage > NS_DECL_NSIDOMSTORAGE > >+ // nsIDOMStorage2 >+ // This interface is quit the same as nsIDOMStorage except GetItem method >+ // that returns DOMString instead of nsIDOMStorageItem. This way of >+ // inheritance preserves original interface unchanged, provides a new one with >+ // just a single method changed and in very cheap way implements >+ // identical methods from those two interfaces in the same class >+ // - nsDOMStorage. Therefor the only one method different from nsIDOMStorage >+ // is listed here. >+ NS_SCRIPTABLE NS_IMETHOD GetItem(const nsAString & key, >+ nsAString & _retval NS_OUTPARAM); This comment is still screwy... there are a lot of extra spaces, and "quite" and "therefore are misspelled. > #endif /* nsDOMStorageDB_h___ */ >diff --git a/dom/tests/mochitest/ajax/Makefile.in b/dom/tests/mochitest/ajax/Makefile.in > DIRS = lib \ > prototype \ > offline \ >+ localstorage \ > mochikit \ > scriptaculous \ > jquery \ > $(NULL) Can we just put this in dom/tests/mochitest/ ? In retrospect I think it was a mistake putting offline/ in ajax/. >+# The Initial Developer of the Original Code is >+# Mozilla Foundation. >+# Portions created by the Initial Developer are Copyright (C) 2007 >+# the Initial Developer. All Rights Reserved. Copyright info is incorrect here, too. >diff --git a/dom/tests/mochitest/ajax/localstorage/frameSlaveEqual.html b/dom/tests/mochitest/ajax/localstorage/frameSlaveEqual.html >new file mode 100644 >--- /dev/null >+++ b/dom/tests/mochitest/ajax/localstorage/frameSlaveEqual.html >@@ -0,0 +1,49 @@ >+<html xmlns="http://www.w3.org/1999/xhtml"> >+<head> >+<title>frame for localStorage test</title> >+ >+<script type="text/javascript" src="interOriginFrame.js"></script> >+<script type="text/javascript"> >+ >+var currentStep = 2; >+ >+function doStep() >+{ >+ switch (currentStep) >+ { >+ case 2: >+ is(localStorage.getItem("X"), "1", "X is 1 in the master"); >+ localStorage.setItem("X", "2"); >+ is(localStorage.getItem("X"), "2", "X set to 2 in the slave"); >+ break; >+ >+ case 4: >+ is(localStorage.getItem("X"), null, "X was removed from the slave"); >+ localStorage.setItem("Y", "3"); >+ is(localStorage.getItem("Y"), "3", "Y set to 3 in the slave"); >+ break; >+ >+ case 6: >+ is(localStorage.length, 0, "Slave is empty"); >+ is(localStorage.getItem("X"), null, "X is null in the slave"); >+ is(localStorage.getItem("Y"), null, "Y is null in the slave"); >+ is(localStorage.getItem("Z"), null, "Z is null in the slave"); >+ break; >+ >+ case 8: >+ return finishTest(); >+ } >+ >+ ++currentStep; >+ ++currentStep; Maybe make a note of the intent here. >+ >--- /dev/null >+++ b/dom/tests/mochitest/ajax/localstorage/interOriginFrame.js >\ No newline at end of file Add a newline here, please. >diff --git a/dom/tests/mochitest/ajax/localstorage/test_localStorageBase.html b/dom/tests/mochitest/ajax/localstorage/test_localStorageBase.html >new file mode 100644 >--- /dev/null >+++ b/dom/tests/mochitest/ajax/localstorage/test_localStorageBase.html >+function startTest() >+{ >+ // Initially check the localStorage is empty >+ is(localStorage.length, 0, "The storage is empty [1]"); >+ checkException(function() {localStorage.key(0);}, INDEX_SIZE_ERR); // this is unspecified! How is this unspecified? And finally, as per our discussion on irc, I think we need to rip out the mLocalStorages hashtable :/
Assignee | ||
Comment 35•16 years ago
|
||
Interdiff follows. Synchronization of localStorage objects bound to same origin but in different windows is ensured thanks implementation of nsDOMStorageItem::GetValue, it reloads the value from database anytime the key is accessed. This doesn't preserve correct order of keys in both storages. Follow up will be filed.
Attachment #338885 -
Attachment is obsolete: true
Attachment #339108 -
Flags: review?(dcamp)
Assignee | ||
Comment 36•16 years ago
|
||
Assignee | ||
Comment 37•16 years ago
|
||
A note: v9 patch needs to fix the tests' makefiles yet. Discovered after full rebuild.
Assignee | ||
Comment 38•16 years ago
|
||
Changes just in tests. I also added test to check if order of keys is preserved after change of key value and after localStorage object re-creation (reload from database). This is sufficient to conform with WHATWG so, there is no need to fix anything around this.
Attachment #339108 -
Attachment is obsolete: true
Attachment #339313 -
Flags: review?(dcamp)
Attachment #339108 -
Flags: review?(dcamp)
Assignee | ||
Comment 39•16 years ago
|
||
- fixed test paths - according to dcamp's suggestion removed WHATWG compatibility changes what makes the patch more simple - added new tests for quota testing and RemoveOwners API testing
Attachment #339109 -
Attachment is obsolete: true
Attachment #339313 -
Attachment is obsolete: true
Attachment #339710 -
Flags: review?(dcamp)
Attachment #339313 -
Flags: review?(dcamp)
Assignee | ||
Comment 40•16 years ago
|
||
Just for case of interest.
Assignee | ||
Comment 41•16 years ago
|
||
As v10 + added missing new tests.
Attachment #339710 -
Attachment is obsolete: true
Attachment #339715 -
Flags: review?(dcamp)
Attachment #339710 -
Flags: review?(dcamp)
Reporter | ||
Comment 42•16 years ago
|
||
(In reply to comment #39) > Created an attachment (id=339710) [details] > - according to dcamp's suggestion removed WHATWG compatibility changes what > makes the patch more simple I think you must have misinterpreted what I said. Could we get a new version based on v9.1, but with the test changes please?
Assignee | ||
Updated•16 years ago
|
Attachment #339711 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #339715 -
Attachment is obsolete: true
Attachment #339715 -
Flags: review?(dcamp)
Assignee | ||
Comment 43•16 years ago
|
||
Misunderstanding, revert to have full WHATWG support + few nits + one new test.
Attachment #339837 -
Flags: review?(dcamp)
Reporter | ||
Updated•16 years ago
|
Attachment #339837 -
Flags: review?(dcamp) → review+
Reporter | ||
Comment 44•16 years ago
|
||
Comment on attachment 339837 [details] [diff] [review] v9.2 Looks ok, with a few nits: >- for (PRInt32 i = 1; i < aOwners.Count(); i++) { >- expression.Append(", ?"); >+ // Using Append(NS_LITERAL_CSTRING()) because >+ // it is faster then AppendLiteral >+ for (PRInt32 i = 0; i < aOwners.Count(); i++) { >+ if (i) >+ expression.Append(NS_LITERAL_CSTRING(" UNION ")); >+ >+ expression.Append(NS_LITERAL_CSTRING( >+ "SELECT DISTINCT scope FROM webappsstore2 WHERE scope GLOB ?")); As mentioned on irc, please use AppendLiteral() here. >+ // Increese by two to distinguish each test step order >+ // in both master doStep and slave doStep functions. "Increase", and this was repeated elsewhere. >+ //alert("master got event: "+event.data); This should be removed. >+<!-- >+ This test is loading two frames from different >+ origins and checks that entries of localStorage >+ objects don't leak each between other. "This test loads two frames...", repeated elsewhere too. >diff --git a/js/src/xpconnect/src/dom_quickstubs.qsconf b/js/src/xpconnect/src/dom_quickstubs.qsconf >--- a/js/src/xpconnect/src/dom_quickstubs.qsconf >+++ b/js/src/xpconnect/src/dom_quickstubs.qsconf >@@ -454,25 +454,26 @@ members = [ > > # dom/public/idl/offline - None. > > # dom/public/idl/range > 'nsIDOMRange.collapsed', > > # dom/public/idl/sidebar - None. > > # dom/public/idl/storage > 'nsIDOMToString.toString', >- 'nsIDOMStorage.setItem', >- 'nsIDOMStorage.length', >- 'nsIDOMStorage.getItem', >- 'nsIDOMStorage.key', >- 'nsIDOMStorage.removeItem', >+ 'nsIDOMStorage2.setItem', >+ 'nsIDOMStorage2.length', >+ 'nsIDOMStorage2.getItem', >+ 'nsIDOMStorage2.key', >+ 'nsIDOMStorage2.removeItem', >+ 'nsIDOMStorage2.clear', I don't know how the quickstubs stuff works, you'll need to pass that by someone that does...
Assignee | ||
Comment 45•16 years ago
|
||
r+ by dcamp (v9.2). Jason, please take a look at the quick stubs part, we were talking about that on #developers recently.
Attachment #339837 -
Attachment is obsolete: true
Attachment #339867 -
Flags: superreview?(jst)
Attachment #339867 -
Flags: review?(jorendorff)
Reporter | ||
Updated•16 years ago
|
Flags: blocking1.9.1?
Comment 46•16 years ago
|
||
Comment on attachment 339867 [details] [diff] [review] v9.2.1 - In nsGlobalWindow::GetLocalStorage(nsIDOMStorage2 ** aLocalStorage): + NS_ENSURE_ARG(aLocalStorage); + + if (!mLocalStorage) { This needs to forward to the inner window so that the local storage objects lifetime is bound to the inner window. And even if this forwards to the inner, this code needs to do security checks in the methods that return or sets data in the local storage object. Dave, we talked about this and if that can land as part of this then this should be all good. Same probably goes for globalStorage as well (which also needs to forward to the inner or outer, see bug 453649). - In nsDOMStorageItem::GetValue(nsAString& aValue): + if (rv == NS_ERROR_DOM_NOT_FOUND_ERR) { + SetDOMStringToNull(aValue); + return NS_OK; + } + else + return rv; Nit of the day, remove the else-after-return. With the nsGlobalWindow::GetLocalStorage() changes above included this could land IMO.
Attachment #339867 -
Flags: superreview?(jst)
Attachment #339867 -
Flags: superreview+
Attachment #339867 -
Flags: review?(jorendorff)
Attachment #339867 -
Flags: review+
Reporter | ||
Comment 47•16 years ago
|
||
Attachment #341199 -
Flags: superreview?(jst)
Attachment #341199 -
Flags: review?(jst)
Comment 48•16 years ago
|
||
Comment on attachment 341199 [details] [diff] [review] security/nit fixes - In nsDOMStorage::CanAccessStorage(): + nsresult rv = ssm->CheckSameOriginURI(subjectURI, mURI, PR_TRUE); Ideally this should use CheckSameOrigin(nsnull, mURI), please file a followup bug to figure out why this didn't work. r+sr=jst
Attachment #341199 -
Flags: superreview?(jst)
Attachment #341199 -
Flags: superreview+
Attachment #341199 -
Flags: review?(jst)
Attachment #341199 -
Flags: review+
Comment 49•16 years ago
|
||
So is there a reason that chrome code can't access storage?
Reporter | ||
Comment 50•16 years ago
|
||
(In reply to comment #49) > So is there a reason that chrome code can't access storage? Talked with jst about this on irc - the newest patch allows chrome access. I ran the chrome tests on this, and globalStorage.getItem() is now returning strings instead of storageItems. I'm working on that before landing...
Comment 51•16 years ago
|
||
This missed the boat, right? If not, please renominate.
Flags: blocking1.9.1? → blocking1.9.1-
Comment 52•16 years ago
|
||
If so, that's very disappointing. There seemed to be so much activity here. Is this feature that far away from finishing? If so, I'll offer my help in testing or docs (haven't been a C programmer in a looong time). Cheers, - Bill
Assignee | ||
Comment 53•16 years ago
|
||
I have a complete patch including dcamp's sec and nit fixes for this for a long time. I just need to merge it and make sure all tests for global, local and sessionStorage work. Currently globalStorage test fails and because this bug missed 3.1 milestone it is not a priority for me to work on it now.
Assignee | ||
Comment 54•16 years ago
|
||
This is merged patch for 1.9.2 branch. It includes 9.2.1 version of the localstorage patch, Dave Camp's security fixes and introduction of DomStorage2 DOM class, my fix for getter consistency and some additions to localStorage tests. Following change to nsDOMStorage::GetDBValue was added because localStorage object implementing nsIDOMStorage2 interface must return null in case a key is not present, I am going to change this more properly for upcoming sessionStorage patch. - rv = gStorageDB->GetKeyValue(mDomain, aKey, value, aSecure, aOwner); + rv = gStorageDB->GetKeyValue(this, aKey, value, aSecure); + + if (rv == NS_ERROR_DOM_NOT_FOUND_ERR && mLocalStorage) { + SetDOMStringToNull(aValue); + } All tests touching globalStorage was run and passing. Interdiff follows.
Attachment #339867 -
Attachment is obsolete: true
Attachment #341199 -
Attachment is obsolete: true
Attachment #351085 -
Flags: review?(jst)
Assignee | ||
Comment 55•16 years ago
|
||
Assignee | ||
Updated•16 years ago
|
Flags: blocking1.9.2?
Updated•16 years ago
|
Attachment #351086 -
Flags: superreview+
Attachment #351086 -
Flags: review+
Comment 56•16 years ago
|
||
Comment on attachment 351086 [details] [diff] [review] interdiff v9 -> v10 - In nsStorage2SH::NewResolve(): I don't see this code doing anything that would change how the JS engine works, is this really needed? - In nsStorage2SH::GetProperty(): + if (JSVAL_IS_STRING(id)) { + // For native wrappers, do not get random names on storage objects. + if (ObjectIsNativeWrapper(cx, obj)) { + return NS_ERROR_NOT_AVAILABLE; + } + + rv = storage->GetItem(nsDependentJSString(id), val); + NS_ENSURE_SUCCESS(rv, rv); + } else { + PRInt32 n = GetArrayIndexFromId(cx, id); + rv = storage->Key(n, val); + NS_ENSURE_SUCCESS(rv, rv); + } "id" here could be either a string, an integer, or an XML value. The else case deals with failure to convert id to an integer, but that'll result in n being -1, which storage->Key() seems to handle, but if I pass in another negative value I could likely hit existing indexes in the storage object which we probably don't want. So I'd say check if n is negative there and throw... - In nsDOMStorage::CanAccessStorage(): + if (NS_FAILED(rv)) + return PR_FALSE; return PR_TRUE; Maybe just do return NS_SUCCEEDED(rv) here? - In dom/src/storage/nsDOMStorageDB.cpp: + printf("QUOTA: %d of %d\n", usage, aQuota); [...] + printf("QUOTA: %d of %d\n", usage, aQuota); Remove the printfs. r+sr=jst with that looked into.
Reporter | ||
Comment 57•16 years ago
|
||
The nsStorageSH::NewResolve has a clause after the storage->GetItem() call: if (item) { if (!::JS_DefineUCProperty(cx, realObj, ::JS_GetStringChars(jsstr), ::JS_GetStringLength(jsstr), JSVAL_VOID, nsnull, nsnull, 0)) { return NS_ERROR_FAILURE; } *objp = realObj; } This should probably be added to the nsStorage2SH::NewResolve() (which will then be relevant).
Assignee | ||
Comment 58•16 years ago
|
||
I didn't address jst's comment for nsDOMStorage::CanAccessStorage(). I leave "if (NS_FAILED(rv)) return PR_FALSE;" because I think: - it conforms with the style of the rest of the method - as suggested it could be overlooked/misinterpreted - it's safer and easier when adding new conditions/code bellow this one
Attachment #351085 -
Attachment is obsolete: true
Attachment #351086 -
Attachment is obsolete: true
Attachment #351085 -
Flags: review?(jst)
Assignee | ||
Comment 61•15 years ago
|
||
Before this patch has landed there were major change to storage management and security code. This is merged + updated version.
Attachment #352593 -
Attachment is obsolete: true
Attachment #357717 -
Flags: review?(dcamp)
Assignee | ||
Comment 62•15 years ago
|
||
Includes merge changes (so some pieces are duplicated).
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs review]
Assignee | ||
Comment 63•15 years ago
|
||
Comment on attachment 357717 [details] [diff] [review] v11 There is also interdiff, attachment 357718 [details] [diff] [review]. We had to change it a bit again because of security checking code updates before the patch got landed.
Attachment #357717 -
Flags: review?(jst)
Assignee | ||
Comment 64•15 years ago
|
||
As full v10 and v11 patches are both build on different repo versions and I had to do lot's of merging I couldn't just qnew over v10 and have automatically a nice interdiff. The previous interdiff isn't perfect therefor. This interdiff is created manually using 'diff' of adjusted v10 and v11 patches. So, you can find @@ REAL PATCH @@ delimiters that refers to hunks in the real patches. It also contains changes introduced and landed on repo between each v10 and v11 have been made, so it might a bit crazy at first sight.
Attachment #357718 -
Attachment is obsolete: true
Updated•15 years ago
|
Attachment #357717 -
Flags: review?(jst) → review+
Comment 65•15 years ago
|
||
Comment on attachment 357717 [details] [diff] [review] v11 This is getting to be a pretty big patch to wrap your head around, but I spent a bunch of time reading through this and it looks good to me (admittedly w/o fully grasping the all of this feature). sr=jst
Reporter | ||
Updated•15 years ago
|
Attachment #357717 -
Flags: review?(dcamp)
Attachment #357717 -
Flags: review?(bzbarsky)
Attachment #357717 -
Flags: review+
Reporter | ||
Comment 66•15 years ago
|
||
Comment on attachment 357717 [details] [diff] [review] v11 The significant change from the last time this patch was reviewed/sr'ed is a small(?) change in CanAccess(). I'm highlighting those changes here, I'd like bz to double-check them. The patch in bug 458091 added a CanAccess() method to nsPIDOMStorage, which at the time did the domain comparison required by the globalStorage spec. Because localStorage is a real origin comparison, it makes sense for CanAccess() on a localStorage object (implemented in nsIDOMStorage2) to store/use a principal comparison to make that decision. >diff --git a/dom/public/idl/storage/nsPIDOMStorage.h b/dom/public/idl/storage/nsPIDOMStorage.h >--- a/dom/public/idl/storage/nsPIDOMStorage.h >+++ b/dom/public/idl/storage/nsPIDOMStorage.h >- virtual void Init(const nsAString &aDomain, PRBool aUseDB) = 0; >+ virtual nsresult InitAsLocalStorage(nsIPrincipal *aPrincipal) = 0; >+ virtual nsresult InitAsGlobalStorage(const nsACString &aDomainDemanded) = 0; >+ virtual nsresult InitAsSessionStorage(nsIURI* aURI) = 0; > NS_DEFINE_STATIC_IID_ACCESSOR(nsPIDOMStorage, NS_PIDOMSTORAGE_IID) > > #endif // __nsPIDOMStorage_h_ >+NS_IMETHODIMP >+nsGlobalWindow::GetLocalStorage(nsIDOMStorage2 ** aLocalStorage) >+{ >+ FORWARD_TO_INNER(GetLocalStorage, (aLocalStorage), NS_ERROR_UNEXPECTED); >+ >+ NS_ENSURE_ARG(aLocalStorage); >+ >+ if (!mLocalStorage) { >+ *aLocalStorage = nsnull; >+ >+ nsresult rv; >+ >+ nsIPrincipal *principal = GetPrincipal(); >+ if (!principal) >+ return NS_OK; >+ >+ PRPackedBool sessionOnly; >+ if (!nsDOMStorage::CanUseStorage(&sessionOnly)) >+ return NS_ERROR_DOM_SECURITY_ERR; >+ >+ if (sessionOnly) >+ return NS_ERROR_DOM_SECURITY_ERR; >+ >+ nsCOMPtr<nsIDOMStorageManager> storageManager = >+ do_GetService("@mozilla.org/dom/storagemanager;1", &rv); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ rv = storageManager->GetLocalStorageForPrincipal(principal, getter_AddRefs(mLocalStorage)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ } >+ >+ NS_ADDREF(*aLocalStorage = mLocalStorage); >+ return NS_OK; >+} >+NS_IMETHODIMP >+nsDOMStorageManager::GetLocalStorageForPrincipal(nsIPrincipal *aPrincipal, >+ nsIDOMStorage2 **aResult) >+{ >+ NS_ENSURE_ARG_POINTER(aPrincipal); >+ *aResult = nsnull; >+ >+ nsresult rv; >+ >+ nsRefPtr<nsDOMStorage2> storage = new nsDOMStorage2(); >+ if (!storage) >+ return NS_ERROR_OUT_OF_MEMORY; >+ >+ rv = storage->InitAsLocalStorage(aPrincipal); >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ *aResult = storage.get(); >+ storage.forget(); >+ >+ return NS_OK; > } >-void >-nsDOMStorage::Init(const nsAString& aDomain, PRBool aUseDB) >+nsresult >+nsDOMStorage::InitAsLocalStorage(nsIPrincipal *aPrincipal) > { >- mDomain.Assign(aDomain); >+ nsresult rv; >+ >+ nsCOMPtr<nsIURI> uri; >+ rv = aPrincipal->GetURI(getter_AddRefs(uri)); >+ NS_ENSURE_SUCCESS(rv, rv); >+ >+ nsCOMPtr<nsIURI> innerUri = NS_GetInnermostURI(uri); >+ if (!innerUri) >+ return NS_ERROR_UNEXPECTED; >+ >+ innerUri->GetAsciiHost(mDomain); >+ > #ifdef MOZ_STORAGE >- mUseDB = aUseDB; >-#else >+ nsDOMStorageDB::CreateOriginScopeDBKey(innerUri, mScopeDBKey); >+ >+ // XXX Bug 357323, we have to solve the issue how to define >+ // origin for file URLs. In that case CreateOriginScopeDBKey >+ // fails (the result is empty) and we must avoid database use >+ // in that case because it produces broken entries w/o owner. >+ mUseDB = !mScopeDBKey.IsEmpty(); >+ >+ nsDOMStorageDB::CreateQuotaDomainDBKey(mDomain, PR_TRUE, mQuotaDomainDBKey); >+#endif >+ >+ mLocalStorage = PR_TRUE; >+ return NS_OK; >+} >+ > PRBool >-nsDOMStorage::CanAccess(nsIPrincipal *aPrincipal) >+nsDOMStorage::CanAccessSystem(nsIPrincipal *aPrincipal) > { >- // Allow C++/system callers to access the storage > if (!aPrincipal) > return PR_TRUE; > > nsIScriptSecurityManager* ssm = nsContentUtils::GetSecurityManager(); > if (!ssm) > return PR_TRUE; > > PRBool isSystem; > if (NS_SUCCEEDED(ssm->IsSystemPrincipal(aPrincipal, &isSystem) && isSystem)) > return PR_TRUE; > >- nsAutoString domain; >+ return PR_FALSE; >+} >+ >+PRBool >+nsDOMStorage::CanAccess(nsIPrincipal *aPrincipal) >+{ >+ // Allow C++/system callers to access the storage >+ if (CanAccessSystem(aPrincipal)) >+ return PR_TRUE; >+ >+ nsCAutoString domain; > nsCOMPtr<nsIURI> unused; > nsresult rv = GetPrincipalURIAndHost(aPrincipal, > getter_AddRefs(unused), domain); > NS_ENSURE_SUCCESS(rv, PR_FALSE); > > return domain.Equals(mDomain); > } .. >+PRBool >+nsDOMStorage2::CanAccess(nsIPrincipal *aPrincipal) >+{ >+ // Allow C++/system callers to access the storage >+ if (mStorage->CanAccessSystem(aPrincipal)) >+ return PR_TRUE; >+ >+ PRBool equal; >+ nsresult rv = mPrincipal->Equals(aPrincipal, &equal); >+ if (NS_FAILED(rv)) >+ return PR_FALSE; >+ >+ return equal; >+} My only question is if we should be using Subsumes() instead of Equals().
Comment 67•15 years ago
|
||
Hmm. 1) Is nsDOMStorage::InitAsLocalStorage actually used? If so, why does it not need to check the return value of GetAsciiHost? Why does it not need to check ports? Or scheme, for that matter? 2) Using Subsumes() in CanAccess would make a lot of sense to me (and remove the need for the CanAccessSystem() call: just allow access if !aPrincipal or aPrincipal subsumes. 3) Code like: if (NS_SUCCEEDED(ssm->IsSystemPrincipal(aPrincipal, &isSystem) && isSystem)) return PR_TRUE; return PR_FALSE; Should just return the if condition and be done with it.
Assignee | ||
Comment 68•15 years ago
|
||
(In reply to comment #67) > Hmm. > > 1) Is nsDOMStorage::InitAsLocalStorage actually used? It is: https://bugzilla.mozilla.org/attachment.cgi?id=357717&action=diff#a/dom/src/storage/nsDOMStorage.cpp_sec5, search for nsDOMStorageManager::GetLocalStorageForPrincipal() > If so, why does it not need to check the return value of GetAsciiHost? We call GetPrincipalURIAndHost (nsDOMStorage.cpp:88) from nsDOMStorage::CanUseStorage before we query the storage manager for a new localStorage. It calls GetAsciiHost on innermost URI. If it fails, we won't get to InitAsLocalStorage. I have to check it and fix or add a comment, but as I remember the check is missing because of file: uris. > Why does it not need to check ports? Or scheme, for that matter? You mean check for a valid port number? You want to filter out things like javascript: or about: from getting a localStorage? The uri passed to InitAsLocalStorage is taken from a principal (its uri attribute). We does the same checks as for sessionStorage here (already out for months) except that to obtain a sessionStorage, URI has to have non-empty asciiHost. There is nothing said in the spec but it would make sense to be more strict here. I tried to do javascript:sessionStorage.foo="bar" in about:blank window. Getting the sessionStorage fails because of an empty asciiHost. Will do the same test for localStorage after I merge the patch. > 2) Using Subsumes() in CanAccess would make a lot of sense to me (and remove > the need for the CanAccessSystem() call: just allow access if !aPrincipal > or > aPrincipal subsumes. I'll update it. > 3) Code like: > > if (NS_SUCCEEDED(ssm->IsSystemPrincipal(aPrincipal, &isSystem) && isSystem)) > return PR_TRUE; > return PR_FALSE; > > Should just return the if condition and be done with it. Irrelevant after 2) is fixed?
Comment 69•15 years ago
|
||
> If it fails, we won't get to InitAsLocalStorage. Ah, ok. Worth asserting that the GetAsciiHost here succeeds, then, with a comment pointing out the above. Or just fail out if it fails. > You mean check for a valid port number? No, I mean to check that script at http://example.com:80 is not getting the storage for http://example.com:70 or https://example.com:80. Does the spec really not require same origin (as opposed to just same host) access?
Assignee | ||
Comment 70•15 years ago
|
||
(In reply to comment #69) > > You mean check for a valid port number? > > No, I mean to check that script at http://example.com:80 is not getting the > storage for http://example.com:70 or https://example.com:80. Does the spec > really not require same origin (as opposed to just same host) access? There is distinct localStorage for each origin. We bound a localStorage object to each (new) window, create it for the window's principal (origin) and throw it away when loading a new URI. initAsLocalStorage reads the URI from the principal given and creates an "origin database key" that consist of scheme + host + port. It is well described here: https://bugzilla.mozilla.org/attachment.cgi?id=357717&action=diff#a/dom/src/storage/nsDOMStorageDB.h_sec1 Example of a database key is 'moc.rab.foo.:http:80.' for http://foo.bar.com. This way http://example.com:80 may never get storage for http://example.com:70
Comment 71•15 years ago
|
||
OK, but why do we need later host compares at all then? And if we need them, why are they only host compares?
Assignee | ||
Comment 72•15 years ago
|
||
(In reply to comment #71) > OK, but why do we need later host compares at all then? And if we need them, > why are they only host compares? Where exactly do you see them, could you refer to the patch please? I'm not aware of any that is made initAsLocalStorage. It just builds some member string vars to be used for DB queries, all info taken from the principal.
Comment 73•15 years ago
|
||
Hold on. What exactly is mDomain used for? And why is a host-only compare the right thing for whatever that thing is?
Assignee | ||
Comment 74•15 years ago
|
||
(In reply to comment #73) > Hold on. What exactly is mDomain used for? And why is a host-only compare the > right thing for whatever that thing is? It was introduced here: https://bugzilla.mozilla.org/attachment.cgi?id=355701&action=diff#a/embedding/components/windowwatcher/src/nsWindowWatcher.cpp_sec2 It's used to copy sessionStorage to a new window. Domain() will be replaced with Principal() in bug 455070.
Reporter | ||
Comment 75•15 years ago
|
||
(In reply to comment #74) > (In reply to comment #73) > > Hold on. What exactly is mDomain used for? And why is a host-only compare the > > right thing for whatever that thing is? > > It was introduced here: > https://bugzilla.mozilla.org/attachment.cgi?id=355701&action=diff#a/embedding/components/windowwatcher/src/nsWindowWatcher.cpp_sec2 To clarify a bit - the globalStorage API is specced as a host-only compare (one of the motivating reasons behind the localStorage change). localStorage *was* also a host-only compare, but the spec changed to a complete origin compare when localStorage was introduced (that's tracked in bug 455070). mDomain has been around forever, the public accessor for it was added in 455070.
Reporter | ||
Comment 76•15 years ago
|
||
> of the motivating reasons behind the localStorage change). localStorage *was*
> also a host-only compare,
sorry - sessionStorage *was* ... etc.
Comment 77•15 years ago
|
||
Ah, ok. Thanks for clarifying that.
Assignee | ||
Comment 78•15 years ago
|
||
So, I merged the patch again to trunk, fortunately just minor changes. It doesn't address the bz's comments yet. I pushed it to the try server and all tests passed, no T regressions visible.
Assignee | ||
Comment 79•15 years ago
|
||
Addressed bz's comments. Interdiff follows.
Attachment #357717 -
Attachment is obsolete: true
Attachment #365899 -
Attachment is obsolete: true
Attachment #357717 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 80•15 years ago
|
||
Attachment #367389 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 81•15 years ago
|
||
Ups, the v12 patch was disallowing system to access globalStorage.
Attachment #367387 -
Attachment is obsolete: true
Attachment #367389 -
Attachment is obsolete: true
Attachment #367389 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 82•15 years ago
|
||
Attachment #367394 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [needs review] → [needs review bzbarsky]
Comment 83•15 years ago
|
||
The Subsumes() check is backwards. If system aPrincipal should cause this method to return true just like null aPrincipal, then the right check is aPrincipal->Subsumes(mPrincipal, &subsumes);
Comment 84•15 years ago
|
||
With the check fixed and a test added (chrome script accessing non-chrome storage and succeeding), looks ok.
Assignee | ||
Comment 85•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/0c0517267c4a
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [needs review bzbarsky]
Assignee | ||
Comment 86•15 years ago
|
||
Attachment #367393 -
Attachment is obsolete: true
Attachment #367394 -
Attachment is obsolete: true
Attachment #367394 -
Flags: review?(bzbarsky)
Comment 87•15 years ago
|
||
> + // Allow system and all weaker principals access the storage
Doesn't make sense... Perhaps:
// Allow more powerful principals (e.g. system) to access the storage
?
Assignee | ||
Comment 88•15 years ago
|
||
(In reply to comment #87) > > + // Allow system and all weaker principals access the storage > > Doesn't make sense... Perhaps: > > // Allow more powerful principals (e.g. system) to access the storage > > ? True, I will fix the comment by a direct checkin.
Assignee | ||
Comment 89•15 years ago
|
||
Attachment #368282 -
Flags: approval1.9.1?
Assignee | ||
Comment 90•15 years ago
|
||
(In reply to comment #87) > > + // Allow system and all weaker principals access the storage > > Doesn't make sense... Perhaps: > > // Allow more powerful principals (e.g. system) to access the storage http://hg.mozilla.org/mozilla-central/rev/55ef043b0af4
Assignee | ||
Comment 91•15 years ago
|
||
The patch still needs a security review. Was decided who should do it? Should I do it?
Assignee | ||
Comment 92•15 years ago
|
||
(In reply to comment #91) > The patch still needs a security review. Was decided who should do it? Should I > do it? https://wiki.mozilla.org/Firefox3.1/localStorage_Security_Review Is any of the list bellow (tests, warning messages) wanted before we land this on 1.9.1?
Comment 93•15 years ago
|
||
From the security review ..: > * currently if cookie lifetime is session-only we turn > globalStorage/localStorage into a lame current-page-only memory store. It would > be better to throw an error (QUOTA_EXCEEDED_ERR is mentioned in the spec for > this case) or return null for locationStorage itself. Something that lets > developers switch to using real sessionStorage instead, or telling users to > enable the feature. Not sure why that's a security comment, but it seems sensible for 1.9.1 > * test that blocking cookies for a host does block localStorage for that host Yes, we need this for 1.9.1 > * test that localStorage is truly origin-based -- i.e. windows on different > ports or schemes should not be able to access each other's localStorage even > if the same host. Absolutely needed for 1.9.1, yes. That's the definition of localStorage and keeps us from leaking data. > * use of globalStorage[] should put a deprecation warning on the error > console. Probably just the first use per window to avoid perf problems > (object creation). I don't think we need this for branch, as it would require that we break string freeze - we should file a follow-up and get it fixed for trunk. > * add default prefs to all.js for missing values (e.g. quota). Needed for 1.9.1. > * Should we put a length limit on keys? It's counted in the quota so we think we're OK. Not needed for 1.9.1 > * test both keys and values with embedded nulls Needed for 1.9.1. > * test both keys and values with invalid UTF-8 sequences and invalid UTF-16, including ending on partial sequences. Absolutely needed for 1.9.1 > * we feed UTF-16 to sqlite, make sure it handles this OK and converts to utf-8 OK. Absolutely needed for 1.9.1
Comment 94•15 years ago
|
||
(In reply to comment #93) What's the vehicle for getting these changes into 1.9.1? Is that going to be this bug? Or should we open follow on bug(s) for these issues?
Assignee | ||
Comment 95•15 years ago
|
||
(In reply to comment #94) > (In reply to comment #93) > What's the vehicle for getting these changes into 1.9.1? Is that going to be > this bug? Or should we open follow on bug(s) for these issues? Bug 486654.
Assignee | ||
Updated•15 years ago
|
Attachment #368282 -
Attachment is obsolete: true
Attachment #368282 -
Flags: approval1.9.1?
Assignee | ||
Comment 96•15 years ago
|
||
Comment on attachment 368282 [details] [diff] [review] v12.1 for 1.9.1 Obsoleting this patch. A newer version with addressed security comments will be added to bug 486654.
Updated•15 years ago
|
Keywords: dev-doc-needed
Comment 97•15 years ago
|
||
I know I'm way too late in the game, but why does localStorage have to be a Storage2 instance? Can't Storage2 and Storage swap names so when a webapp developer tries to extend Storage, it extends localStorage instead of globalStorage[location.hostname]? This is how every other browser with localStorage support (WebKit, IE, Google Chrome) handles it, making localStorage be an instance of Storage. For example, I noticed that my Storage.prototype.removeKey function wasn't working in Firefox's latest nightly while worked in Google Chrome and WebKit.
Comment 98•15 years ago
|
||
Another suggestion: Is it possible to make globalStorage items and localStorage both inherit from Storage?
Comment 99•15 years ago
|
||
> I noticed that my Storage.prototype.removeKey function wasn't working in
> Firefox's latest nightly
If so (as in, if the internal class naming is web-detectable), then that's most certainly a bug. Is it filed (ideally with a testcase)? It should be blocking 1.9.1.
Comment 100•15 years ago
|
||
(In reply to comment #99) Should I make a separate bug for this or upload the testcase here?
Comment 101•15 years ago
|
||
Separate bug, like I said in comment 99.
Assignee | ||
Comment 102•15 years ago
|
||
Removing b192 request as it is already in 1.9.1.
Flags: blocking1.9.2?
Updated•11 years ago
|
Component: DOM: Mozilla Extensions → DOM
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
•