694145 - IndexedDB: various methods should accept both keys and KeyRanges

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

All three of them

Comment 1

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

Making bug more generic. 

Here's the list so far of methods that needs to be able to take both but doesn't:

IDBObjectStore.openCursor
IDBIndex.openCursor
IDBIndex.openKeyCursor
IDBObjectStore.delete
IDBIndex.get     (returns the first)
IDBIndex.getKey  (returns the first)
IDBObjectStore.getAll
IDBIndex.getAll
IDBIndex.getAllKeys


We're actually getting IDBObjectStore.get correct!! (Bent wants to point out that we're getting all of them correct per what the spec looked like when he implemented them)

Comment 2

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

All of these methods should also take null for "everything". Possibly we don't want to do that for delete as it could mean that a bug nukes all data. Also we already have IDBObjectStore.clear().

Comment 3

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

Attachment: Remove nsIVariant, v1

Before I can really do anything here (and before we can mess with arrays-as-keys) we need to make sure that nsIVariant usage is gone. This patch is big, but the real changes are in IDBKeyRange.cpp and Key.h. All the tests pass so I think you can rubberstamp the rest.

Comment 4

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

Comment on attachment 569284 [details] [diff] [review]
Remove nsIVariant, v1

khuey, feel free look this over if you want to. Two pairs of eyes would be great.

Comment 5

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

Attachment: Remove nsIVariant, v1

Oops, that was an old patch. This is the right one.

Comment 6

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

Comment on attachment 569286 [details] [diff] [review]
Remove nsIVariant, v1

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

I just skimmed the patch.  A few comments.

::: dom/indexedDB/IDBDatabase.cpp
@@ +623,5 @@
> +  if (!JSVAL_IS_PRIMITIVE(aStoreNames)) {
> +    JSObject* obj = JSVAL_TO_OBJECT(aStoreNames);
> +
> +    // See if this is a JS array.
> +    if (JS_IsArrayObject(aCx, obj)) {

I think you need a null check here.  I don't think you can pass null to JS_IsArrayObject.

@@ +630,5 @@
> +        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +      }
> +
> +      if (!length) {
> +        return NS_ERROR_DOM_INVALID_ACCESS_ERR;

Should we really be passing back a generic DOM error code here?

@@ +658,5 @@
> +      nsIXPConnect* xpc = nsContentUtils::XPConnect();
> +      NS_ASSERTION(xpc, "This should never be null!");
> +
> +      nsCOMPtr<nsIXPConnectWrappedNative> wrapper;
> +      rv = xpc->GetWrappedNativeOfJSObject(aCx, obj, getter_AddRefs(wrapper));

And I know you can't pass null to GetWrappedNativeOfJSObject.

::: dom/indexedDB/nsIIDBDatabase.idl
@@ +77,5 @@
>  
>    [optional_argc, implicit_jscontext]
>    nsIIDBTransaction
> +  transaction(in jsval storeNames, // js array of strings
> +              [optional /* READ_ONLY */] in unsigned short mode);

Why did we drop the timeout here?

Comment 7

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #6)
> I think you need a null check here.

|JSVAL_IS_PRIMITIVE| is basically |!JSVAL_IS_OBJECT || JSVAL_IS_NULL| so this negated test only returns true for a real (non-null) object.

> Should we really be passing back a generic DOM error code here?

Blame sicking, he says it's going into the spec soon.

> Why did we drop the timeout here?

It's no longer part of the spec, we need to rip out all the timeout stuff.

Comment 8

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

Comment on attachment 569286 [details] [diff] [review]
Remove nsIVariant, v1

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

r=me with the below fixed/checked.

::: dom/indexedDB/IDBKeyRange.cpp
@@ +213,1 @@
>    }

We should also throw if lower > upper

@@ +284,5 @@
> +
> +    // See if it's one of our IDBKeyRange objects.
> +    keyRange = do_QueryInterface(wrappedObject);
> +
> +    if (!keyRange) {

I think this is dead code. Booo!

@@ +368,2 @@
>    NS_INTERFACE_MAP_ENTRY(nsIIDBKeyRange)
> +  NS_INTERFACE_MAP_ENTRY(IDBKeyRange)

If you add [builtinclass] you can get rid of this.

::: dom/indexedDB/IDBKeyRange.h
@@ +47,5 @@
> +#include "Key.h"
> +
> +// {9c4f058f-1e56-4050-a075-bf86f1bc1c4d}
> +#define IDBKEYRANGE_IID \
> +  {0x9c4f058f, 0x1e56, 0x4050, {0xa0, 0x75, 0xbf, 0x86, 0xf1, 0xbc, 0x1c, 0x4d}}

If you add [builtinclass] you can get rid of this.

@@ +54,5 @@
>  
>  class IDBKeyRange : public nsIIDBKeyRange
>  {
>  public:
> +  NS_DECLARE_STATIC_IID_ACCESSOR(IDBKEYRANGE_IID)

If you add [builtinclass] you can get rid of this.

::: dom/indexedDB/Key.h
@@ +61,5 @@
>    {
>      *this = aOther;
>    }
>  
>    Key& operator=(const Key& aOther)

This seems very risky.

WON'T SOMEONE PLEASE THINK OF THE CHILDREN!

@@ +119,5 @@
>    bool operator<(const Key& aOther) const
>    {
>      switch (mType) {
> +      case KEYTYPE_VOID:
> +        if (aOther.mType == KEYTYPE_VOID) {

Remove all handling in this function of unset keys. Instead assert up top if either this key or the other is unset.

@@ +186,5 @@
> +    mJSValIsDirty = true;
> +
> +    mTempStringBuffer = nsStringBuffer::FromString(aString);
> +    NS_ASSERTION(mTempStringBuffer,
> +                 "We should only be dealing with shared strings!");

Make sure this works for empty strings.

@@ +214,5 @@
> +        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +      }
> +      size_t length;
> +      const jschar* chars =
> +        JS_GetStringCharsAndLength(aCx, JSVAL_TO_STRING(aVal), &length);

Make sure the JS-string is immutable.

@@ +247,5 @@
> +    return NS_OK;
> +  }
> +
> +  nsresult ToJSVal(JSContext* aCx,
> +                   jsval* aVal) const

Can you look into renaming this to ToJSValNoCache or some such?

@@ +401,5 @@
> +    KEYTYPE_INTEGER
> +  };
> +
> +  // Only touched on the main thread, and only rooted if holding a GCThing.
> +  nsAutoJSValHolder mJSVal;

Please double-check with mrbkap that compartments really don't hold on to anything.

Comment 9

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

Attachment: Remove nsIVariant, v1.1

This changes Key.h to not hold jsvals any more... It was just too complicated.

Comment 10

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

Comment on attachment 570103 [details] [diff] [review]
Remove nsIVariant, v1.1

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

::: dom/indexedDB/Key.h
@@ +191,5 @@
> +        return NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;
> +      }
> +    }
> +    else if (IsInteger()) {
> +      if (!JS_NewNumberValue(aCx, static_cast<double>(ToInteger()), aVal)) {

Use jsdouble

Comment 11

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

Comment on attachment 570103 [details] [diff] [review]
Remove nsIVariant, v1.1

I moved this patch to bug 692669 where it belongs.

Comment 12

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

Attachment: Patch, v1

This should fix all of the things mentioned in comment 0.

Comment 13

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

Comment on attachment 571518 [details] [diff] [review]
Patch, v1

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

I still think some of the string concatenation is silly. But it looks ok :)

r=me either way. Yay!

Comment 14

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/311497d5b2d1

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/311497d5b2d1

Comment 16

[Chris Mills [:cmills]]

I've gone through all the doc pages for these methods, and made sure this detail is made clear throughout:

https://developer.mozilla.org/en-US/docs/Web/API/IDBObjectStore/getAll
https://developer.mozilla.org/en-US/docs/Web/API/IDBObjectStore/openCursor
https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex/getAll
https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex/openKeyCursor
https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex/openCursor
https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex/get
https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex/getKey
https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex/getAll
https://developer.mozilla.org/en-US/docs/Web/API/IDBIndex/getAllKeys

I didn't make a change to IDBObjectStore.delete, to be clear. I'm pretty sure that isn't affected by this bug?

I've also added a note to 

https://developer.mozilla.org/en-US/Firefox/Releases/10#IndexedDB