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

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
7 years ago
2 months ago

People

(Reporter: Ms2ger, Assigned: janv)

Tracking

(Depends on 2 bugs, Blocks 1 bug, Regressed 1 bug)

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

No description provided.
Posted patch WIP (obsolete) — Splinter Review
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.
Attachment #704514 - Flags: feedback?(khuey)
Blocks: SyncIDB
Is this still relevant enough to review?  I suspect it may have bitrotted substantially in the last 4 months.
Posted patch WIP v2 (obsolete) — Splinter Review
Attachment #704514 - Attachment is obsolete: true
Attachment #704514 - Flags: feedback?(khuey)
Attachment #755282 - Flags: feedback?(khuey)
Comment on attachment 755282 [details] [diff] [review]
WIP v2

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

Why does this need to inherit from nsWrapperCache?
Depends on: 890364
No longer blocks: SyncIDB
Posted patch Patch v1 (obsolete) — Splinter Review
Attachment #755282 - Attachment is obsolete: true
Attachment #755282 - Flags: feedback?(khuey)
Attachment #782286 - Flags: review?(khuey)
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?
Attachment #782286 - Flags: review?(khuey) → review+
Depends on: 900577
Depends on: 900578
Can we land this?
Flags: needinfo?(Ms2ger)
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}]
Flags: needinfo?(Ms2ger)
+  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?
Flags: needinfo?(Ms2ger)
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.
Flags: needinfo?(Ms2ger) → needinfo?(Jan.Varga)
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.
Flags: needinfo?(Jan.Varga)
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.
Sounds good to me.
Oh, and:

1a. Make sure we have tests for all the different spots (bootstrap.js, "ipcshell", components, JSMs, jetpack sandbox, anything else?)
(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
See dom/workers/test/extensions/bootstrap/ for an example of a test extension that lets you try things in bootstrap.js.
(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.
Summary: Move IDBKeyRange to Paris bindings → Move IDBKeyRange to WebIDL and define indexedDB/IDBKeyRange in all the spots
Assignee: Ms2ger → Jan.Varga
Posted patch rebased patch v1 (obsolete) — Splinter Review
Attachment #782286 - Attachment is obsolete: true
Posted patch all in one patch (obsolete) — Splinter Review
Attachment #808756 - Flags: review?(bent.mozilla)
Posted patch interdiff (obsolete) — Splinter Review
Attachment #808753 - Attachment is obsolete: true
Attachment #808756 - Attachment is obsolete: true
Attachment #808758 - Attachment is obsolete: true
Attachment #808756 - Flags: review?(bent.mozilla)
Attachment #811222 - Flags: review?(bent.mozilla)
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!
Attachment #811222 - Flags: review?(bent.mozilla) → review+
Gabor, could you please upstream the addon-sdk changes, once it appears on m-c ?
https://hg.mozilla.org/mozilla-central/rev/81f8a3c52c23
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 922837
This causes consistent fatal assertions in DEBUG builds when using google search, on at least the two devices I've tested.
(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.
This appears to be causing fatal assertions in DEBUG builds on most websites.
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)
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?
(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?
Sure... Bug 932345
(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.
Yes, that works in both FF25 and FF28 (nightly just updated...)

Thanks.

I'll repost your answer in my bug.
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);


```
Status: RESOLVED → REOPENED
Flags: needinfo?(Jan.Varga)
Resolution: FIXED → ---
(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 ?
Flags: needinfo?(Jan.Varga)
I think this bug is supposed to be fixed. Feel free to reopen if needed.
Status: REOPENED → RESOLVED
Closed: 6 years ago5 years ago
Resolution: --- → FIXED
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]
(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
Depends on: 1122204
Regressions: 919946
You need to log in before you can comment on or make changes to this bug.