Implement "localStorage" jars for apps

RESOLVED FIXED in mozilla18

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sicking, Assigned: janv)

Tracking

(Blocks: 1 bug, {feature})

Trunk
mozilla18
feature
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:S][WebAPI:P1][qa-])

Attachments

(6 attachments, 3 obsolete attachments)

Basically we need to key on additional information from the nsIPrincipal which is added in bug 758258 when determining which localStorage data to access.
blocking-basecamp: --- → +
localStorage uses a key based on origin to store/query entries bound to that origin in the database.  The form is like: "moc.allizom.www.:80" (= <reversed-host-name>.:<port-number>).  It is used to easily query usage by a domain and all its subdomains.  

Thus, for apps I presume there will be more added to this DB key string, probably to the end of it, right?

It is being built in nsDOMStorageDBWrapper::CreateOriginScopeDBKey().  It now takes an URI, but it can be relatively easily changed to take nsIPrincipal directly.  

DOMStorageBase::InitAsSessionStorage/InitAsLocalStorage has to be changed to take nsIPrincipal.  

We also have to preserve e10s behavior, but keys are built on the child side, so it will "just work".
Whiteboard: [LOE:S]

Comment 2

6 years ago
Who could be the owner for this case?
Assignee: nobody → jonas
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P1]
Created attachment 659092 [details] [diff] [review]
Part 1: Remove some dead code

Just remove a function that's no longer used.
Created attachment 659093 [details] [diff] [review]
Part 2: Use principals instead of URIs

This patch is very untested, but should generally work. It doesn't add appID and browserFlag to the key though, so no functional changes
The remaining work here is adding the appID and browserflag to nsDOMStorageDBWrapper::CreateScopeDBKey and nsDOMStorageDBWrapper::CreateQuotaDBKey. And making sure that everything still works.

Note that we should only add the Appid+browserflag to the scopekey if appID != 0 or browserflag != false. Otherwise we would lose all existing data since they would use a new key.


Note that ideally we should stop storing these keys using reversed domains. But I don't think that's worth doing at this time since we should migrate localStorage to use the new temporary storage area (once that's done). So it's not really worth migrating the data between different data formats multiple times.


Jan has awesomely offered to help drive this one home. Jan: If you have any questions, don't hesitate to ask.

The attached patches are pretty untested, so expect to find bugs. Fortunately we have pretty good localStorage tests.

The patches apply on top of the patch in bug 781866, which apply on top of the patch in bug 776416
Assignee: jonas → Jan.Varga
Keywords: feature
(Assignee)

Comment 6

6 years ago
Created attachment 661573 [details] [diff] [review]
Part 3: Additional URI -> principal fixes

This fixes 2 mochitests (one of them is a crash)
(Assignee)

Comment 7

6 years ago
(In reply to Honza Bambas (:mayhemer) from comment #1)
> Thus, for apps I presume there will be more added to this DB key string,
> probably to the end of it, right?

hmm, I think the key should start with appId and isInBrowserElement flag
for example:
moc.allizom.www.:80
0:t:moc.allizom.www.:80
1:f:moc.allizom.www.:80
2:f:moc.allizom.www.:80

if we add it to the end of the key, then the quota limit will be shared by all apps, no ?
Yeah, we probably need to prepend the appid+browserflag. That will also make it easier/faster to delete all localStoage data associated with a particular app when the app is uninstalled or when we clear private data for an app.
(Assignee)

Comment 9

6 years ago
Created attachment 661611 [details] [diff] [review]
Part 4: Add appId and isInBrowserElement to key generation
(Assignee)

Comment 10

6 years ago
Created attachment 661612 [details] [diff] [review]
Part 5: Automatic tests
(Assignee)

Comment 11

6 years ago
Created attachment 661613 [details] [diff] [review]
Part 5: Automatic tests
Attachment #661612 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 12

6 years ago
Created attachment 661746 [details] [diff] [review]
Part 5: Automatic tests
Attachment #661613 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
Ok, so the test passes on all platforms except the android :(
(Assignee)

Comment 14

6 years ago
The android failure is not specific to this bug, even the IDB app isolation test fails on android.

Jonas, would you review part 4 and 5 ?
Who should review part 1 and part 2, honzab ?
Attachment #661611 - Flags: review?(jonas) → review+
Comment on attachment 661746 [details] [diff] [review]
Part 5: Automatic tests

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

Actually, it would probably be better if Mounir reviewed this. He knows this stuff better.
Attachment #661746 - Flags: review?(jonas) → review?(mounir)
Comment on attachment 661746 [details] [diff] [review]
Part 5: Automatic tests

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

r=me with the nits addressed.

::: dom/tests/mochitest/localstorage/test_appIsolation.html
@@ +100,5 @@
> +      iframe.addEventListener('mozbrowsershowmodalprompt', function(e) {
> +        is(e.detail.message, "success", "test number " + i);
> +
> +        // Calling removeChild() produces an error that creates failures.
> +        //document.getElementById('content').removeChild(iframe);

Please, uncomment this. It should work now.

@@ +132,5 @@
> +
> +function startTest()
> +{
> +  localStorage.setItem("0", "foo");
> +  is(localStorage.getItem("0"), "foo", "data have been written");

<3 sync APIs

@@ +143,5 @@
> +</script>
> +
> +</head>
> +
> +<body onload="startTest();">

I would prefer addLoadEvent(startTest);

@@ +146,5 @@
> +
> +<body onload="startTest();">
> +<div id="content" style="display: none">
> +
> +</div>

That <div> is quite useless ;)
Attachment #661746 - Flags: review?(mounir) → review+
Blocks: 786301
(Assignee)

Comment 17

6 years ago
(In reply to Mounir Lamouri (:mounir) from comment #16)
> @@ +132,5 @@
> > +
> > +function startTest()
> > +{
> > +  localStorage.setItem("0", "foo");
> > +  is(localStorage.getItem("0"), "foo", "data have been written");
> 
> <3 sync APIs

what do you mean here ?

btw, I added |is(localStorage.getItem("0"), null, "no data");|
before |localStorage.setItem("0", "foo");| in the meantime
(Assignee)

Comment 18

6 years ago
Created attachment 663791 [details] [diff] [review]
Part 5: Automatic tests
Attachment #661746 - Attachment is obsolete: true
Jan: is part 5 ready for review?
(Assignee)

Comment 20

6 years ago
yeah, I just fixed those nits and Mounir said that "<3 sync APIs" was just a joke
Those patches pass try except the new tests that have to be disabled on Android:
https://tbpl.mozilla.org/?tree=Try&rev=2a44d3ae4a33
Comment on attachment 663791 [details] [diff] [review]
Part 5: Automatic tests

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

That was already reviewed ;) (see obsolete patches)
Attachment #663791 - Flags: review?(mounir) → review+
Attachment #659092 - Flags: review?(honzab.moz) → review+
Attachment #659093 - Flags: review?(honzab.moz) → review+
Created attachment 664675 [details] [diff] [review]
Part 6 - Disable tests on Android
Attachment #664675 - Flags: review?(jonas)
Attachment #664675 - Flags: review?(jonas) → review+

Updated

6 years ago
Whiteboard: [LOE:S][WebAPI:P1] → [LOE:S][WebAPI:P1][qa-]
You need to log in before you can comment on or make changes to this bug.