We implemented shared-pool in bug 785884, which uses appId/isBrowser. In Bug 1163254 we will update the origin, so SessionStorage should take advantage of that to get the correct data jar.

Comment on attachment 8639814 [details] [diff] [review] Patch. Review of attachment 8639814 [details] [diff] [review]: ----------------------------------------------------------------- This deals with persistent storage, right? Can you explain why we don't need any migration code here?

(In reply to Bobby Holley (:bholley) from comment #3) > Comment on attachment 8639814 [details] [diff] [review] > Patch. > > Review of attachment 8639814 [details] [diff] [review]: > ----------------------------------------------------------------- > > This deals with persistent storage, right? Can you explain why we don't need > any migration code here? Sorry I forgot to explain. In the original code it just uses the origin info to keep track of visited origins. http://lxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStorage.jsm#78 it doesn't use origin to deal with persistent data here. It's DOMStorageManager (Bug 1165214) will handle the persistent data. And in the original bug for SessionStorage Bug 785884 it's also a one line change in SessionStorage.jsm. Therefore my patch is a one line change as well. But if you think I should duplicate this to Bug 1165214 I can do this. Thanks

(In reply to Yoshi Huang[:allstars.chh] from comment #4) > it doesn't use origin to deal with persistent data here. > It's DOMStorageManager (Bug 1165214) will handle the persistent data. The value of |origin| is used as a key to |data|, which then gets returned to the caller, right? > And in the original bug for SessionStorage Bug 785884 it's also a one line > change in SessionStorage.jsm. > > Therefore my patch is a one line change as well. That's a fair argument. NI tim to make sure that this is the right thing.

Comment on attachment 8639814 [details] [diff] [review] Patch. Actually let's just have him review.

Comment on attachment 8639814 [details] [diff] [review] Patch. Review of attachment 8639814 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Bobby Holley (:bholley) from comment #5) > (In reply to Yoshi Huang[:allstars.chh] from comment #4) > > it doesn't use origin to deal with persistent data here. > > It's DOMStorageManager (Bug 1165214) will handle the persistent data. > > The value of |origin| is used as a key to |data|, which then gets returned > to the caller, right? We will use the |origin| later and pass it to ios.newURI() when restoring, to retrieve a principal and fill the right storage. Does that break it in any way? Another question is... do we really have to change Firefox Desktop? It seems that bug 1163254 is about a new FirefoxOS security model, would that suddenly break Desktop if we don't change anything?

(In reply to Tim Taubert [:ttaubert] from comment #7) > (In reply to Bobby Holley (:bholley) from comment #5) > > (In reply to Yoshi Huang[:allstars.chh] from comment #4) > > > it doesn't use origin to deal with persistent data here. > > > It's DOMStorageManager (Bug 1165214) will handle the persistent data. > > > > The value of |origin| is used as a key to |data|, which then gets returned > > to the caller, right? > > We will use the |origin| later and pass it to ios.newURI() when restoring, > to retrieve a principal and fill the right storage. Does that break it in > any way? Yes, you'll need to use BrowserUtils.principalFromOrigin instead, which handles the additional attributes. However, given that the current code computes origin as |principal.jarPrefix + principal.originNoSuffix|, I don't understand how this could possibly be working properly on trunk. > Another question is... do we really have to change Firefox Desktop? It seems > that bug 1163254 is about a new FirefoxOS security model, would that > suddenly break Desktop if we don't change anything? We are using OriginAttributes for lots of new projects, including Firefox Desktop ones like bug 1179557 (which will allow separate "user contexts" with different cookies etc). So we need to support them everywhere.

(In reply to Bobby Holley (:bholley) from comment #8) > However, given that the current code computes origin as |principal.jarPrefix > + principal.originNoSuffix|, I don't understand how this could possibly be > working properly on trunk. > Is this because SessionStorage.jsm is only running on Desktop? And on Desktop jarPrefix is "" so the code could work. I agree it will fail in newURI with jarPrefix on b2g. :P

try https://treeherder.mozilla.org/#/jobs?repo=try&revision=818116a0b3f7

Comment on attachment 8642979 [details] [diff] [review] Patch v2. Review of attachment 8642979 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!