Closed Bug 773373 Opened 8 years ago Closed 7 years ago

Implement "localStorage" jars for apps

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: sicking, Assigned: janv)

References

Details

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

Attachments

(6 files, 3 obsolete files)

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: --- → +
Depends on: 775861
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".
Who could be the owner for this case?
Assignee: nobody → jonas
Whiteboard: [LOE:S] → [LOE:S][WebAPI:P1]
Just remove a function that's no longer used.
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
This fixes 2 mochitests (one of them is a crash)
(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.
Attached patch Part 5: Automatic tests (obsolete) — Splinter Review
Attached patch Part 5: Automatic tests (obsolete) — Splinter Review
Attachment #661612 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch Part 5: Automatic tests (obsolete) — Splinter Review
Attachment #661613 - Attachment is obsolete: true
Ok, so the test passes on all platforms except the android :(
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
(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
Attachment #661746 - Attachment is obsolete: true
Jan: is part 5 ready for review?
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+
Attachment #664675 - Flags: review?(jonas) → review+
Whiteboard: [LOE:S][WebAPI:P1] → [LOE:S][WebAPI:P1][qa-]
Depends on: 797935
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.