786688 - Allow indexedDB to be created without window but be associated by the given domain / url

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect, P1)

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

At the moment windowless indexedDB is associated to the general chrome, which means that add-ons may easily run into conflicts. While it's possible to wrap `indexedDB` object to prefix db names on open / delete it's not ideal. Since requests, transactions, etc... expose properties that will require wrapping too, this can easily become overkill. It would be much better to have a way of specifying url to `initWindowless` to which it will be associated with.

Comment 1

[Gabor Krizsanits (INACTIVE)]

Added janv since I know he knows a lot about indexeddb in general. Jan: is there a chance you can take a look at this and give me some hint how should I implement it? So the problem is that in case of addons, indexeddb is created with chrome principal, but we would like each addon use their own separate db, without running into any conflicts with each other, or with anything else.

Comment 2

[Jan Varga [:janv]]

cc'ing Jonas and Ben

there's a similar work in progress for installed apps

Comment 3

[Jan Varga [:janv]]

The "chrome" origin should be probably prefixed with addon id or something similar
(like we do for installed apps)

Comment 4

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

(In reply to Irakli Gozilalishvili [:irakli] [:gozala] [@gozala] from comment #0)
> At the moment windowless indexedDB is associated to the general chrome,
> which means that add-ons may easily run into conflicts. While it's possible
> to wrap `indexedDB` object to prefix db names on open / delete it's not
> ideal. Since requests, transactions, etc... expose properties that will
> require wrapping too

I'm not sure what you mean when you say that you need to wrap requests and transactions.

"Simply" wrapping the indexedDB property and prefixing calls to .open and .delete should be enough.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

> "Simply" wrapping the indexedDB property and prefixing calls to .open and .delete
> should be enough.

That's exactly what we do right now.

> I'm not sure what you mean when you say that you need to wrap requests and
> transactions.

I was assuming that `event.target` passed to `onsuccess`, `onerror`, etc.. will be unwrapped `indexedDB` object giving away access to a shared `open` `delete` methods.

Comment 6

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

There's never any events fired on the indexedDB object. They are only fired on IDBDatabase, IDBTransaction or IDBRequest objects.

Comment 7

[Gabor Krizsanits (INACTIVE)]

At this point I would really like to see an example that is failing/conflicting if we still consider this a bug I should fix on platform side. Or is Jonas's answer enough for us to start using indexeddb in jetpack as it is now without any change?

Comment 8

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

It looks like our current workaround is a proper solution, so I'm just closing this one as wontfix.

Thanks

Comment 9

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Excellent

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Jonas actually here is one of the cases where I would say behavior will be surprising:

var { indexedDB } = require('indexed-db');
var request = indexedDB.open('MyDatabase');
request.onsuccess = function(event) {
  console.log('Opened data base: ' + request.result.name);
};

This example will not log expected message: 

"Opened data base: MyDatabase"

Instead it will log:

"Opened data base: addonid:MyDatabase"


I could see this causing issues if on tries to use `db.name` instead of string name.

Comment 11

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

That's indeed true. And fixing this would introduce the complexity mentioned earlier about wrapping event targets.

I actually started working on a patch to do something like this in bug 789348 attachment 659066 [details] [diff] [review]. Never finished it though since it turned out we didn't need it.

Jan is on vacation right now, but might be able to finish that work up once he gets back.

What work is this bug blocking? How urgent is it?

Comment 12

[Gabor Krizsanits (INACTIVE)]

(In reply to Jonas Sicking (:sicking) from comment #11)
> What work is this bug blocking? How urgent is it?

It is the remaining last bit to support indexeddb from jetpack addons. It is P1, so I'd gladly put some work into this to have it fixed.

Comment 13

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

You could try the dom/ parts of attachment 659066 [details] [diff] [review] and see if that works for you.

Comment 14

[Gabor Krizsanits (INACTIVE)]

So I think I can use that patch in a way... I talked with bholley about this and we both agree that combining system principal with appIds and extended origins is probably a bad idea, since the system principal is a singleton and there are too many equal checks. So what I had in mind is something like IDBFOpenForPrincipal, but instead of a principal using only a string for the origin. But I'm not entirely sure about the role of that asciiOrigin in indexeddb.

So what is the relationship between the principal of the database, the asciiOrigin, and the location where the database is stored?

As it turns out this is pretty urgent for us, so in the meanwhile we will think about some kind of workaround that can be replaced by this patch later, without any API breakage.

Comment 15

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

You can "simply" create a principal which uses whatever you want as the URL, and NO_APP_ID as appid and false as browserflag. That will let you for example create indexedDB databases for "http://appdata.jetpack.whatever.mozilla.org"

Comment 16

[Gabor Krizsanits (INACTIVE)]

(In reply to Jonas Sicking (:sicking) from comment #15)
> You can "simply" create a principal which uses whatever you want as the URL,
> and NO_APP_ID as appid and false as browserflag. That will let you for
> example create indexedDB databases for
> "http://appdata.jetpack.whatever.mozilla.org"

Right... The name tricked my, I was assuming that the principal will be used in some way in the inexeddb, which scared me a little, but only the origin is. Yeah sure, we can do that too.

Irakli: so I looked at how the directory is created. And the thing is if we release now as it is and we change this later, that will mean addons all of a sudden will have their databases stored at a different location. So it would not be only an API breakage but even they would lose all the data after update... I'm not sure how to progress from here, your call.

Comment 17

[Dave Townsend [:mossop] (PTO until Aug 21st)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16)
> Irakli: so I looked at how the directory is created. And the thing is if we
> release now as it is and we change this later, that will mean addons all of
> a sudden will have their databases stored at a different location. So it
> would not be only an API breakage but even they would lose all the data
> after update... I'm not sure how to progress from here, your call.

How long do you think it would take to implement this and land on m-c?

Comment 18

[Gabor Krizsanits (INACTIVE)]

(In reply to Dave Townsend (:Mossop) from comment #17)
> How long do you think it would take to implement this and land on m-c?

So we could pretty much use the patch as it is. I wanted to change something on it, but that's probably not that important as Jonas pointed it out. The question is when will the patch reviewed and landed... Jonas can you give us any estimation?

Comment 19

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

The remaining work is to write tests and verify that the patch works. I haven't done any more testing than to ensure that it builds.

I won't have time to do that work though. Possibly Jan Varga will have time once he is back from vacation next week.

Alternatively if someone else can do that, I'm happy to review.

Comment 20

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/18e6b0f270bee958453f74746cd917c1feef301b
Reverting indexedDB API until Bug 786688 is fixed. a=@gozala

Comment 21

[[github robot]]

Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/c0ec47bae2ea392b1ff552e96baa23d8218105f8
Reverting indexedDB API until Bug 786688 is fixed. a=@gozala(cherry picked from commit 18e6b0f270bee958453f74746cd917c1feef301b)

Comment 22

[Jan Varga [:janv]]

I pushed a patch queue for this bug to try:
https://tbpl.mozilla.org/?tree=Try&rev=6a5efcf815d8

Comment 23

[Jan Varga [:janv]]

Attachment: Part 1: Add chrome only openForPrincipal() and deleteForPrincipal()

Comment 24

[Jan Varga [:janv]]

Attachment: Part 2: Code cleanup

Comment 25

[Jan Varga [:janv]]

Attachment: Part 3: Automatic tests

Comment 26

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 669869 [details] [diff] [review]
Part 1: Add chrome only openForPrincipal() and deleteForPrincipal()

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

::: dom/base/nsDOMClassInfo.cpp
@@ +8008,5 @@
>  
> +// IDBFactory helper
> +
> +/* static */
> +template<nsresult (*func)(JSContext *cx, unsigned argc, jsval *vp, bool aDelete), bool aDelete>

Nit: To make this a little clearer how about making a function pointer typedef? Or just remove the arg names.

@@ +8018,5 @@
> +    xpc::Throw(cx, rv);
> +    return false;
> +  }
> +
> +  return true;  

Nit: trailing whitespace.

@@ +8061,5 @@
> +  rv = static_cast<indexedDB::IDBFactory*>(factory.get())->
> +    OpenCommon(name, version, extendedOrigin, aDelete, cx,
> +               getter_AddRefs(request));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  

Nit: here too.

Comment 27

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ac59628202
https://hg.mozilla.org/integration/mozilla-inbound/rev/90e2bd5252aa
https://hg.mozilla.org/integration/mozilla-inbound/rev/be030e22b120

Comment 28

[Jan Varga [:janv]]

(In reply to ben turner [:bent] from comment #26)
> Comment on attachment 669869 [details] [diff] [review]
> Part 1: Add chrome only openForPrincipal() and deleteForPrincipal()
> 
> Review of attachment 669869 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/nsDOMClassInfo.cpp
> @@ +8008,5 @@
> >  
> > +// IDBFactory helper
> > +
> > +/* static */
> > +template<nsresult (*func)(JSContext *cx, unsigned argc, jsval *vp, bool aDelete), bool aDelete>
> 
> Nit: To make this a little clearer how about making a function pointer
> typedef? Or just remove the arg names.

removed the arg names

> 
> @@ +8018,5 @@
> > +    xpc::Throw(cx, rv);
> > +    return false;
> > +  }
> > +
> > +  return true;  
> 
> Nit: trailing whitespace.
> 
> @@ +8061,5 @@
> > +  rv = static_cast<indexedDB::IDBFactory*>(factory.get())->
> > +    OpenCommon(name, version, extendedOrigin, aDelete, cx,
> > +               getter_AddRefs(request));
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +  
> 
> Nit: here too.

already removed the extra whitespace in the cleanup patch

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0ac59628202
https://hg.mozilla.org/mozilla-central/rev/90e2bd5252aa
https://hg.mozilla.org/mozilla-central/rev/be030e22b120

Comment 30

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Would really like these change uplifted to Aurora if possible.

Comment 31

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, I'm merging it now.

Comment 32

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Comment on attachment 669869 [details] [diff] [review]
Part 1: Add chrome only openForPrincipal() and deleteForPrincipal()

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: the SDK waits 6 weeks longer to provide IndexedDB api.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 33

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Comment on attachment 669870 [details] [diff] [review]
Part 2: Code cleanup

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: the SDK waits 6 weeks longer to provide IndexedDB api.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 34

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Comment on attachment 669871 [details] [diff] [review]
Part 3: Automatic tests

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: the SDK waits 6 weeks longer to provide IndexedDB api.
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 35

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to ben turner [:bent] from comment #31)
> Yeah, I'm merging it now.

Er, oops. This comment was meant for a different bug. I haven't looked at merging this at all.

Comment 36

[bhavana bajaj [:bajaj]]

(In reply to Jeff Griffiths (:canuckistani) from comment #32)
> Comment on attachment 669869 [details] [diff] [review]
> Part 1: Add chrome only openForPrincipal() and deleteForPrincipal()
> 
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): 
> User impact if declined: the SDK waits 6 weeks longer to provide IndexedDB
> api.

 Was this the case for FF17,FF16.. ?

> Testing completed (on m-c, etc.): 

Any testing around this ?

> Risk to taking this patch (and alternatives if risky): 
can you please provide the risk assessment ?
> String or UUID changes made by this patch:

 Not clear to me why this needs to land on FF18 and not ride the trains ?

Comment 37

[Jan Varga [:janv]]

Testing completed (on m-c, etc.): 
Mochitests landed on m-c a week ago.

Risk to taking this patch (and alternatives if risky): 
There's very little risk. The patch logic is simple. New API can be accessed only from chrome/JS components

String or UUID changes made by this patch:
No UUID changes

Comment 38

[Jan Varga [:janv]]

and if I remember correctly this is the last feature which blocks JetPack team from releasing a new version ?

Comment 39

[Gabor Krizsanits (INACTIVE)]

It's blocking jetpack to release indexeddb support at least.

Comment 40

[bhavana bajaj [:bajaj]]

Comment on attachment 669869 [details] [diff] [review]
Part 1: Add chrome only openForPrincipal() and deleteForPrincipal()

Approving on aurora, considering it is low risk and has good mochitest coverage

Comment 41

[Jan Varga [:janv]]

I'm going to test patches on aurora, will land if everything works ok

Comment 42

[Jan Varga [:janv]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/65bf4be85f7d
https://hg.mozilla.org/releases/mozilla-aurora/rev/2946ed324df5
https://hg.mozilla.org/releases/mozilla-aurora/rev/c07b17e59971

Comment 43

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/46d1e3ce7302e5923147b2a9094e5b24325ac7a4
Expose IndexedDb api (bug 786688)

Compared to previous commit (862ba11a):

* remove db prefixing
* per-addon security principal
* all strings changed to double quotes
* freeze `exports.indexedDB`
  - (heritage.extend, rather than `object.Create`)
  - includes test
* use `addon://` fake scheme in principal
* makeFunc replaced with `bind`
* sanitized "indexeddb" uri for principal

Risks:

* will make data created using previous sdk INACESSIBLE
  (unless they take effort to find the data and export it,
  which is non-trivial)
* relying on `sanitize(packageID)` might make data INACCESSIBLE in the
  future if we change how `packageID` is derived.

Doc updates:

* indexedDB docs reflect openForPrinciple
* external example usage
* changed 'aside' to reflect current bug status on protocols, principle
* removed outdated notes on Database Naming

All tests pass.

https://github.com/mozilla/addon-sdk/commit/309c69f87ce4f2491a97ecde7192c1c98cb06362
Merge pull request #719 from gregglind/bug786688indexeddb

Expose IndexedDb api (bug 786688) r=@ochameau