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

RESOLVED FIXED

Status

()

Core
DOM: IndexedDB
P1
normal
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: irakli, Assigned: janv)

Tracking

unspecified
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

Attachments

(3 attachments)

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.
Assignee: nobody → gkrizsanits

Updated

5 years ago
Priority: -- → P1
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.
(Assignee)

Comment 2

5 years ago
cc'ing Jonas and Ben

there's a similar work in progress for installed apps
(Assignee)

Comment 3

5 years ago
The "chrome" origin should be probably prefixed with addon id or something similar
(like we do for installed apps)
(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.
> "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.
There's never any events fired on the indexedDB object. They are only fired on IDBDatabase, IDBTransaction or IDBRequest objects.
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?
It looks like our current workaround is a proper solution, so I'm just closing this one as wontfix.

Thanks
Excellent
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
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.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
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?
(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.
You could try the dom/ parts of attachment 659066 [details] [diff] [review] and see if that works for you.
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.
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"
(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.
(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?
(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?
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

5 years ago
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

5 years ago
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)
(Assignee)

Comment 22

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

Comment 23

5 years ago
Created attachment 669869 [details] [diff] [review]
Part 1: Add chrome only openForPrincipal() and deleteForPrincipal()
Assignee: gkrizsanits → Jan.Varga
Status: REOPENED → ASSIGNED
Attachment #669869 - Flags: review?(bent.mozilla)
(Assignee)

Comment 24

5 years ago
Created attachment 669870 [details] [diff] [review]
Part 2: Code cleanup
Attachment #669870 - Flags: review?(jonas)
(Assignee)

Comment 25

5 years ago
Created attachment 669871 [details] [diff] [review]
Part 3: Automatic tests
Attachment #669871 - Flags: review?(jonas)
Attachment #669870 - Flags: review?(jonas) → review+
Attachment #669871 - Flags: review?(jonas) → review+
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.
Attachment #669869 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 27

5 years ago
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
(Assignee)

Comment 28

5 years ago
(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
https://hg.mozilla.org/mozilla-central/rev/a0ac59628202
https://hg.mozilla.org/mozilla-central/rev/90e2bd5252aa
https://hg.mozilla.org/mozilla-central/rev/be030e22b120
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Would really like these change uplifted to Aurora if possible.

Updated

5 years ago
Component: General → DOM: IndexedDB
Product: Add-on SDK → Core
Yeah, I'm merging it now.
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:
Attachment #669869 - Flags: approval-mozilla-aurora?
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:
Attachment #669870 - Flags: approval-mozilla-aurora?
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:
Attachment #669871 - Flags: approval-mozilla-aurora?
(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.
(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 ?
(Assignee)

Comment 37

5 years ago
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
(Assignee)

Comment 38

5 years ago
and if I remember correctly this is the last feature which blocks JetPack team from releasing a new version ?
It's blocking jetpack to release indexeddb support at least.
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
Attachment #669869 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #669870 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #669871 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 41

5 years ago
I'm going to test patches on aurora, will land if everything works ok
(Assignee)

Comment 42

5 years ago
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
status-firefox18: --- → fixed
(Assignee)

Updated

5 years ago
status-firefox19: --- → fixed

Comment 43

4 years ago
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
You need to log in before you can comment on or make changes to this bug.