Implement webapps session and persistent storage

RESOLVED FIXED in mozilla1.8.1beta1

Status

()

Core
DOM
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Neil Deakin (not available until Aug 9), Assigned: Neil Deakin (not available until Aug 9))

Tracking

(Depends on: 1 bug, {fixed1.8.1})

1.8 Branch
mozilla1.8.1beta1
fixed1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(17 attachments, 4 obsolete attachments)

24.52 KB, patch
Details | Diff | Splinter Review
6.04 KB, patch
Details | Diff | Splinter Review
9.20 KB, patch
Details | Diff | Splinter Review
12.62 KB, patch
Details | Diff | Splinter Review
121.02 KB, patch
vlad
: review+
Details | Diff | Splinter Review
121.33 KB, patch
Details | Diff | Splinter Review
1.28 KB, patch
shaver
: review+
bsmedberg
: review+
Details | Diff | Splinter Review
938 bytes, patch
Details | Diff | Splinter Review
1.90 KB, patch
Details | Diff | Splinter Review
134.98 KB, patch
Details | Diff | Splinter Review
48.29 KB, patch
Details | Diff | Splinter Review
4.12 KB, patch
Details | Diff | Splinter Review
18.01 KB, patch
Details | Diff | Splinter Review
107.98 KB, patch
Neil Deakin (not available until Aug 9)
: review+
Details | Diff | Splinter Review
541 bytes, patch
Details | Diff | Splinter Review
130.83 KB, patch
Details | Diff | Splinter Review
4.29 KB, patch
Details | Diff | Splinter Review
See URL for spec
Created attachment 219881 [details] [diff] [review]
Work in progress

Global storage is working fairly well. There's are two testcases at http://xulplanet.com/ndeakin/tests/sessionstorage.html and http://xulplanet.com/ndeakin/tests/domains.html

Session storage isn't implemented.
Created attachment 220599 [details] [diff] [review]
Neil's patch and my work merged together.

Here's more WIP. I took Neil's changes and merged in what I had. There's still some duplication of code here that needs to be removed (my stubs of some of the global storage code can be removed etc). Most of the code I wrote lives in dom/src/base/nsDOMStorage.{cpp,h} and should probably be moved to dom/src/whatwg, and that directory should IMO be renamed to "storage" rather than "whatwg" since the spec for this is likely to be taken over by the W3C which would make the whatwg name confusing.

My changes add some of the storage event plumbing, but there's still more work to be done there. The session storage objects also need to be properly hooked up and they need to be shared between windows in the same toplevel window from the same domain etc, and the whole session history part needs to be figured out too.
I had started writing some code for session storage. I assumed that each top level docshell would have a DOMStorageList object. Then window.sessionStorage would just look up the current domain in that list and return the right DOMStorage. When a new window is opened, that current DOMStorage would be copied to the new docshell as the sole entry in its DOMStorageList.

jst, were you planning on merging the two DOMStorage implementations into one?
Yes, I think we should merge the two into one.
Created attachment 220746 [details] [diff] [review]
Additional changes to merge the two storage objects, and other random cleanup.

This merges the two storage objects into nsWebAppsStorage. I also changed the storage item code to keep the value in the case where the storage item is used for session storage, and made the mItems hash in nsWebAppsStorage hold items rather than string values since we need to store the secure bit for all items as well. That part needs to come out of the DB for global storage items. Neil, could you look into that?
Two issues here:
 - getItem is supposed to return a different StorageItem every time it is called
 - the value in StorageItem is a live value, so it needs to be re-retrieved every time
(In reply to comment #6)
> Two issues here:
>  - getItem is supposed to return a different StorageItem every time it is
> called

It is? I didn't see that in the spec last time I read it, and looking again I don't see that in the spec now either.

>  - the value in StorageItem is a live value, so it needs to be re-retrieved
> every time

With the non-db storage object per the last patch, the storage item *is* the value, and thus any changes to it means anyone having a reference to the item will see the new value.
(In reply to comment #7)
> It is? I didn't see that in the spec last time I read it, and looking again I
> don't see that in the spec now either.

In the description of getItem:

"Subsequent calls to this method with the same key must return different instances of the StorageItem interface."
Maybe we just should get the spec changed so that it does return the same instance every time. It would make things much easier and the setting the secure flag would make more sense.
Hmm, yeah... I never read enough of that paragraph, apparently. And the first sentence is:

  The getItem(key) method must return *the* StorageItem object representing the key/value pair with the given key.

So that's not very consistent. Ian?
I wanted it to return the same object every time (which is why it starts of with "the") but I can't see how to do that given that the object might be accessed from multiple different windows, which might be running in completely separate threads in some implementations.

As you're the first implementors, you get to tell me what you want. What do you want, given the constraint of handling this across totally independent windows in a way consistent with handling it across windows that are separate but can communicate?
Created attachment 220840 [details] [diff] [review]
Updated work in progress

Implements both global and session storage, and is shared as one navigates to different pages.

Still left to implement:
  - secure flag, could be stubbed out for now though
  - event handling is there, but not sure whether it is right
  - copying session data on new window
(In reply to comment #11)
> I wanted it to return the same object every time (which is why it starts of
> with "the") but I can't see how to do that given that the object might be
> accessed from multiple different windows, which might be running in completely
> separate threads in some implementations.

I just posted about this to the whatwg group as well... Threading of windows etc is something that needs to be solved at a different level than this API, if one was to write an implementation that shared these objects between different threads then the objecst would simply need to be thread safe to permit concurrent access to the values etc, and with the proper locking it can all be made to work as expected.

> 
> As you're the first implementors, you get to tell me what you want. What do you
> want, given the constraint of handling this across totally independent windows
> in a way consistent with handling it across windows that are separate but can
> communicate?
> 

I want the method to always return the same instance for a given key. Syncronizing data etc between threads and/or processes is up to the implementation of this API.
Created attachment 220942 [details] [diff] [review]
Additional changes to implement session storage cloning and clone appropriately.

This implements nsDOMStorage::Clone() and fixes a problem where nsGlobalWindow::OpenInternal() didn't get the right URI when comparing domains.
The cloning code is actually not correct yet. We clone the current session storage object, but not the session storage objects for all domains in the opening window. I'll be looking into fixing that next...
Another thing I don't think we've got right in the patch yet is nsGlobalWindow::GetSessionStorage(). Right now it gets the domain from the caller and uses that to get to the session storage object. From reading the spec, it seems like it should use the domain of the window on which the session storage object is accessed. I'll change that too unless someone can tell me why the current code is what we'd want.
The cloning should only copy the one domain. See near the bottom of 4.9.4
Ah, right indeed. I got confused by the earlier paragraph that talks about cloning of browsing contexts, which is something we don't support.
Created attachment 220982 [details] [diff] [review]
Make the event code work right.

I think this fixes the event code to fire according to the spec, except for any batching or delaying for windows in the fastback cache (I'll be working on that next).
Created attachment 221058 [details] [diff] [review]
More fixes.

This makes windows in the fastback cache remember what storage events fired while the window was in the cache, implements the secure flag and proper checks for whether a caller is secure or not. And this also adds a bunch of missing security checks (i.e. whether the caller is secure and we're accessing secure items etc).
Created attachment 221060 [details] [diff] [review]
Complete diff with all the above changes.

This contains all of the above changes and is AFAICT a complete (or near at least) implementation of the spec. There's things that could be optimized etc, but that's a secondary bug IMO.
Attachment #221060 - Flags: review?

Updated

11 years ago
Attachment #221060 - Flags: review? → review?(vladimir)
Comment on attachment 221060 [details] [diff] [review]
Complete diff with all the above changes.

Don't review this yet. It would have been nice if this had been coordinated a bit more, so that we didn't end up duplicating effort. I have a number of changes to dom/src/storage that may or may not need to go in.
Attachment #221060 - Flags: review?(vladimir)
Created attachment 221309 [details] [diff] [review]
Merged Changes

Merged changes, but I'm getting a crash at:

       mPendingStorageEvents->Put(Substring(someData,
                                           someData + nsCRT::strlen(someData)),
                                 PR_TRUE);

when I call SetItem on a session store. 'this' appears to be invalid.
Attachment #219881 - Attachment is obsolete: true
Attachment #220599 - Attachment is obsolete: true
Attachment #220840 - Attachment is obsolete: true
Attachment #221060 - Attachment is obsolete: true
Attachment #221309 - Flags: review?(vladimir)
The crash could be a conflict with the patch from bug 336359 which added an observer to GlobalWindow in a different way.
OK, the crash doesn't occur on the branch.
Comment on attachment 221309 [details] [diff] [review]
Merged Changes


Looks fine, though:

>+  service = do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  rv = service->OpenDatabase(storageFile, getter_AddRefs(mConnection));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRBool exists;
>+  rv = mConnection->TableExists(NS_LITERAL_CSTRING("moz_webappsstore"), &exists);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (! exists) {
>+    rv = mConnection->ExecuteSimpleSQL(
>+           NS_LITERAL_CSTRING("CREATE TABLE moz_webappsstore ("
>+                              "domain TEXT, "
>+                              "key TEXT, "
>+                              "value TEXT, "
>+                              "secure INTEGER) "));
>+    NS_ENSURE_SUCCESS(rv, rv);

I think you want to create some indexes here, at least on domain and domain,key since your queries will generally only reference those two combinations.  something like:

CREATE INDEX moz_webappsstore_domain_index ON moz_webappsstore(domain);
CREATE INDEX moz_webappsstore_domain_key_index ON moz_webappsstore(domain,key);
Attachment #221309 - Flags: review?(vladimir) → review+
Created attachment 221336 [details] [diff] [review]
Smae patch for 1.8 Branch
The code I didn't write looks OK. Some things to do in the future or file follow up bugs on:

- attempting to call the key function when there is an inaccessible secure key in the list throws an NS_ERROR_DOM_INDEX_SIZE_ERR error
- inconsistency between whether NS_ERROR_DOM_INVALID_ACCESS_ERR or NS_ERROR_DOM_SECURITY_ERR should be thrown for security issues
- add some indicies for database lookups
- observer on GlobalWindow should be a seperate object as in bug 336359
Comment on attachment 221309 [details] [diff] [review]
Merged Changes

Looks all good to me. I'll see if I can reproduce the trunk crash you mentioned, but this should be good to go for the branch at least.
Attachment #221309 - Flags: superreview+

Updated

11 years ago
Blocks: 337193
Created attachment 221375 [details] [diff] [review]
Move db and storage earlier in build order
Attachment #221375 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #221375 - Flags: review? → review?(benjamin)
Comment on attachment 221375 [details] [diff] [review]
Move db and storage earlier in build order

r=shaver for build bustage
Attachment #221375 - Flags: review?(benjamin) → review+
Comment on attachment 221375 [details] [diff] [review]
Move db and storage earlier in build order

bsmedberg, can you give this a look as well?
Attachment #221375 - Flags: review?(benjamin)
We should not change nsIDocShell on the 1.8 branch.  Please revert this change or refactor it to use a new interface.  Thanks.
(Assignee)

Updated

11 years ago
Depends on: 337205
Depends on: 337208
(Bug 337205 tracks the refactoring of the docshell interface.)
Comment on attachment 221375 [details] [diff] [review]
Move db and storage earlier in build order

interesting, I figured we'd have to do this at some point but I didn't figure it to happen so soon
Attachment #221375 - Flags: review?(benjamin) → review+

Comment 36

11 years ago
You guys meant nsIDocShell_MOZILLA_1_8_BRANCH or nsIDocShell2 right?  You didn't mean to change nsIDocShell on the branch where we have said that we aren't changing any interfaces, right? ;-)

Can this be fixed before releasing bon echo a2?
(In reply to comment #36)
> Can this be fixed before releasing bon echo a2?

See comment 34.
This checkin appears to have broken Camino on the 1.8 branch (note the Guilty column on the Cm tinderbox page only shows trunk checkins, not any branch checkins, but the build error is in nsGlobalWindow.o, which seems relevant to some of the files touched here and the bustage matches the timing of the 1.8 checkin here).  Will bug 337205 fix this bustage, or is more work needed?

Comment 39

11 years ago
Created attachment 221407 [details] [diff] [review]
patch to try to fix thunderbird build bustage

this patch should make thunderbird start building moz_storage on the branch to fix the build problems now that DOM depends on mozstorage. 

I'd like to suggest that we consider adding build flags (via ifdefs?) to the new dom code so you can still build mozilla without mozstorage. I suspect some folks like minimo, seamonkey, camino and others don't build mozstorage and in the case of minimo, probably don't want mozstorage anyway. It would be a shame to require them to build it now.

Comment 40

11 years ago
I'm band-aiding the problem for Thunderbird, but if we don't end up ifdefing the dependency on moz_storage in the dom code with a MOZ_STORAGE ifdef, then we should really force moz_storage to be defined for all projects. And that should probably get discussed with all the project leads. 

I don't think it makes sense for everyone to have to add moz_storage=1 to their project configuration information when you can't build without storage. My two cents anyway :).

Comment 41

11 years ago
Created attachment 221408 [details] [diff] [review]
Windows bustage fix

I checked this in on branch; it should go on trunk with the rest of the patch.

Comment 42

11 years ago
Smokey Ardisson, camino will need to add --enable-storage like tbird...

But frankly, if we're having DOM depend on --enable-storage, it should be enabled by default like mscott says.
I filed 337208 earlier for the storage-enabling issue :)  I do think that we should be able to build without storage; it's a matter of getting a patch in for some ifdefs in the implementation of the global storage stuff to throw NS_ERROR_NOT_IMPLEMENTED or somesuch.
I'm fairly sure that tree rules include not breaking Camino builds.

Changes which might break Camino shouldn't land (or should be backed out) until the appropriate Camino parts are ready to land.

Advanced notice should be the smallest requirement for landing a change that breaks Camino and, if the build is unexpectedly broken, the change should be backed out until Camino developers have been given a reasonable amount of time to land a fix.

The above should be doubly true for the branch.
OS: Windows XP → All
Hardware: PC → All
Blocks: 337261

Comment 45

11 years ago
Actually, this has broken SeaMonkey on the 1.8 branch as well.

IMHO, breaking the non-Firefox world on a kind-of-stable branch is completely unacceptable.
If you introduce a dependency, care that it is fulfilled in all "normal" and sane configurations or don't introduce it as a hard dependency.

In my opionion, this has to be backed out on the branch until it can be fixed to not break builds with default configurations.
why does this bug have no indication what patches landed and where?
Because we're all out to get you.  I'm not sure what you mean by asking "why?" -- there are no indications because people didn't make such indications, obviously, which I don't find surprising in the heat of yesterday's tree gymnastics.

If you want to know what patches landed where, just ask that: 221336, 221375, 221407 and 221408 landed on the 1.8 branch.  Nothing has landed on trunk because of trunk closures and an outstanding crash there (see comment 24).

That the other products were broken because they don't have storage enabled is unfortunate, and was unintentional, as was the breaking of the clobber build due to build-order issues.  I still think that the right course there was and is for the those products to add storage (or for us to make it default to on, see bug 337208), rather than backing the whole thing out to land it later with a few different lines of Makefile.in.

Discussions of whether and how to support non-mozStorage builds should go in another bug or thread.
Keywords: fixed1.8.1
No longer blocks: 337261
Depends on: 337261
This caused bug 337259 on the branch.
Depends on: 337259

Updated

11 years ago
Depends on: 337311

Comment 49

11 years ago
I filed bug 337311 on the security checks this patch added...

Updated

11 years ago
Depends on: 337342

Updated

11 years ago
No longer depends on: 337342
Created attachment 221510 [details] [diff] [review]
Complete branch backout.

This is the patch that backs out this change from the branch and all the followup fixes etc that went in since it initially landed.

Updated

11 years ago
Depends on: 337342
Created attachment 221832 [details] [diff] [review]
dom/src/storage changes

adds ifdef MOZ_STORAGE and fixes some bugs

Comment 52

11 years ago
When we reland this, could we put the classinfo IDs at the end of the non-optional list or something?  Or are we no longer bothering with that sort of thing?

Also, what's up with the license change to dom/public/idl/Makefile.in ?

> *aSecure = (PRBool)secureInt;

Is secureInt guaranteed to be 0 or 1?  If so, please assert that?  If not, this code is wrong.

Updated

11 years ago
Depends on: 337755
Flags: blocking1.8.1?
Keywords: fixed1.8.1
Target Milestone: --- → mozilla1.8.1alpha3
Version: Trunk → 1.8 Branch

Comment 53

11 years ago
Re-targeting to a3
No longer blocks: 337193
Created attachment 221879 [details] [diff] [review]
Additional patch that'll apply ontop of the other patches in this bug.

This fixes various build issues and adds an #ifdef MOZ_STORAGE to nsGlobalWindow.
Created attachment 221904 [details] [diff] [review]
Yet more misc changes.

This I think fixes bug 337311 and most of bug 337342 (except issue 2 which is unclear in the spec), and it also includes Neil's fix (with a modification mentioned in that bug) for bug 337311. This also includes some other minor cleanup etc.

I'll attach a new trunk diff that includes all the changes in one diff.
Created attachment 221905 [details] [diff] [review]
Complete trunk diff with all the above changes.
One additional change is that db now has a dependency on netwerk on the trunk, so it, and storage, need to be moved afterwards.
Created attachment 222461 [details] [diff] [review]
Move db and storage after netwerk.
Comment on attachment 221904 [details] [diff] [review]
Yet more misc changes.

>+
>+            nsCOMPtr<nsIScriptObjectPrincipal> sop =
>+                do_QueryInterface(mScriptGlobal);
>+            nsCOMPtr<nsIURI> currentCodebase;
>+
>+            if (sop) {
>+                nsIPrincipal *principal = sop->GetPrincipal();
>+
>+                if (principal) {
>+                    principal->GetURI(getter_AddRefs(currentCodebase));
>+                }
>+            }


Does this get the principal of the document in the docshell, or the one of the current script?

> 
>   nsCOMPtr<nsIDOMStorage> storage(do_QueryWrappedNative(wrapper));
> 
>+  // GetItem() will return an error if the caller can't access the
>+  // session storage item.

Actually, it just returns null, as the spec suggests.

>+  if (currentCodebase && newDocShell && mDocShell && url.get()) {
>+    nsCOMPtr<nsIURI> newURI;
> 
>     JSContext       *cx;
>     PRBool           freePass;
>     BuildURIfromBase(url, getter_AddRefs(newURI), &freePass, &cx);
> 
>-    // XXXjst: What about redirects to different domains?
>-
>-    if (thisURI && newURI) {
>+    if (currentCodebase && newURI) {

currentCodebase was null-checked above.
(In reply to comment #59)
> (From update of attachment 221904 [details] [diff] [review] [edit])
> >+            if (sop) {
> >+                nsIPrincipal *principal = sop->GetPrincipal();
> >+
> >+                if (principal) {
> >+                    principal->GetURI(getter_AddRefs(currentCodebase));
> >+                }
> >+            }
> 
> Does this get the principal of the document in the docshell, or the one of the
> current script?

It gets the principal of the document in the docshell in which the link was clicked, and thus compares to see if the document where the link was clicked is from the same domain as the URL the link points to.

> >+  // GetItem() will return an error if the caller can't access the
> >+  // session storage item.
> 
> Actually, it just returns null, as the spec suggests.

Ah, true. Fixed the comment.

[...]
> 
> currentCodebase was null-checked above.
> 

Indeed. Fixed locally.
Comment on attachment 221905 [details] [diff] [review]
Complete trunk diff with all the above changes.

r/sr=jst for Neils changes in this diff.
Attachment #221905 - Flags: superreview+
Comment on attachment 221905 [details] [diff] [review]
Complete trunk diff with all the above changes.

The dom/base/src changes look ok too. Probably want to have someone else look at the security parts as well.
Attachment #221905 - Flags: review+
Created attachment 222581 [details] [diff] [review]
Complete trunk diff with all the above changes.

This one's going in on the trunk.

Comment 64

11 years ago
I believe this change has caused BeOS building to fail.  Please see bug 338512.
That was fixed right after the initial landing (once tinderbox showed the problem).
Created attachment 222636 [details] [diff] [review]
Post trunk landing fixes.

These changes went in after the initial landing on the trunk to fix some build bustage (that somehow never made it over from the 1.8 branch changes), and some other minor changes I found while fixing build bustage.
This seems to have been checked in:

http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/dom/src/base/nsGlobalWindow.cpp&rev=MOZILLA_1_8_BRANCH&mark=1.761.2.37

Should we be marking it fixed1.8.1 and removing the blocking status?
Retargetting to beta1 along with bug 337311, when that one closes (need bz to verify) so will this one. Shouldn't block alpha3 release, though.
Target Milestone: mozilla1.8.1alpha3 → mozilla1.8.1beta1

Updated

11 years ago
Depends on: 339940

Updated

11 years ago
Depends on: 340072

Updated

11 years ago
Blocks: 340144

Updated

11 years ago
No longer blocks: 340144
Depends on: 340144
This is done
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Does this code follow the global and per-site cookie prefs?

Comment 71

11 years ago
Filed bug 341524 on following the cookie prefs.

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Updated

11 years ago
Keywords: fixed1.8.1

Comment 72

11 years ago
You know, looking over the source code, I don't see any checks in setItem() to make sure there is a storage limit per domain name.... in my own testing against the client side cache, I haven't been able to hit a limit yet, and I've definently gone over the 5 MB limit suggested in the WHAT WG spec. Seems dangerous.

Comment 73

11 years ago
Yeah, we might want to put in a limit.  File a bug?

Comment 74

11 years ago
Filed bug 362446
Depends on: 362446
You need to log in before you can comment on or make changes to this bug.