832883 - Move IDBKeyRange to WebIDL and define indexedDB/IDBKeyRange in all the spots

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: WIP

The main issue is IndexedDatabaseManager::InitWindowless, I think. I guess we'll need a GetConstructorObject that doesn't try to cache the result in the protoAndIfaceArray.

Comment 2

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

Is this still relevant enough to review?  I suspect it may have bitrotted substantially in the last 4 months.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: WIP v2

Comment 4

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

Comment on attachment 755282 [details] [diff] [review]
WIP v2

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

Why does this need to inherit from nsWrapperCache?

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

Comment 6

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

Comment on attachment 782286 [details] [diff] [review]
Patch v1

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

::: dom/indexedDB/IDBKeyRange.h
@@ +28,5 @@
>  class KeyRange;
>  } // namespace FIXME_Bug_521898_objectstore
>  } // namespace ipc
>  
> +class IDBKeyRange MOZ_FINAL : public nsISupports

I don't think this needs nsISupports anymore.  Please file a followup on possibly removing it.

@@ +104,5 @@
>    {
>      return mIsOnly ? mLower : mUpper;
>    }
>  
> +  // TODO: Remove these in favour of LowerOpen() / UpperOpen().

File a followup please.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +528,5 @@
>  
>    JS::Rooted<JSObject*> obj(aCx, JSVAL_TO_OBJECT(aObj));
> +  if (!(js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL)) {
> +    return NS_ERROR_FAILURE;
> +  }

Add in an NS_WARNING about using InitWindowless in a DOM global

::: dom/indexedDB/nsIIDBObjectStore.idl
@@ +90,5 @@
>  
>    void
>    deleteIndex([Null(Stringify)] in DOMString name);
>  
> +  // Accepts null, a key value, or a IDBKeyRange object.

an IDBKeyRange

::: dom/webidl/IDBKeyRange.webidl
@@ +24,5 @@
> +  static IDBKeyRange lowerBound (any lower, optional boolean open = false);
> +  [Creator, Throws]
> +  static IDBKeyRange upperBound (any upper, optional boolean open = false);
> +  [Creator, Throws]
> +  static IDBKeyRange bound (any lower, any upper, optional boolean lowerOpen = false, optional boolean upperOpen = false);

Can you file a spec bug on getting the default values in the IDL in the spec?

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

All done: bug 900577, bug 900578 and https://www.w3.org/Bugs/Public/show_bug.cgi?id=22856

Comment 8

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

Can we land this?

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

No, it breaks all B2G tests, like:

https://tbpl.mozilla.org/php/getParsedLog.php?id=26064836&tree=Mozilla-Central

with things like

02:28:43  WARNING -  E/GeckoConsole(   45): [JavaScript Error: "[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIIndexedDatabaseManager.initWindowless]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/PushService.jsm :: PushDB :: line 48"  data: no]" {file: "resource://gre/modules/PushService.jsm" line: 48}]
02:28:43  WARNING -  E/GeckoConsole(   45): [JavaScript Error: "[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIIndexedDatabaseManager.initWindowless]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: jar:file:///system/b2g/omni.ja!/components/SettingsManager.js :: SettingsManager :: line 256"  data: no]" {file: "jar:file:///system/b2g/omni.ja!/components/SettingsManager.js" line: 256}]
02:28:43  WARNING -  E/GeckoConsole(   45): [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIIndexedDatabaseManager.initWindowless]" {file: "jar:file:///system/b2g/omni.ja!/components/SettingsManager.js" line: 256}]
02:28:43  WARNING -  E/GeckoConsole(   45): [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIIndexedDatabaseManager.initWindowless]" {file: "resource://gre/modules/ContactService.jsm" line: 42}]
02:28:43  WARNING -  E/GeckoConsole(   45): [JavaScript Error: "[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIIndexedDatabaseManager.initWindowless]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: resource://gre/modules/ActivitiesService.jsm :: actdb_init :: line 42"  data: no]" {file: "resource://gre/modules/ActivitiesService.jsm" line: 42}]

Comment 10

[Andrea Marchesini [:baku]]

+  if (!(js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL)) {
+    return NS_ERROR_FAILURE;
+  }

It seems this is the problem of B2G tests failing. Is it strictly related to IDBKeyRange?

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

The GetConstructorObject call also has that check; I added it here for explicitness and to make sure that we'd throw immediately rather than halfway through the function.

I think I saw Jan pushed something to try, though.

Comment 12

[Jan Varga [:janv]]

Yeah, I'd like to get rid of initWindowless() altogether.
I'm officially on vacation, but I couldn't sleep last night, so I tried something :)
Expect a patch from me early next week or sooner.

Comment 13

[Jan Varga [:janv]]

Ms2ger, I spoke with bent, here's the plan:
1. make idb work everywhere without initWindowless()
2. remove initWindowless()

The latter will be probably done in a separate bug.
I have a fix that does 1, I just need to do some cleanup and more testing.

So, initWindowless() will be kept for now, but with the global object requirement that you added.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Sounds good to me.

Comment 15

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

Oh, and:

1a. Make sure we have tests for all the different spots (bootstrap.js, "ipcshell", components, JSMs, jetpack sandbox, anything else?)

Comment 16

[Jan Varga [:janv]]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #15)
> Oh, and:
> 
> 1a. Make sure we have tests for all the different spots (bootstrap.js,
> "ipcshell", components, JSMs, jetpack sandbox, anything else?)

I've got the tests for content, chrome windows, xpcshell, JS modules, JS components, JS sandboxes
I'm going to take a look at the ipcshell and bootstrap.js

Comment 17

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

See dom/workers/test/extensions/bootstrap/ for an example of a test extension that lets you try things in bootstrap.js.

Comment 18

[Jan Varga [:janv]]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #17)
> See dom/workers/test/extensions/bootstrap/ for an example of a test
> extension that lets you try things in bootstrap.js.

yeah, and dom/workers/test/test_extensionsBootstrap.xul
that's the last one, I added ipcshell in the meantime

I'm renaming this bug, since it would be too much work to split the patch.

Comment 19

[Jan Varga [:janv]]

Attachment: rebased patch v1

Comment 20

[Jan Varga [:janv]]

Attachment: all in one patch

Comment 21

[Jan Varga [:janv]]

Attachment: interdiff

Comment 22

[Jan Varga [:janv]]

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

Comment 23

[Jan Varga [:janv]]

Attachment: all in one patch v2

Comment 24

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

Comment on attachment 811222 [details] [diff] [review]
all in one patch v2

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

r=me!

::: dom/indexedDB/IDBKeyRange.cpp
@@ +65,2 @@
>  
> +  JS::RootedObject obj(aCx, aVal.isObject() ? &aVal.toObject() : NULL);

Nit: nullptr

@@ +158,5 @@
>    DropJSObjects();
>  }
>  
> +JSObject*
> +IDBKeyRange::WrapObject(JSContext* aCx, JS::Handle<JSObject*> aScope)

Nit: JS::HandleObject

@@ +210,5 @@
>  
> +// static
> +already_AddRefed<IDBKeyRange>
> +IDBKeyRange::Only(const GlobalObject& aGlobal, JSContext* aCx,
> +                  JS::Handle<JS::Value> aValue, ErrorResult& aRv)

Nit: JS::HandleValue, here and below.

::: dom/indexedDB/IDBKeyRange.h
@@ +62,5 @@
>    {
>      return mIsOnly ? mLower : mUpper;
>    }
>  
> +  // TODO: Remove these in favour of LowerOpen() / UpperOpen().

This either should be done here or a followup filed/referenced here.

::: dom/indexedDB/IndexedDatabaseManager.cpp
@@ +133,5 @@
>    BINDING_ENTRY(IDBCursor)
>    BINDING_ENTRY(IDBRequest)
>    BINDING_ENTRY(IDBOpenDBRequest)
>    BINDING_ENTRY(IDBVersionChangeEvent)
> +  BINDING_ENTRY(IDBFileHandle)

Hm, I suck. Can you alphabetize this?

@@ +207,5 @@
>  };
>  
> +bool
> +GetIndexedDB(JSContext* aCx, JS::Handle<JSObject*> aGlobal,
> +             JS::MutableHandle<JS::Value> aResult)

Nit: JS::HandleValue and JS::MutableHandleValue, here and everywhere.

@@ +210,5 @@
> +GetIndexedDB(JSContext* aCx, JS::Handle<JSObject*> aGlobal,
> +             JS::MutableHandle<JS::Value> aResult)
> +{
> +  NS_ASSERTION(nsContentUtils::IsCallerChrome(), "Only for chrome!");
> +  NS_ASSERTION(js::GetObjectClass(aGlobal)->flags & JSCLASS_DOM_GLOBAL,

Nit: MOZ_ASSERT, here and everywhere you're adding new code.

@@ +221,5 @@
> +  }
> +
> +  NS_ASSERTION(factory, "This should never fail for chrome!");
> +
> +  if (!WrapNewBindingObject(aCx, aGlobal, factory, aResult)) {

Just return !!Wrap...

@@ +230,5 @@
> +}
> +
> +bool
> +IndexedDBLazyGetter(JSContext* aCx, JS::Handle<JSObject*> aGlobal,
> +                    JS::Handle<jsid> aId, JS::MutableHandle<JS::Value> aVp)

Nit: JS::HandleId

@@ +239,5 @@
> +  NS_ASSERTION(JSID_IS_STRING(aId), "Bad id!");
> +  NS_ASSERTION(JS_FlatStringEqualsAscii(JSID_TO_FLAT_STRING(aId), IDB_STR),
> +               "Bad id!");
> +
> +  JS::Rooted<JS::Value> indexedDB(aCx);

Nit: JS::RootedValue

@@ +244,5 @@
> +  if (!GetIndexedDB(aCx, aGlobal, &indexedDB)) {
> +    return false;
> +  }
> +
> +  if (!JS_DeletePropertyById(aCx, aGlobal, aId) ||

I'm not sure that you need to do this. JS_Define should be enough.

@@ +250,5 @@
> +                             JSPROP_ENUMERATE)) {
> +    return false;
> +  }
> +
> +  return JS_GetPropertyById(aCx, aGlobal, aId, aVp);

You should just do aVp.set(indexedDB) here, no need to go through the engine again.

@@ +482,5 @@
>  
>  // static
>  bool
> +IndexedDatabaseManager::DefineConstructors(JSContext* aCx,
> +                                           JS::Handle<JSObject*> aGlobal)

All these functions need main thread assertions.

@@ +524,5 @@
> +  NS_ASSERTION(nsContentUtils::IsCallerChrome(), "Only for chrome!");
> +  NS_ASSERTION(js::GetObjectClass(aGlobal)->flags & JSCLASS_DOM_GLOBAL,
> +               "Passed object is not a global object!");
> +
> +  JSString* indexedDBStr = JS_InternString(aCx, IDB_STR);

There's no real reason to intern anything here since you're only using the string once. Just use JS_DefineProperty directly.

@@ +531,5 @@
> +  }
> +
> +  jsid indexedDBId = INTERNED_STRING_TO_JSID(aCx, indexedDBStr);
> +
> +  if (!JS_DefinePropertyById(aCx, aGlobal, indexedDBId, JSVAL_VOID,

Nit: Just return the result directly instead if |if (!) { return false; } return true;|

@@ +752,3 @@
>  
>    bool hasIndexedDB;
> +  if (!JS_HasProperty(aCx, global, "indexedDB", &hasIndexedDB)) {

Nit: Use your IDB_STR here

::: dom/indexedDB/test/chromeHelpers.js
@@ +35,5 @@
> +}
> +
> +function errorHandler(event)
> +{
> +  throw new Error("indexedDB error, code " + event.target.errorCode);

This isn't right, we don't have errorCode any more. .error.name?

::: js/xpconnect/src/Sandbox.cpp
@@ +910,5 @@
>  
>  bool
>  xpc::SandboxOptions::GlobalProperties::Define(JSContext* cx, JS::HandleObject obj)
>  {
> +    if (indexedDB && AccessCheck::isChrome(obj) &&

Why do we need the isChrome check here? I think this can just be asserted?

::: js/xpconnect/src/nsXPConnect.cpp
@@ +537,5 @@
>          return UnexpectedFailure(NS_ERROR_FAILURE);
>      }
>  
> +    if (nsContentUtils::IsSystemPrincipal(aPrincipal) &&
> +        !IndexedDatabaseManager::DefineIndexedDBLazyGetter(aJSContext,

Make sure bholley signs off on this!

Comment 25

[Jan Varga [:janv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/81f8a3c52c23

Comment 26

[Jan Varga [:janv]]

Gabor, could you please upstream the addon-sdk changes, once it appears on m-c ?

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/81f8a3c52c23

Comment 28

[Kelsey Gilbert [:jgilbert]]

This causes consistent fatal assertions in DEBUG builds when using google search, on at least the two devices I've tested.

Comment 29

[Kelsey Gilbert [:jgilbert]]

(In reply to Jeff Gilbert [:jgilbert] from comment #28)
> This causes consistent fatal assertions in DEBUG builds when using google
> search, on at least the two devices I've tested.

This is bug 922837.

Comment 30

[Kelsey Gilbert [:jgilbert]]

This appears to be causing fatal assertions in DEBUG builds on most websites.

Comment 31

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/7e07545c3e6db32ad91fa366a5279526281658fb
Bug 832883 - Move IDBKeyRange to WebIDL and define indexedDB/IDBKeyRange in all the spots. r=khuey,bent (initial work done by Ms2ger)

Comment 32

[[github robot]]

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

https://github.com/mozilla/addon-sdk/commit/24471bc8b86272b36cd834980527d1ea3ba6250e
Fix a bad merge from Bug 832883.

Comment 33

[Mindaugas <LA-MJ>]

OK, so AFAIK, you must use initWindowless to be able to acess IDB from chrome until FF26 (aurora).

But calling it in FF27 (nightly) breaks our add-on with NS_ERROR_FAILURE even though that function is still defined.

So how do you figure we should test for this?

Comment 34

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

(In reply to 4mr.minj from comment #33)
> But calling it in FF27 (nightly) breaks our add-on with NS_ERROR_FAILURE
> even though that function is still defined.

Uh oh, sounds like we broke something? Can you file a new bug please?

Comment 35

[Mindaugas <LA-MJ>]

Sure... Bug 932345

Comment 36

[Jan Varga [:janv]]

(In reply to 4mr.minj from comment #33)
> OK, so AFAIK, you must use initWindowless to be able to acess IDB from
> chrome until FF26 (aurora).
> 
> But calling it in FF27 (nightly) breaks our add-on with NS_ERROR_FAILURE
> even though that function is still defined.
> 
> So how do you figure we should test for this?

Hm, take a look at this JS code:
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/indexed-db.js#30

and let me know if that doesn't work for you

Btw, this is going to change again, bug 921478.
One will have to call |Components.utils.importGlobalProperties(["indexedDB"])| to get IndexedDB e.g. in chrome.

Comment 37

[Mindaugas <LA-MJ>]

Yes, that works in both FF25 and FF28 (nightly just updated...)

Thanks.

I'll repost your answer in my bug.

Comment 38

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

This `initWindowless()` thing is still causing me some trouble from Jetpack code for `deleteRange`.  What is the approved pattern now from addons?

I did have
```
// handle misnaming bug in idb code;
let moreindexeddb = {};
Cc["@mozilla.org/dom/indexeddb/manager;1"]
  .getService(Ci.nsIIndexedDatabaseManager)
  .initWindowless(moreindexeddb);


```

Comment 39

[Jan Varga [:janv]]

(In reply to Gregg Lind (User Research - Test Pilot) from comment #38)
> This `initWindowless()` thing is still causing me some trouble from Jetpack
> code for `deleteRange`.  What is the approved pattern now from addons?
> 
> I did have
> ```
> // handle misnaming bug in idb code;
> let moreindexeddb = {};
> Cc["@mozilla.org/dom/indexeddb/manager;1"]
>   .getService(Ci.nsIIndexedDatabaseManager)
>   .initWindowless(moreindexeddb);
> 
> 
> ```

deleteRange ? where is that ?

initWindowless() is used only in indexed-db.js, no ?

Comment 40

[Florian Bender]

I think this bug is supposed to be fixed. Feel free to reopen if needed.

Comment 41

[david.richardson]

For Firefox 27+, initWindowless throws the following error when used in a chrome:// url 

[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIIndexedDatabaseManager.initWindowless]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: debugger eval code :: <TOP_LEVEL> :: line 1"  data: no]

Comment 42

[Jan Varga [:janv]]

(In reply to david.richardson from comment #41)
> For Firefox 27+, initWindowless throws the following error when used in a
> chrome:// url 
> 
> [Exception... "Component returned failure code: 0x80004005
> (NS_ERROR_FAILURE) [nsIIndexedDatabaseManager.initWindowless]"  nsresult:
> "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: debugger eval code
> :: <TOP_LEVEL> :: line 1"  data: no]

that's bug 932345