773373 - Implement "localStorage" jars for apps

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Basically we need to key on additional information from the nsIPrincipal which is added in bug 758258 when determining which localStorage data to access.

Comment 1

[Honza Bambas (:mayhemer)]

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

Comment 2

[Kai-Chih Hu [:khu]]

Who could be the owner for this case?

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Part 1: Remove some dead code

Just remove a function that's no longer used.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 6

[Jan Varga [:janv]]

Attachment: Part 3: Additional URI -> principal fixes

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

Comment 7

[Jan Varga [:janv]]

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

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 9

[Jan Varga [:janv]]

Attachment: Part 4: Add appId and isInBrowserElement to key generation

Comment 10

[Jan Varga [:janv]]

Attachment: Part 5: Automatic tests

Comment 11

[Jan Varga [:janv]]

Attachment: Part 5: Automatic tests

Comment 12

[Jan Varga [:janv]]

Attachment: Part 5: Automatic tests

Comment 13

[Jan Varga [:janv]]

Ok, so the test passes on all platforms except the android :(

Comment 14

[Jan Varga [:janv]]

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 ?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 16

[Mounir Lamouri (: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 ;)

Comment 17

[Jan Varga [:janv]]

(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

Comment 18

[Jan Varga [:janv]]

Attachment: Part 5: Automatic tests

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Jan: is part 5 ready for review?

Comment 20

[Jan Varga [:janv]]

yeah, I just fixed those nits and Mounir said that "<3 sync APIs" was just a joke

Comment 21

[Mounir Lamouri (:mounir)]

Those patches pass try except the new tests that have to be disabled on Android:
https://tbpl.mozilla.org/?tree=Try&rev=2a44d3ae4a33

Comment 22

[Mounir Lamouri (:mounir)]

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)

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Part 6 - Disable tests on Android

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ede3e79682af
https://hg.mozilla.org/integration/mozilla-inbound/rev/c80ba174e4b9
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4198733790b
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c70558b823f
https://hg.mozilla.org/integration/mozilla-inbound/rev/423c12f60b5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e92aec1bea5

Thanks everyone for quick reviews and quick patches!! I'm very happy to have this landed!

Comment 25

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/ede3e79682af
https://hg.mozilla.org/mozilla-central/rev/c80ba174e4b9
https://hg.mozilla.org/mozilla-central/rev/e4198733790b
https://hg.mozilla.org/mozilla-central/rev/3c70558b823f
https://hg.mozilla.org/mozilla-central/rev/423c12f60b5d
https://hg.mozilla.org/mozilla-central/rev/7e92aec1bea5