587797 - IndexedDB: make it possible to access IndexedDB APIs from chrome

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

There has been some discussion in the Jetpack project of making IndexedDB available to addons that have more complex storage needs than can be satisfied by Jetpack's Simple Storage API, either as a core library or as a third-party library made available via Add-on Builder.

In order for Jetpack developers to do that, we would need to be able to access the IndexedDB APIs from chrome and use it to create and give addons access to addon-specific databases (where addons are identified by unique IDs rather than by domain names/URLs).

Filing this under Core - DOM: Core & HTML because that's where other IndexedDB bugs are filed, but feel free to move to a more appropriate product/component.

Comment 1

[Julián Ceballos]

(In reply to comment #0)
> There has been some discussion in the Jetpack project of making IndexedDB
> available to addons that have more complex storage needs than can be satisfied
> by Jetpack's Simple Storage API, either as a core library or as a third-party
> library made available via Add-on Builder.
> 
> In order for Jetpack developers to do that, we would need to be able to access
> the IndexedDB APIs from chrome and use it to create and give addons access to
> addon-specific databases (where addons are identified by unique IDs rather than
> by domain names/URLs).
> 
> Filing this under Core - DOM: Core & HTML because that's where other IndexedDB
> bugs are filed, but feel free to move to a more appropriate product/component.

I don't know if i understood well, IndexedDB would have access to sqlite databases?, so IndexedDB API would have an sqlite<->indexedDB converter?

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #1)
> I don't know if i understood well, IndexedDB would have access to sqlite
> databases?, so IndexedDB API would have an sqlite<->indexedDB converter?

No, the IndexedDB API is actually an API for accessing IndexedDB databases (which may happen to be implemented using SQLite).  You can't use the API to access SQLite databases.  For that, you need to use the Storage API <https://developer.mozilla.org/en/storage>.

Comment 3

[Julián Ceballos]

Well, i think that put IndexedDB like an official addons SDK library would be great, because:

1st: Is Standard way to make storage, suggested by w3c.
2nd: Add the library in the addon SDK would be a nice way to make diffussion of the IndexedDB standard.

That's what i think.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

(reposting my comment from bug 681024 here)

There's a similar problem to mozIndexedDB access throwing from chrome (bug 681024) in the IDBFactory::Open/CheckPermissionHelper.cpp GetIndexedDBPermissions pairing, although maybe what I'm doing is bad.

Specifically, I have JS code that is loaded by jetpack.  I try and steal a reference to mozIndexedDB from the hidden window.  This works on a build with the patch.

Unfortunately, when I issue the call to IDBFactory::Open, it uses:
  nsresult rv = nsContentUtils::GetSecurityManager()->
    GetSubjectPrincipal(getter_AddRefs(principal));

This nets it the system principal because it is using the context of the calling code, so:
  if (nsContentUtils::IsSystemPrincipal(principal)) {
    origin.AssignLiteral("chrome");
  }
ends up assigning us the URI of "chrome".

But in GetIndexedDBPermissions, the principal is looked up by doing:
  nsCOMPtr<nsIScriptObjectPrincipal> sop(do_QueryInterface(aWindow));
  NS_ENSURE_TRUE(sop, nsIPermissionManager::DENY_ACTION);

Unfortunately, the hidden window's principal is not the system principal, so this check does not pass:
  if (nsContentUtils::IsSystemPrincipal(sop->GetPrincipal())) {
    return nsIPermissionManager::ALLOW_ACTION;
  }

and instead we end up dying at:
  nsresult rv = NS_NewURI(getter_AddRefs(uri), aASCIIOrigin);
  NS_ENSURE_SUCCESS(rv, nsIPermissionManager::DENY_ACTION);


Would it be okay to make GetIndexedDBPermissions detect a URI of chrome or save off the principal of the caller so things could work?  I have tried some other hackery like invoking IDBFactory::Open in a loaded document and passing that back out to my jetpack module, but unfortunately I start getting JS same-compartment assertions when the callback triggers and my code tries to get at the 'result' on the db request.

I would ideally like to avoid loading my JS as a straight-up content document because everything else is using the jetpack module system, but I can do that if that's the only option.  Suggestions appreciated!

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

I altered CheckPermissionHelper to provide a pass if the URI is "chrome", and got the same compartment check failure.  Here's the terse backtrace for posterity:

  0 raise
  1 CrashInJS
  2 JS_Assert
  3 js::CompartmentChecker::fail
  4 js::CompartmentChecker::check
  5 js::CompartmentChecker::check
  6 js::CompartmentChecker::check
  7 js::assertSameCompartment<JSObject*, js::Value>
  8 js::CallJSPropertyOp
  9 js::Shape::get
 10 js_NativeGetInline
 11 js_GetPropertyHelperInline
 12 js_GetPropertyHelper
 13 js::Interpret
 14 js::RunScript
 15 js::InvokeKernel
 16 js::Invoke
 17 js::Invoke
 18 JS_CallFunctionValue
 19 nsXPCWrappedJSClass::CallMethod
 20 nsXPCWrappedJS::CallMethod
 21 PrepareAndDispatch
 22 SharedStub
 23 nsDOMEventListenerWrapper::HandleEvent
 24 nsEventListenerManager::HandleEventSubType
 25 nsEventListenerManager::HandleEventInternal
 26 nsEventListenerManager::HandleEvent
 27 nsEventTargetChainItem::HandleEvent
 28 nsEventTargetChainItem::HandleEventTargetChain
 29 nsEventDispatcher::Dispatch
 30 nsEventDispatcher::DispatchDOMEvent
 31 nsDOMEventTargetHelper::DispatchEvent
 32 dom::indexedDB::AsyncConnectionHelper::OnSuccess
 33 dom::indexedDB::AsyncConnectionHelper::Run
 34 nsThread::ProcessNextEvent

and this was on:
    var dbOpenRequest = IndexedDB.open("deuxdrop-" + nsprefix);
    dbOpenRequest.onsuccess = function(event) {
      self._db = dbOpenRequest.result;
                              ^^^^^^^^

Comment 6

[Myk Melez [:myk] [@mykmelez]]

cc:ing Jetpack's platform hackers and drivers.

Drivers: would it help to have an Add-on SDK product bug that depends on this one to track the priority/milestone of a fix for the Jetpack project?

Comment 7

[Gabor Krizsanits (INACTIVE)]

Is there any update on this one? 
Andrew: how far have you got with this / do you need any help with it?
Dave/Myk: what is the priority of this bug for jetpack?

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7)
> Andrew: how far have you got with this / do you need any help with it?

We worked around the problem by just running our 'daemon' code inside a hidden-frame using RequireJS instead of in chrome-space using jetpack.  Use of the hidden-frame allows us to poke our jsctypes-using binding into the namespace, whereas page-mod with proper electrolysis friendliness would not.

I am not pursuing any fixes to platform at this time, but I am confident khuey will fix everything for everyone! :)

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Andrew Sutherland (:asuth) from comment #8)
> I am not pursuing any fixes to platform at this time, but I am confident
> khuey will fix everything for everyone! :)

If this is important to Jetpack, we (the IndexedDB folks) need to know that, because at the moment we're not spending any effort on getting IndexedDB working in chrome ...

Comment 10

[Gabor Krizsanits (INACTIVE)]

(In reply to Andrew Sutherland (:asuth) from comment #8)
> We worked around the problem by just running our 'daemon' code inside a

Hmmm... wondering if we could use that solution... where can I find it?

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #9)
> If this is important to Jetpack, we (the IndexedDB folks) need to know that,
> because at the moment we're not spending any effort on getting IndexedDB
> working in chrome ...

It is still a wanted feature for jetpack yes. And I can work on it even, especially if I get some initial hints where to start and some help if I get stuck with it.

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #10)
> (In reply to Andrew Sutherland (:asuth) from comment #8)
> > We worked around the problem by just running our 'daemon' code inside a
> 
> Hmmm... wondering if we could use that solution... where can I find it?

The jetpack script that spawns the pages (and pre-authorizes IndexedDB use):
https://github.com/mozilla/deuxdrop/blob/develop/clients/addon/lib/main.js

The page that bootstraps and runs the client daemon:
https://github.com/mozilla/deuxdrop/blob/develop/clients/firefox/clientdaemon.html

We *do not* use an IndexedDB shim anymore; our code that uses it just pulls it out of the global ('window') namespace:
https://github.com/mozilla/deuxdrop/blob/develop/clients/deps/rdplat/gendb.js

There is, however, some moot shim code from our failed attempt to steal the IndexedDB reference into chrome-space and not die:
https://github.com/mozilla/deuxdrop/blob/develop/clients/addon/lib/indexdbshim.js

Comment 12

[Philipp von Weitershausen [:philikon]]

Allow me to weigh in as well. We're investigating IndexedDB for storing various kinds of data in Gecko for B2G (contacts and SMS come to mind for now, see e.g. bug 701468). So we would thus also need a way to create IndexedDBs from JS XPCOM and JSM code.

I already spoke to bent and sicking about this, and they seemed to think that this could be done akin to how an XMLHttpRequest instance can be acquired via Cc[...].createInstance(...).

Comment 13

[Dave Townsend [:mossop]]

While for Jetpack itself we think this is a P2 it looks like this is also wanted in enough other areas to make it well worth us starting on implementation on this.

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

bent and I are happy to advise/answer questions/etc.

Comment 15

[Gregor Wagner [:gwagner]]

Hey Gabor, are you currently working on this?

Comment 16

[Gabor Krizsanits (INACTIVE)]

Attachment: proposed fix

So I made the dom window and the related global and what no optional everywhere, added an extra init function for exception services and IDBKeyrange static functions. 
mrbkap: I wanted you to take a look at the changes in IDBRequest
khuey: if you want anyone else to review it let me know

Tests will follow in a bit.

Comment 17

[Gabor Krizsanits (INACTIVE)]

Attachment: tests

So I tried to keep the old tests and reuse the same code for the xpcshell tests. I'm open for any improvements, ideas.

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 579707 [details] [diff] [review]
proposed fix

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

Looks pretty good.  Some comments:

(Please attach an updated diff with 8 lines of context)

::: dom/indexedDB/AsyncConnectionHelper.cpp
@@ +148,5 @@
> +  if (mRequest->ScriptContext()) {
> +    global = mRequest->ScriptContext()->GetNativeGlobal();
> +  }
> +  else {
> +    global = mRequest->GetWrapper();

The wrapper is not the global ... either you need to JS_GetGlobalForObject or rename this variable.

::: dom/indexedDB/CheckPermissionsHelper.cpp
@@ +78,5 @@
>    }
>  
> +  // No window here means chrome access
> +  if (!aWindow)
> +    return nsIPermissionManager::ALLOW_ACTION;

if (condition) {
  expr;
}

even for single line statements.

There's a lot of these, I didn't call them all out.

::: dom/indexedDB/IDBDatabase.cpp
@@ +710,4 @@
>      nsresult rv = aVisitor.mDOMEvent->GetType(type);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
> +    if (type.EqualsLiteral(ERROR_EVT_STR) && mOwner) {

Instead of adding mOwner to the conditional here, check for it at the beginning of the function and return early if we don't have an owner.

::: dom/indexedDB/IDBEvents.cpp
@@ +82,5 @@
>                                              Bubbles aBubbles,
>                                              Cancelable aCancelable)
>  {
> +  // Let's make sure that XPConnect is still alive:
> +  NS_ENSURE_TRUE(nsContentUtils::XPConnect(), nsnull);

This should not be necessary.

::: dom/indexedDB/IDBFactory.cpp
@@ -109,5 @@
>  
>    nsRefPtr<IDBFactory> factory = new IDBFactory();
>  
>    factory->mWindow = do_GetWeakReference(aWindow);
> -  NS_ENSURE_TRUE(factory->mWindow, nsnull);

Please keep this, but change it to

NS_ENSURE_TRUE(factory->mWindow || !aWindow ...)

@@ -406,4 @@
>    }
>  
>    nsCOMPtr<nsPIDOMWindow> window = do_QueryReferent(mWindow);
> -  NS_ENSURE_TRUE(window, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

Same here, if mWindow is not null but we don't get a window here we need to error out.  Otherwise, if you request a database from a factory whose window has gone away we'll give them a window that has no quota checks!

::: dom/indexedDB/IDBRequest.cpp
@@ +56,4 @@
>  #include "IDBTransaction.h"
>  #include "nsContentUtils.h"
>  
> +#include "xpcprivate.h"

Instead of including xpcprivate.h you should use nsContentUtils::ThreadJSContextStack()

@@ +98,5 @@
>    if (!aScriptContext || !aOwner) {
>      NS_ERROR("Null context and owner!");
>      return nsnull;
>    }
> +*/

Don't leave commented out code in the tree.  Just remove it.

@@ +156,5 @@
> +    obj = mScriptContext->GetNativeGlobal();
> +    NS_ASSERTION(obj, "Failed to get global object!");
> +  } 
> +  else {
> +    if (NS_FAILED(nsXPConnect::GetXPConnect()->GetSafeJSContext(&cx)))

s/nsXPConnect::GetXPConnect()/nsContentUtils::ThreadJSContextStack()/

::: dom/indexedDB/IDBTransaction.cpp
@@ +182,4 @@
>    NS_ASSERTION(!mSavepointCount, "Should have released them all!");
>    NS_ASSERTION(!mConnection, "Should have called CommitOrRollback!");
>    NS_ASSERTION(!mCreating, "Should have been cleared already!");
> +  NS_ASSERTION(mFiredCompleteOrAbort || !nsContentUtils::XPConnect(), "Should have fired event!");

This should not be necessary ...

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ -559,5 @@
>    else {
> -#ifdef DEBUG
> -    NS_ASSERTION(PR_GetThreadPrivate(mCurrentWindowIndex),
> -               "Somebody forgot to clear the current window!");
> -#endif

Please replace this with a comment that we can't assert that there is a Window here because NSPR doesn't distinguish between the window being null and PR_GetThreadPrivate failing.

@@ +1255,5 @@
> +
> +NS_IMETHODIMP
> +IndexedDatabaseManager::CreateFactory(nsIIDBFactory** _retval)
> +{
> +	*_retval = IDBFactory::Create(NULL).get();

no tabs!

also, please use nsnull instead of NULL.

@@ +1277,5 @@
> +  JSObject* keyrangeObj = JS_NewObject(aCx, NULL, NULL, NULL);
> +  NS_ENSURE_TRUE(keyrangeObj, NS_ERROR_OUT_OF_MEMORY);
> +    
> +  if (!IDBKeyRange::DefineConstructors(aCx, keyrangeObj)) 
> +    return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;

Just return NS_ERROR_FAILURE here, this is exposed to XPCOM, not to content script.

@@ +1281,5 @@
> +    return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +
> +  jsval keyrangeVal = OBJECT_TO_JSVAL(keyrangeObj);
> +  if (!JS_SetProperty(aCx, global, "IDBKeyRange", &keyrangeVal))
> +    return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;

Ditto.

::: dom/indexedDB/nsIIndexedDatabaseManager.idl
@@ +100,5 @@
> +   * @param aGlobal
> +   *        The global object.
> +   */
> +  [implicit_jscontext]
> +  void init(in jsval aGlobal);

You need to change the IID of this interface since you've added methods to it.

Comment 19

[Gabor Krizsanits (INACTIVE)]

Attachment: cleaned up version

It's a cleaned up version. Since there was an issue with one of the tests (caused by another patch), I'll update the tests after they are fixed.

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 580415 [details] [diff] [review]
cleaned up version

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

This looks great.  A few comments, and we'll get Blake and Ben to give this a look too.

::: dom/indexedDB/AsyncConnectionHelper.cpp
@@ +156,3 @@
>  
>    nsresult rv =
> +    nsContentUtils::WrapNative(aCx, obj, aNative, aResult);

mrbkap needs to look at this to make sure that it's ok to pass a non-global for obj.

::: dom/indexedDB/IDBRequest.cpp
@@ +138,5 @@
>  
> +  JSContext* cx = nsnull;
> +  JSObject* obj = nsnull;
> +  if (mScriptContext) {
> +    // Otherwise we need to get the result from the helper.

Please move this comment back above the JSContext* cx = nsnull; line

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +1266,5 @@
> +NS_IMETHODIMP
> +IndexedDatabaseManager::Init(const jsval& aGlobal, JSContext* aCx)
> +{
> +  // init exception services
> +  nsCOMPtr<nsIDOMScriptObjectFactory> sof(do_GetService(kDOMSOF_CID));

Can we make the comment a bit more verbose here?  If I remember correctly we do this to ensure that the DOM error handler stuff is all set up in xpcshell, right?

@@ +1271,5 @@
> +
> +  // define IDBKeyrange static functions on the global
> +  if (JSVAL_IS_PRIMITIVE(aGlobal))
> +    return NS_ERROR_INVALID_ARG;
> +

Blake and Bent should look at the JSAPI goop in depth.  There are some more if statements that need braces here too.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 580415 [details] [diff] [review]
cleaned up version

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

::: dom/indexedDB/AsyncConnectionHelper.cpp
@@ +156,3 @@
>  
>    nsresult rv =
> +    nsContentUtils::WrapNative(aCx, obj, aNative, aResult);

Yeah, XPConnect deals fine with this.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +1276,5 @@
> +  JSObject* global = JSVAL_TO_OBJECT(aGlobal);
> +  if (!global || JS_GetParent(aCx, global))
> +    return NS_ERROR_INVALID_ARG;
> +
> +  JSObject* keyrangeObj = JS_NewObject(aCx, NULL, NULL, NULL);

I'd prefer to see us pass global for the parent here.

@@ +1283,5 @@
> +  if (!IDBKeyRange::DefineConstructors(aCx, keyrangeObj)) 
> +    return NS_ERROR_FAILURE;
> +
> +  jsval keyrangeVal = OBJECT_TO_JSVAL(keyrangeObj);
> +  if (!JS_SetProperty(aCx, global, "IDBKeyRange", &keyrangeVal))

Can any code run before this? If so, it might be better to use JS_DefineProperty so we're sure this does what we want.

Comment 22

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

Comment on attachment 580415 [details] [diff] [review]
cleaned up version

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

Most of this looks great, I just found some nits and a few small things to fix.

However, I'd like to change the interface you use to define these objects on non-window scopes (hence the r-). Those comments are all near the end.

Thanks for tackling this!

::: dom/indexedDB/AsyncConnectionHelper.cpp
@@ +143,5 @@
>    NS_ASSERTION(aNative, "Null pointer!");
>    NS_ASSERTION(aResult, "Null pointer!");
>    NS_ASSERTION(mRequest, "Null request!");
>  
> +  JSObject* obj = nsnull;

Nit: No need to initialize here, you're setting a value in every possible branch.

::: dom/indexedDB/IDBDatabase.h
@@ +115,5 @@
>  
>    already_AddRefed<nsIDocument> GetOwnerDocument()
>    {
> +    if (!mOwner) {
> +      return NULL;

Nit: nsnull please.

@@ +121,1 @@
>      nsCOMPtr<nsIDocument> doc = do_QueryInterface(mOwner->GetExtantDocument());

Nit: Please add an extra newline above this line.

::: dom/indexedDB/IDBEvents.cpp
@@ +51,5 @@
>  
>  #include "IDBRequest.h"
>  #include "IDBTransaction.h"
>  
> +#include "nsContentUtils.h"

Nit: Please stick this in the ns* block above.

::: dom/indexedDB/IDBFactory.cpp
@@ +105,1 @@
>      aWindow = aWindow->GetCurrentInnerWindow();

Hm, this is wrong. If aWindow is null at the beginning then we can assume this is a chrome call. However, if aWindow isn't null, and then GetCurrentInnerWindow returns null then we lose the ability to distinguish this case. If we call GetCurrentInnerWindow and it returns null then we need to let this method fail, not proceed as if it were chrome.

I suggest this:

  if (aWindow && aWindow->IsOuterWindow()) {
    aWindow = aWindow->GetCurrentInnerWindow();
    NS_ENSURE_TRUE(aWindow, nsnull);
  }

@@ +107,5 @@
>  
>    nsRefPtr<IDBFactory> factory = new IDBFactory();
>  
>    factory->mWindow = do_GetWeakReference(aWindow);
> +  NS_ENSURE_TRUE(factory->mWindow || !aWindow, nsnull);

Nit: Please just wrap this block in an |if (aWindow)| check rather than doing an unnecessary function call and making the return condition more complicated.

@@ +402,5 @@
> +  nsIScriptContext* context = nsnull;
> +  if (sgo) {
> +    context = sgo->GetContext();
> +    NS_ENSURE_TRUE(context, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +  }

Hm, this whole block is a bit confusing. How about you do this:

  nsCOMPtr<nsPIDOMWindow> window;
  nsCOMPtr<nsIScriptGlobalObject> sgo;
  nsIScriptContext* context = nsnull;

  if (mWindow) {
    window = do_QueryReferent(mWindow);
    NS_ENSURE_TRUE(window, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

    sgo = sgo = do_QueryInterface(window);
    NS_ENSURE_TRUE(sgo, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

    context = sgo->GetContext();
    NS_ENSURE_TRUE(context, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
  }

This way it's really clear that those things are fine to be null if mWindow is null.

::: dom/indexedDB/IDBRequest.cpp
@@ +56,5 @@
>  #include "IDBTransaction.h"
>  #include "nsContentUtils.h"
>  
> +#include "nsDOMJSUtils.h"
> +#include "nsIJSContextStack.h"

Nit: The nsIJSContextStack should go in the interface block, and the nsDOMJSUtils should go in the ns* block.

@@ +327,4 @@
>      void *gcThing = JSVAL_TO_GCTHING(tmp->mResultVal);
>      NS_IMPL_CYCLE_COLLECTION_TRACE_JS_CALLBACK(gcThing, "mResultVal")
>    }
>  NS_IMPL_CYCLE_COLLECTION_TRACE_END

Yikes. If you're making this inherit nsDOMEventTargetWrapperCache then you need to make the trace function inherit too. You'll need to use NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED here or else we won't trace the wrapper during CC.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +62,5 @@
>  #include "IDBFactory.h"
>  #include "LazyIdleThread.h"
>  #include "TransactionThreadPool.h"
> +#include "IDBKeyRange.h"
> +#include "nsIDOMScriptObjectFactory.h"

Nit: please move this interface to the block with the other interfaces.

@@ +562,5 @@
>    }
>    else {
> +    // we can't assert PR_GetThreadPrivate(mCurrentWindowIndex) here
> +    // because NSPR doesn't distinguish between the window being null 
> +    // and PR_GetThreadPrivate failing.

It's really unlikely that PR_GetThreadPrivate would fail, but even if it did we still would want some kind of warning. Please revert this block.

Did you see this fail?

@@ +729,5 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(),
>                 "We're about to touch a window off the main thread!");
>  
> +  if (!aWindow) {

Can you add an assertion here like this:

  NS_ASSERTION(nsContentUtils::IsCallerChrome(), "Null window but not chrome!");

@@ +1258,5 @@
> +
> +NS_IMETHODIMP
> +IndexedDatabaseManager::CreateFactory(nsIIDBFactory** _retval)
> +{
> +  *_retval = IDBFactory::Create(nsnull).get();

Even though this is technically correct I think it's much clearer if you write this way:

  nsCOMPtr<nsIIDBFactory> factory = IDBFactory::Create(nsnull);
  factory.forget(_retval);

Also, someday, we may make the thing you wrote (having an un-forgotten already_AddRefed<> go out of scope) illegal.

@@ +1273,5 @@
> +  if (JSVAL_IS_PRIMITIVE(aGlobal))
> +    return NS_ERROR_INVALID_ARG;
> +
> +  JSObject* global = JSVAL_TO_OBJECT(aGlobal);
> +  if (!global || JS_GetParent(aCx, global))

I don't think we should enforce that this is a global, see below.

::: dom/indexedDB/nsIIndexedDatabaseManager.idl
@@ +37,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
>  #include "nsISupports.idl"
> +#include "nsIIDBFactory.idl"

For any interfaces that aren't inherited you can simply forward declare:

  interface nsIIDBFactory;

However, I'm convinced that we should remove this, see below.

@@ +91,5 @@
> +
> +  /**
> +   * Creates an IDBFactory.
> +   */
> +  nsIIDBFactory createFactory();

I think we should remove this method entirely... IndexedDB is heavily dependent on JS so making a IDBFactory that could be used in C++ is basically worthless. Whenever you use indexeddb you'll need to be running JS. Let's instead combine this with the init method below.

@@ +100,5 @@
> +   * @param aGlobal
> +   *        The global object.
> +   */
> +  [implicit_jscontext]
> +  void init(in jsval aGlobal);

Let's make this method define both 'mozIndexedDB' and 'IDBKeyRange', just like we have on window scopes, on whatever object we pass in. That way some savvy callers can avoid polluting their global scopes and others can just use indexedDB exactly like they would in a window.

This will mean that you'll have to manually wrap an IDBFactory object and JS_DefineProperty that in addition to the IDBKeyRange object. It's basically just a call to nsContentUtils::WrapNative, feel free to ping me if you need help.

And how about we rename this method to something like "defineGlobalObjects"? Maybe "defineFactoryObjects"? Maybe sicking will have a better name for this bikeshed.

Comment 23

[Gabor Krizsanits (INACTIVE)]

Attachment: cleaned up version 2

Thanks for all 3 thorough reviews. 

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #20)
> Comment on attachment 580415 [details] [diff] [review]
> cleaned up version
> 
> Can we make the comment a bit more verbose here?  If I remember correctly we
> do this to ensure that the DOM error handler stuff is all set up in
> xpcshell, right?
> 

Let me know if it's still vague.


(In reply to Blake Kaplan (:mrbkap) from comment #21)
> Comment on attachment 580415 [details] [diff] [review]
> cleaned up version
 
Alright, I made all the changes you suggested.


(In reply to ben turner [:bent] from comment #22)
> Comment on attachment 580415 [details] [diff] [review]
> cleaned up version
> 
> Nit: No need to initialize here, you're setting a value in every possible
> branch.

I initialize pointers always, since later if the code changed it's hard to point out sometimes (for a reviewer), but if the convention here is different I can change on that habit in the future...

> 
> @@ +327,4 @@
> >      void *gcThing = JSVAL_TO_GCTHING(tmp->mResultVal);
> >      NS_IMPL_CYCLE_COLLECTION_TRACE_JS_CALLBACK(gcThing, "mResultVal")
> >    }
> >  NS_IMPL_CYCLE_COLLECTION_TRACE_END
> 
> Yikes. If you're making this inherit nsDOMEventTargetWrapperCache then you
> need to make the trace function inherit too. You'll need to use
> NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN_INHERITED here or else we won't trace
> the wrapper during CC.

I'm still not sure if I'm doing the right thing here, please double check this for me.

> @@ +562,5 @@
> >    }
> >    else {
> > +    // we can't assert PR_GetThreadPrivate(mCurrentWindowIndex) here
> > +    // because NSPR doesn't distinguish between the window being null 
> > +    // and PR_GetThreadPrivate failing.
> 
> It's really unlikely that PR_GetThreadPrivate would fail, but even if it did
> we still would want some kind of warning. Please revert this block.
> 
> Did you see this fail?

So SetCurrentWindowInternal is called in pairs. Once with aWindow to set thread private and once with nsnull to unset it. In our case aWindow is null, so we call twice with nsnull, and the assertion I removed cannot be true, since we never set the thread private in the first place. If we want to keep this assert because you think it's important enough, I will have to either change the function signature (extra bool that indicates if we want to set or unset) or try to eliminate every SetCurrentWindowInternal in case of not having a window. Which version do you prefer?

> 
> I think we should remove this method entirely... IndexedDB is heavily
> dependent on JS so making a IDBFactory that could be used in C++ is
> basically worthless. Whenever you use indexeddb you'll need to be running
> JS. Let's instead combine this with the init method below.
>

Done.

> 
> And how about we rename this method to something like "defineGlobalObjects"?
> Maybe "defineFactoryObjects"? Maybe sicking will have a better name for this
> bikeshed.

I stuck to defineGlobalObjects for now. Anyone is against this name and have a better idea, let me know.

Comment 24

[Gregor Wagner [:gwagner]]

I get errors for optimize builds with your new patch. 

Undefined symbols for architecture x86_64:
  "nsWrapperCache::GetWrapper() const", referenced from:
      mozilla::dom::indexedDB::HelperBase::WrapNative(JSContext*, nsISupports*, JS::Value*) in AsyncConnectionHelper.o
      mozilla::dom::indexedDB::IDBRequest::NotifyHelperCompleted(mozilla::dom::indexedDB::HelperBase*) in IDBRequest.o

It seems you are missing
#include "nsWrapperCacheInlines.h"

Comment 25

[Gabor Krizsanits (INACTIVE)]

Attachment: fixed linking problem on x64 optimized build

(In reply to Gregor Wagner [:gwagner] from comment #24)
> I get errors for optimize builds with your new patch. 
 
Weird... It builds for me in 32bit mode, but I don't get where it is getting that header from. I assume from some generated file that is different on 64bit mode, but just guessing. Anyway I fixed it and thanks for the notice. Also: I cannot run this on the try servers since my commit right is not granted yet. So if you still encounter this problem after my fix attempt, let me know.

Comment 26

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

Comment on attachment 583446 [details] [diff] [review]
fixed linking problem on x64 optimized build

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

::: dom/indexedDB/IDBRequest.cpp
@@ +151,5 @@
> +    if (NS_FAILED(cxStack->GetSafeJSContext(&cx))) {
> +      return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +    }
> +
> +    obj = GetWrapper();

This should be ENSURE'd too.

::: dom/indexedDB/IDBRequest.h
@@ +56,5 @@
>  
>  class HelperBase;
>  class IDBTransaction;
>  
> +class IDBRequest : public nsDOMEventTargetWrapperCache,

Ok, so I looked into this a little more, if we switch to nsDOMEventTargetWrapperCache we will also need to switch the flags in nsDOMClassInfo.cpp:

https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#1514

It will need to use nsEventTargetSH and EVENTTARGET_SCRIPTABLE_FLAGS.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +610,5 @@
>    }
>    else {
> +    // we can't assert PR_GetThreadPrivate(mCurrentWindowIndex) here
> +    // because NSPR doesn't distinguish between the window being null 
> +    // and PR_GetThreadPrivate failing.

Ah I see what is going on now. It's not really that PR_GetThreadPrivate can fail but that we set it with null if we don't have a window. Let's fix this comment.

@@ +1598,5 @@
>    mDelayedRunnables.Clear();
>  }
>  
> +NS_IMETHODIMP
> +IndexedDatabaseManager::DefineGlobalObjects(const jsval& aObj, JSContext* aCx)

Let's ensure that we're being called from chrome here:

  NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome(), NS_ERROR_NOT_AVAILABLE)

@@ +1601,5 @@
> +NS_IMETHODIMP
> +IndexedDatabaseManager::DefineGlobalObjects(const jsval& aObj, JSContext* aCx)
> +{
> +  // instantiating this class will register exception providers so even 
> +  // in xpcshell we will get typed (dom) exceptions, instead of general exceptions

Nit: Please capitalize 'instantiating' and add a period at the end of your comments.

@@ +1604,5 @@
> +  // instantiating this class will register exception providers so even 
> +  // in xpcshell we will get typed (dom) exceptions, instead of general exceptions
> +  nsCOMPtr<nsIDOMScriptObjectFactory> sof(do_GetService(kDOMSOF_CID));
> +
> +  // define IDBKeyrange static functions on the global

Nit: Here too.

@@ +1610,5 @@
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  JSObject* obj = JSVAL_TO_OBJECT(aObj);
> +  if (!obj || JS_GetParent(aCx, obj)) {

There's no need to check !obj here, JSVAL_IS_PRIMITIVE will filter out null as well. Please remove that.

I think what you really want to do is call JS_GetGlobalForObject(obj) and see if it equals obj. The parent thing is something that the JS team is trying to get rid of I believe.

@@ +1614,5 @@
> +  if (!obj || JS_GetParent(aCx, obj)) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  JSObject* mozIndexedDBObj = JS_NewObject(aCx, nsnull, nsnull, obj);

Looks like this is not needed (it's never used).

@@ +1618,5 @@
> +  JSObject* mozIndexedDBObj = JS_NewObject(aCx, nsnull, nsnull, obj);
> +  NS_ENSURE_TRUE(mozIndexedDBObj, NS_ERROR_OUT_OF_MEMORY);
> +
> +  jsval mozIndexedDBVal;
> +  nsCOMPtr<nsIIDBFactory> factory = IDBFactory::Create(nsnull);

Please assert that factory is non-null here.

@@ +1627,5 @@
> +      nsnull, nsnull, JSPROP_READONLY)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  JSObject* keyrangeObj = JS_NewObject(aCx, nsnull, nsnull, obj);

So I'm just now realizing... This is ok for IDBKeyRange, but you also need the interface constants, things like IDBCursor.NEXTNO_DUPLICATE and IDBTransaction.READ_WRITE. All of those things are magically created via the code here:

https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.cpp#6269

I think we need to expose some function on nsDOMClassInfo to define an interface object and its constants. It's not worth doing by hand here.

::: dom/indexedDB/nsIIndexedDatabaseManager.idl
@@ +96,5 @@
> +   * @param aObject
> +   *        The object, mozIndexedDB and IDBKeyrange should be defined on.
> +   */
> +  [implicit_jscontext]
> +  void defineGlobalObjects(in jsval aGlobal);

How about we call this "prepareGlobalForIndexedDB"?

Comment 27

[Gabor Krizsanits (INACTIVE)]

(In reply to ben turner [:bent] from comment #26)
> > +
> > +  JSObject* obj = JSVAL_TO_OBJECT(aObj);
> > +  if (!obj || JS_GetParent(aCx, obj)) {
> 
> There's no need to check !obj here, JSVAL_IS_PRIMITIVE will filter out null
> as well. Please remove that.
> 
> I think what you really want to do is call JS_GetGlobalForObject(obj) and
> see if it equals obj. The parent thing is something that the JS team is
> trying to get rid of I believe.
> 

Actually, since we wanted to allow none global object to be used here, I removed the whole thing.

> 
> So I'm just now realizing... This is ok for IDBKeyRange, but you also need
> the interface constants, things like IDBCursor.NEXTNO_DUPLICATE and
> IDBTransaction.READ_WRITE. All of those things are magically created via the
> code here:
> 
> https://mxr.mozilla.org/mozilla-central/source/dom/base/nsDOMClassInfo.
> cpp#6269
> 
> I think we need to expose some function on nsDOMClassInfo to define an
> interface object and its constants. It's not worth doing by hand here.
>

Well, since you have chrome access anyway, my solution for this problem was simply accessing the interfaces via the Componets.interfaces api. For enums, those interfaces work just fine but for those static like functions on IDBKeyRange I needed a workaround. So I would prefer a js side solution for initializing those globals, or let the users handle the problem them self.
 
> 
> How about we call this "prepareGlobalForIndexedDB"?

It is very descriptive, but since it is already on the IndexedDatabaseManager object the ForIndexedDB part sounds a bit redundant for me, plus the method works on a non global object as well. I would focus on the windowless part, to make it clear for users 'when' they should use this method like windowlessInit. But I'm also fine with prepareGlobal or prepareGlobalForIndexedDB if you like it better.

Comment 28

[Gabor Krizsanits (INACTIVE)]

Attachment: fixed and updated

Bent answered my questions on irc. We'll stick to the InitWindowless method name, and the globals for the interfaces will be probably implemented as a follow up in jsm.

Comment 29

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

Comment on attachment 584590 [details] [diff] [review]
fixed and updated

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

Just a few more things to fix here! Otherwise looks great.

::: dom/indexedDB/IDBRequest.cpp
@@ +150,5 @@
> +
> +    NS_ENSURE_SUCCESS(cxStack->GetSafeJSContext(&cx),
> +                      NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +    obj = GetWrapper();

Previous comment not addressed here. Please ENSURE obj here.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +1602,5 @@
> +NS_IMETHODIMP
> +IndexedDatabaseManager::InitWindowless(const jsval& aObj, JSContext* aCx)
> +{
> +  NS_ENSURE_TRUE(nsContentUtils::IsCallerChrome(), NS_ERROR_NOT_AVAILABLE);
> +  // Instantiating this class will register exception providers so even 

Nit: Please put a newline before the comment.

@@ +1607,5 @@
> +  // in xpcshell we will get typed (dom) exceptions, instead of general exceptions.
> +  nsCOMPtr<nsIDOMScriptObjectFactory> sof(do_GetService(kDOMSOF_CID));
> +
> +  // Defining IDBKeyrange static functions on the global.
> +  if (aObj.isObject()) {

This is using private methods for the js engine (I'm surprised that it works!). Please stick with the public API (anything in jsapi.h) and use JSVAL_IS_PRIMITIVE.

@@ +1611,5 @@
> +  if (aObj.isObject()) {
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  JSObject* obj = &aObj.toObject();

And JSVAL_TO_OBJECT here.

@@ +1612,5 @@
> +    return NS_ERROR_INVALID_ARG;
> +  }
> +
> +  JSObject* obj = &aObj.toObject();
> +  jsval mozIndexedDBVal;

Nit: Please move this three lines down, right before it's used in WrapNative.

@@ +1620,5 @@
> +  nsresult rv = nsContentUtils::WrapNative(aCx, obj, factory, &mozIndexedDBVal);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!JS_DefineProperty(aCx, obj, "mozIndexedDB", mozIndexedDBVal, 
> +      nsnull, nsnull, JSPROP_READONLY)) {

Sorry I didn't spot this earlier, please use only JSPROP_ENUMERATE and lose the READONLY. I don't see any point in restricting this.

@@ +1624,5 @@
> +      nsnull, nsnull, JSPROP_READONLY)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  JSObject* keyrangeObj = JS_NewObject(aCx, nsnull, nsnull, obj);

If we're not going to enforce that the object is a global then this last argument is invalid. Just pass nsnull again.

@@ +1632,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  jsval keyrangeVal = OBJECT_TO_JSVAL(keyrangeObj);
> +  if (!JS_DefineProperty(aCx, obj, "IDBKeyRange", keyrangeVal, 

Nit: Just pass OBJECT_TO_JSVAL(keyrangeObj) for the fourth argument and lose the stack var.

::: dom/indexedDB/nsIIndexedDatabaseManager.idl
@@ +96,5 @@
> +   * @param aObject
> +   *        The object, mozIndexedDB and IDBKeyrange should be defined on.
> +   */
> +  [implicit_jscontext]
> +  void initWindowless(in jsval aGlobal);

Nit: Make this aObject to match comments.

Comment 30

[Gabor Krizsanits (INACTIVE)]

(In reply to ben turner [:bent] from comment #29)
> Previous comment not addressed here. Please ENSURE obj here.
>
Sorry, for this, I remember this one, I thought I fixed it.
 
> > +  if (aObj.isObject()) {
> 
> This is using private methods for the js engine (I'm surprised that it
> works!). Please stick with the public API (anything in jsapi.h) and use
> JSVAL_IS_PRIMITIVE.

Ms2ger mentioned this api but he didn't know in which context I'm working in and I had no clue that it is private just it looked cleaner... Anyway I changed it back.

Comment 31

[Gabor Krizsanits (INACTIVE)]

Attachment: fixed and updated

Comment 32

[Philipp von Weitershausen [:philikon]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #30)
> > > +  if (aObj.isObject()) {
> > 
> > This is using private methods for the js engine (I'm surprised that it
> > works!). Please stick with the public API (anything in jsapi.h) and use
> > JSVAL_IS_PRIMITIVE.
> 
> Ms2ger mentioned this api but he didn't know in which context I'm working in
> and I had no clue that it is private just it looked cleaner... Anyway I
> changed it back.

I came across this the other day myself. According to mrbkap, mounir, and others, the jsval.foobar() API is indeed not only public now but also the preferred way over JSVAL_FOOBAR (because, as you say, it's cleaner). Please don't take my word for it, but we probably want to make sure that new code uses the preferred style.

Comment 33

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

Comment on attachment 584618 [details] [diff] [review]
fixed and updated

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

Looks great, thanks!

Comment 34

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

(In reply to Philipp von Weitershausen [:philikon] from comment #32)

Huh, neat! I'd rather not mess with these here, though. Maybe we should just switch all the jsapi stuff at some point.

Comment 35

[Gabor Krizsanits (INACTIVE)]

https://tbpl.mozilla.org/?tree=Try&rev=2f802e55df03

Something happened on the try servers today so I think a rerun would be nice. I haven't got my level 1 commit right yet so I cannot do that but since they look good so far it is very likely that it will pass. So could someone push this patch for me?

Comment 36

[Gabor Krizsanits (INACTIVE)]

Just a note: the tests are not ready yet to be pushed, I'm working on it and will update them soon as well. So only the attachment 584618 [details] [diff] [review] is ready to be pushed.

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #35)
> https://tbpl.mozilla.org/?tree=Try&rev=2f802e55df03
> 
> So could
> someone push this patch for me?

I'm out of town until tonight. If no one pushes it for you by then, I could do it. Leave me a ping on IRC later today if you still need it pushed?

Comment 38

[Dave Townsend [:mossop]]

Pushed: https://tbpl.mozilla.org/?tree=Try&rev=1c929fcd7b78

Comment 39

[Dave Townsend [:mossop]]

(In reply to Dave Townsend (:Mossop) from comment #38)
> Pushed: https://tbpl.mozilla.org/?tree=Try&rev=1c929fcd7b78

Actually I've cancelled this run, tinderbox is not working right now so it wouldn't show any results. We'll have to push it again later once bug 713255 is fixed

Comment 40

[Mozilla RelEng Bot]

Try run for 1c929fcd7b78 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=1c929fcd7b78
Results (out of 14 total builds):
    exception: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-1c929fcd7b78

Comment 41

[Mozilla RelEng Bot]

Try run for d5ab56ba2b18 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d5ab56ba2b18
Results (out of 272 total builds):
    exception: 24
    success: 224
    warnings: 22
    failure: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d5ab56ba2b18

Comment 42

[Gabor Krizsanits (INACTIVE)]

Attachment: test updated 1st batch

Here are some updated tests (51 files...). Since tests are changing I would prefer to push the patch and these tests that are already working as soon as possible, so I don't have to convert the test files all over again and again. There are some tests that I could not convert yet into xpcshell test, but I'm afraid when I get them done some tests will be changed again and I can start it all over again etc... Also could someone help me out with the tinderbox results? Are any of these failures real or just random oranges / build server issues?

Comment 43

[Jan Varga [:janv]]

Are you creating a new test directory ?

See https://bugzilla.mozilla.org/show_bug.cgi?id=661877#c19

Comment 44

[Gabor Krizsanits (INACTIVE)]

(In reply to Jan Varga [:janv] from comment #43)
> Are you creating a new test directory ?
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=661877#c19

I'm not sure that should be a problem for me since I only want to put xpcshell test files into that directory. The mochi test files are staying in the same directory (except they will load the same script files from that new 'unit' directory, this way sharing the same code with the xpcshell tests). If it will be a problem I think I will need to find a fix for it since the xpcshell based unit tests should be in a unit subdir as far as I know, but thanks for the headsup!

Comment 45

[Gabor Krizsanits (INACTIVE)]

Attachment: added missing files

Some files were missing from the test patch, I've added them.

Comment 46

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31d2fae950ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/4dd520a2dab0

Comment 47

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Backed out due to a test failure

https://tbpl.mozilla.org/php/getParsedLog.php?id=8291593&tree=Mozilla-Inbound&full=1#error0

Comment 48

[Gabor Krizsanits (INACTIVE)]

Attachment: removed one failing test file

Removed the failing test that caused the previous back out. The original version of test is still there (mochi test version). It might hit the same assertion on some platforms occasionally, just since asserts are not crashing mochi tests, this might have never been noticed so far. (More investigation will follow)

Comment 49

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Landed and stuck

https://hg.mozilla.org/mozilla-central/rev/29f2858488df
https://hg.mozilla.org/mozilla-central/rev/ea4d463ec99f

Comment 50

[Nadim Kobeissi]

As of Firefox 19 IndexedDB in fact does not work within chrome:// paths..

window.indexedDB, window.mozIndexedDB, indexedDB and mozIndexedDB all give the following error:

[00:15:15.599] [Exception... "The operation failed for reasons unrelated to the database itself and not covered by any other error code."  code: "0" nsresult: "0x80660001 (UnknownError)"  location: "Web Console Line: 1"]

Comment 51

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Nadim Kobeissi from comment #50)
> As of Firefox 19 IndexedDB in fact does not work within chrome:// paths..
> 
> window.indexedDB, window.mozIndexedDB, indexedDB and mozIndexedDB all give
> the following error:
> 
> [00:15:15.599] [Exception... "The operation failed for reasons unrelated to
> the database itself and not covered by any other error code."  code: "0"
> nsresult: "0x80660001 (UnknownError)"  location: "Web Console Line: 1"]

You want
nsIIndexedDatabaseManager.initWindowless

See:
https://mxr.mozilla.org/mozilla-central/source/dom/indexedDB/nsIIndexedDatabaseManager.idl

Comment 52

[Jan Varga [:janv]]

hm, I think the initWindowless() is for JSM components and xpcshell
I'll investigate what's wrong with chrome windows

Comment 53

[Nadim Kobeissi]

Is it possible to use initWindowless() to gain access to IndexedDB components inside a chrome window?

Comment 54

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Nadim Kobeissi from comment #50)
> As of Firefox 19 IndexedDB in fact does not work within chrome:// paths..
> 
> window.indexedDB, window.mozIndexedDB, indexedDB and mozIndexedDB all give
> the following error:
> 
> [00:15:15.599] [Exception... "The operation failed for reasons unrelated to
> the database itself and not covered by any other error code."  code: "0"
> nsresult: "0x80660001 (UnknownError)"  location: "Web Console Line: 1"]

I can't reproduce this.  http://img818.imageshack.us/img818/9065/idbwfm.png

Comment 55

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #54)
> (In reply to Nadim Kobeissi from comment #50)
> > As of Firefox 19 IndexedDB in fact does not work within chrome:// paths..
> > 
> > window.indexedDB, window.mozIndexedDB, indexedDB and mozIndexedDB all give
> > the following error:
> > 
> > [00:15:15.599] [Exception... "The operation failed for reasons unrelated to
> > the database itself and not covered by any other error code."  code: "0"
> > nsresult: "0x80660001 (UnknownError)"  location: "Web Console Line: 1"]
> 
> I can't reproduce this.  http://img818.imageshack.us/img818/9065/idbwfm.png

So after talking through it on IRC we figure out the problem.

Nadim is loading a chrome URI in a content docshell.  Because its a content docshell the window is an nsGlobalWindow, not an nsGlobalChromeWindow, and IsChromeWindow() returns false and bug 681024 happens because we skip the checks if(IsChromeWindow()).

We figured out a workaround using initWindowless, and since loading chrome URIs in content docshells isn't actually supported I don't think there's anything else to do.