Last Comment Bug 335540 - Implement webapps session and persistent storage
: Implement webapps session and persistent storage
Status: RESOLVED FIXED
: fixed1.8.1
Product: Core
Classification: Components
Component: DOM (show other bugs)
: 1.8 Branch
: All All
: -- normal with 1 vote (vote)
: mozilla1.8.1beta1
Assigned To: Neil Deakin
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
http://www.whatwg.org/specs/web-apps/...
Depends on: 337342 337205 337208 337259 337261 337311 337755 339940 340072 340144 362446
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-26 09:06 PDT by Neil Deakin
Modified: 2007-08-08 09:57 PDT (History)
31 users (show)
darin.moz: blocking1.8.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Work in progress (66.78 KB, patch)
2006-04-26 09:08 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Neil's patch and my work merged together. (105.22 KB, patch)
2006-05-02 20:19 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Additional changes to merge the two storage objects, and other random cleanup. (24.52 KB, patch)
2006-05-03 23:45 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Updated work in progress (100.48 KB, patch)
2006-05-04 14:59 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Additional changes to implement session storage cloning and clone appropriately. (6.04 KB, patch)
2006-05-05 09:40 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Make the event code work right. (9.20 KB, patch)
2006-05-05 14:08 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
More fixes. (12.62 KB, patch)
2006-05-05 22:39 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Complete diff with all the above changes. (102.71 KB, patch)
2006-05-05 22:42 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Merged Changes (121.02 KB, patch)
2006-05-08 09:09 PDT, Neil Deakin
vladimir: review+
jst: superreview+
Details | Diff | Splinter Review
Smae patch for 1.8 Branch (121.33 KB, patch)
2006-05-08 13:01 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Move db and storage earlier in build order (1.28 KB, patch)
2006-05-08 15:32 PDT, Neil Deakin
shaver: review+
benjamin: review+
Details | Diff | Splinter Review
patch to try to fix thunderbird build bustage (938 bytes, patch)
2006-05-08 21:33 PDT, Scott MacGregor
no flags Details | Diff | Splinter Review
Windows bustage fix (1.90 KB, patch)
2006-05-08 21:49 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Complete branch backout. (134.98 KB, patch)
2006-05-09 15:51 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
dom/src/storage changes (48.29 KB, patch)
2006-05-12 11:38 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Additional patch that'll apply ontop of the other patches in this bug. (4.12 KB, patch)
2006-05-12 18:32 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Yet more misc changes. (18.01 KB, patch)
2006-05-13 08:20 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Complete trunk diff with all the above changes. (107.98 KB, patch)
2006-05-13 08:25 PDT, Johnny Stenback (:jst, jst@mozilla.com)
enndeakin: review+
jst: superreview+
Details | Diff | Splinter Review
Move db and storage after netwerk. (541 bytes, patch)
2006-05-17 23:54 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Complete trunk diff with all the above changes. (130.83 KB, patch)
2006-05-18 21:41 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
Post trunk landing fixes. (4.29 KB, patch)
2006-05-19 09:20 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review

Description Neil Deakin 2006-04-26 09:06:24 PDT
See URL for spec
Comment 1 Neil Deakin 2006-04-26 09:08:29 PDT
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.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-02 20:19:48 PDT
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.
Comment 3 Neil Deakin 2006-05-03 06:56:00 PDT
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, jst@mozilla.com) 2006-05-03 15:13:05 PDT
Yes, I think we should merge the two into one.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-03 23:45:17 PDT
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?
Comment 6 Neil Deakin 2006-05-04 07:07:49 PDT
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, jst@mozilla.com) 2006-05-04 07:44:08 PDT
(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 2006-05-04 07:47:27 PDT
(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 2006-05-04 07:49:45 PDT
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, jst@mozilla.com) 2006-05-04 07:58:26 PDT
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) 2006-05-04 10:42:36 PDT
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 2006-05-04 14:59:39 PDT
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
Comment 13 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-04 15:35:11 PDT
(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, jst@mozilla.com) 2006-05-05 09:40:46 PDT
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.
Comment 15 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-05 09:58:24 PDT
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, jst@mozilla.com) 2006-05-05 10:00:53 PDT
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 2006-05-05 10:34:32 PDT
The cloning should only copy the one domain. See near the bottom of 4.9.4
Comment 18 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-05 12:34:31 PDT
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, jst@mozilla.com) 2006-05-05 14:08:34 PDT
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).
Comment 20 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-05 22:39:14 PDT
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).
Comment 21 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-05 22:42:09 PDT
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.
Comment 22 Neil Deakin 2006-05-06 09:03:10 PDT
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 2006-05-08 09:09:39 PDT
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.
Comment 24 Neil Deakin 2006-05-08 09:29:05 PDT
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 2006-05-08 12:37:21 PDT
OK, the crash doesn't occur on the branch.
Comment 26 Vladimir Vukicevic [:vlad] [:vladv] 2006-05-08 12:49:14 PDT
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 2006-05-08 13:01:11 PDT
Created attachment 221336 [details] [diff] [review]
Smae patch for 1.8 Branch
Comment 28 Neil Deakin 2006-05-08 13:26:21 PDT
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, jst@mozilla.com) 2006-05-08 14:13:52 PDT
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 2006-05-08 15:32:39 PDT
Created attachment 221375 [details] [diff] [review]
Move db and storage earlier in build order
Comment 31 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-05-08 15:43:05 PDT
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 -- probably not reading bugmail closely) 2006-05-08 15:43:52 PDT
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) 2006-05-08 16:04:14 PDT
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 -- probably not reading bugmail closely) 2006-05-08 17:02:07 PDT
(Bug 337205 tracks the refactoring of the docshell interface.)
Comment 35 Benjamin Smedberg [:bsmedberg] 2006-05-08 17:15:31 PDT
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 2006-05-08 17:55:23 PDT
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: gavin@gavinsharp.com] 2006-05-08 17:56:21 PDT
(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) 2006-05-08 21:22:59 PDT
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 2006-05-08 21:33:14 PDT
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 Scott MacGregor 2006-05-08 21:40:35 PDT
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 [:bz] (still a bit busy) 2006-05-08 21:49:52 PDT
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 Boris Zbarsky [:bz] (still a bit busy) 2006-05-08 21:51:40 PDT
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] 2006-05-08 22:22:47 PDT
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) 2006-05-08 22:41:48 PDT
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 2006-05-09 04:21:55 PDT
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) 2006-05-09 05:03:43 PDT
why does this bug have no indication what patches landed and where?
Comment 47 Mike Shaver (:shaver -- probably not reading bugmail closely) 2006-05-09 08:00:42 PDT
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: gavin@gavinsharp.com] 2006-05-09 09:11:16 PDT
This caused bug 337259 on the branch.
Comment 49 Boris Zbarsky [:bz] (still a bit busy) 2006-05-09 10:35:22 PDT
I filed bug 337311 on the security checks this patch added...
Comment 50 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-09 15:51:37 PDT
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.
Comment 51 Neil Deakin 2006-05-12 11:38:18 PDT
Created attachment 221832 [details] [diff] [review]
dom/src/storage changes

adds ifdef MOZ_STORAGE and fixes some bugs
Comment 52 Boris Zbarsky [:bz] (still a bit busy) 2006-05-12 13:30:42 PDT
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 2006-05-12 18:31:55 PDT
Re-targeting to a3
Comment 54 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-12 18:32:07 PDT
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.
Comment 55 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-13 08:20:49 PDT
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.
Comment 56 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-13 08:25:42 PDT
Created attachment 221905 [details] [diff] [review]
Complete trunk diff with all the above changes.
Comment 57 Neil Deakin 2006-05-13 13:10:05 PDT
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, jst@mozilla.com) 2006-05-17 23:54:13 PDT
Created attachment 222461 [details] [diff] [review]
Move db and storage after netwerk.
Comment 59 Neil Deakin 2006-05-18 12:59:09 PDT
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, jst@mozilla.com) 2006-05-18 13:47:28 PDT
(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, jst@mozilla.com) 2006-05-18 13:48:49 PDT
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 2006-05-18 13:59:27 PDT
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, jst@mozilla.com) 2006-05-18 21:41:06 PDT
Created attachment 222581 [details] [diff] [review]
Complete trunk diff with all the above changes.

This one's going in on the trunk.
Comment 64 Doug Shelton 2006-05-19 00:15:15 PDT
I believe this change has caused BeOS building to fail.  Please see bug 338512.
Comment 65 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-19 09:17:04 PDT
That was fixed right after the initial landing (once tinderbox showed the problem).
Comment 66 Johnny Stenback (:jst, jst@mozilla.com) 2006-05-19 09:20:53 PDT
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.
Comment 67 Mike Beltzner [:beltzner, not reading bugmail] 2006-05-25 11:32:04 PDT
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] 2006-05-25 12:46:07 PDT
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 2006-06-12 14:44:16 PDT
This is done
Comment 70 David Baron :dbaron: ⌚️UTC-8 2006-06-13 14:49:51 PDT
Does this code follow the global and per-site cookie prefs?
Comment 71 Boris Zbarsky [:bz] (still a bit busy) 2006-06-14 08:10:49 PDT
Filed bug 341524 on following the cookie prefs.
Comment 72 Brad Neuberg 2006-11-16 14:24:43 PST
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 [:bz] (still a bit busy) 2006-11-16 16:17:56 PST
Yeah, we might want to put in a limit.  File a bug?
Comment 74 Boris Zbarsky [:bz] (still a bit busy) 2006-11-30 22:48:38 PST
Filed bug 362446

Note You need to log in before you can comment on or make changes to this bug.