335540 - Implement webapps session and persistent storage

Status: RESOLVED FIXED

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

Description

[Neil Deakin]

See URL for spec

Comment 1

[Neil Deakin]

Attachment: 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.

Comment 2

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 3

[Neil Deakin]

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?

Comment 4

[Johnny Stenback (:jst)]

Yes, I think we should merge the two into one.

Comment 5

[Johnny Stenback (:jst)]

Attachment: 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?

Comment 6

[Neil Deakin]

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

Comment 7

[Johnny Stenback (:jst)]

(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.

Comment 8

[Neil Deakin]

(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."

Comment 9

[Neil Deakin]

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.

Comment 10

[Johnny Stenback (:jst)]

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?

Comment 11

[Hixie (not reading bugmail)]

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?

Comment 12

[Neil Deakin]

Attachment: 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

Comment 13

[Johnny Stenback (:jst)]

(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.

Comment 14

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 15

[Johnny Stenback (:jst)]

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...

Comment 16

[Johnny Stenback (:jst)]

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.

Comment 17

[Neil Deakin]

The cloning should only copy the one domain. See near the bottom of 4.9.4

Comment 18

[Johnny Stenback (:jst)]

Ah, right indeed. I got confused by the earlier paragraph that talks about cloning of browsing contexts, which is something we don't support.

Comment 19

[Johnny Stenback (:jst)]

Attachment: 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).

Comment 20

[Johnny Stenback (:jst)]

Attachment: 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).

Comment 21

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 22

[Neil Deakin]

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.

Comment 23

[Neil Deakin]

Attachment: 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.

Comment 24

[Neil Deakin]

The crash could be a conflict with the patch from bug 336359 which added an observer to GlobalWindow in a different way.

Comment 25

[Neil Deakin]

OK, the crash doesn't occur on the branch.

Comment 26

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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);

Comment 27

[Neil Deakin]

Attachment: Smae patch for 1.8 Branch

Comment 28

[Neil Deakin]

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 29

[Johnny Stenback (:jst)]

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.

Comment 30

[Neil Deakin]

Attachment: Move db and storage earlier in build order

Comment 31

[Mike Shaver (:shaver emeritus)]

Comment on attachment 221375 [details] [diff] [review]
Move db and storage earlier in build order

r=shaver for build bustage

Comment 32

[Mike Shaver (:shaver emeritus)]

Comment on attachment 221375 [details] [diff] [review]
Move db and storage earlier in build order

bsmedberg, can you give this a look as well?

Comment 33

[Brian Ryner (not reading)]

We should not change nsIDocShell on the 1.8 branch.  Please revert this change or refactor it to use a new interface.  Thanks.

Comment 34

[Mike Shaver (:shaver emeritus)]

(Bug 337205 tracks the refactoring of the docshell interface.)

Comment 35

[Benjamin Smedberg]

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

Comment 36

[Darin Fisher]

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?

Comment 37

[:Gavin Sharp [email: [e-mail]]]

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

See comment 34.

Comment 38

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

[Scott MacGregor]

Attachment: 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

[Scott MacGregor]

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

[Boris Zbarsky [:bzbarsky]]

Attachment: Windows bustage fix

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

Comment 42

[Boris Zbarsky [:bzbarsky]]

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.

Comment 43

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 44

[Samuel Sidler (old account; do not CC)]

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.

Comment 45

[Robert Kaiser]

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.

Comment 46

[Christian :Biesinger (don't email me, ping me on IRC)]

why does this bug have no indication what patches landed and where?

Comment 47

[Mike Shaver (:shaver emeritus)]

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.

Comment 48

[:Gavin Sharp [email: [e-mail]]]

This caused bug 337259 on the branch.

Comment 49

[Boris Zbarsky [:bzbarsky]]

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

Comment 50

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 51

[Neil Deakin]

Attachment: dom/src/storage changes

adds ifdef MOZ_STORAGE and fixes some bugs

Comment 52

[Boris Zbarsky [:bzbarsky]]

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.

Comment 53

[Mike Schroepfer]

Re-targeting to a3

Comment 54

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 55

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 56

[Johnny Stenback (:jst)]

Attachment: Complete trunk diff with all the above changes.

Comment 57

[Neil Deakin]

One additional change is that db now has a dependency on netwerk on the trunk, so it, and storage, need to be moved afterwards.

Comment 58

[Johnny Stenback (:jst)]

Attachment: Move db and storage after netwerk.

Comment 59

[Neil Deakin]

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.

Comment 60

[Johnny Stenback (:jst)]

(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 61

[Johnny Stenback (:jst)]

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

r/sr=jst for Neils changes in this diff.

Comment 62

[Neil Deakin]

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.

Comment 63

[Johnny Stenback (:jst)]

Attachment: Complete trunk diff with all the above changes.

This one's going in on the trunk.

Comment 64

[Doug Shelton]

I believe this change has caused BeOS building to fail.  Please see bug 338512.

Comment 65

[Johnny Stenback (:jst)]

That was fixed right after the initial landing (once tinderbox showed the problem).

Comment 66

[Johnny Stenback (:jst)]

Attachment: 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.

Comment 67

[Mike Beltzner [:beltzner, not reading bugmail]]

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?

Comment 68

[Mike Beltzner [:beltzner, not reading bugmail]]

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.

Comment 69

[Neil Deakin]

This is done

Comment 70

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Does this code follow the global and per-site cookie prefs?

Comment 71

[Boris Zbarsky [:bzbarsky]]

Filed bug 341524 on following the cookie prefs.

Comment 72

[Brad Neuberg]

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

[Boris Zbarsky [:bzbarsky]]

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

Comment 74

[Boris Zbarsky [:bzbarsky]]

Filed bug 362446