Closed Bug 1165277 Opened 5 years ago Closed 4 years ago

Use origin in SessionStorage.jsm

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox41 --- affected
firefox43 --- fixed

People

(Reporter: allstars.chh, Assigned: allstars.chh)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Component: Security: CAPS → DOM: IndexedDB
No longer depends on: 1163254
Blocks: 1179985
Assignee: nobody → allstars.chh
Attached patch Patch. (obsolete) — Splinter Review
Attached patch Patch. (obsolete) — Splinter Review
Attachment #8639813 - Attachment is obsolete: true
Attachment #8639814 - Flags: review?(bobbyholley)
Blocks: nsec-origins
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.
Flags: needinfo?(ttaubert)
Comment on attachment 8639814 [details] [diff] [review]
Patch.

Actually let's just have him review.
Flags: needinfo?(ttaubert)
Attachment #8639814 - Flags: review?(ttaubert)
Attachment #8639814 - Flags: review?(bobbyholley)
Attachment #8639814 - Flags: feedback+
Component: DOM: IndexedDB → Session Restore
Product: Core → Firefox
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?
Attachment #8639814 - Flags: review?(ttaubert)
(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
Attachment #8642979 - Flags: review?(ttaubert)
Comment on attachment 8642979 [details] [diff] [review]
Patch v2.

Review of attachment 8642979 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!
Attachment #8642979 - Flags: review?(ttaubert) → review+
https://hg.mozilla.org/mozilla-central/rev/e2eb44fd269b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.