The default bug view has changed. See this FAQ.

IndexedDB: various methods should accept both keys and KeyRanges

RESOLVED FIXED

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: sicking, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({dev-doc-complete})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

All three of them
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)
Summary: IndexedDB: openCursor should accept keys in addition to KeyRanges → IndexedDB: various methods should accept both keys and KeyRanges
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().
Created attachment 569284 [details] [diff] [review]
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.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #569284 - Flags: review?(jonas)
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.
Created attachment 569286 [details] [diff] [review]
Remove nsIVariant, v1

Oops, that was an old patch. This is the right one.
Attachment #569284 - Attachment is obsolete: true
Attachment #569284 - Flags: review?(jonas)
Attachment #569286 - Flags: review?(jonas)
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?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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 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.
Attachment #569286 - Flags: review?(jonas) → review+
Blocks: 692669
Created attachment 570103 [details] [diff] [review]
Remove nsIVariant, v1.1

This changes Key.h to not hold jsvals any more... It was just too complicated.
Attachment #569286 - Attachment is obsolete: true
Attachment #570103 - Flags: review?(jonas)
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
Attachment #570103 - Flags: review?(jonas) → review+
Comment on attachment 570103 [details] [diff] [review]
Remove nsIVariant, v1.1

I moved this patch to bug 692669 where it belongs.
Attachment #570103 - Attachment is obsolete: true
Created attachment 571518 [details] [diff] [review]
Patch, v1

This should fix all of the things mentioned in comment 0.
Attachment #571518 - Flags: review?(jonas)
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!
Attachment #571518 - Flags: review?(jonas) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/311497d5b2d1
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/311497d5b2d1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Keywords: dev-doc-needed
Whiteboard: [inbound]
Component: DOM → DOM: IndexedDB
Target Milestone: mozilla10 → ---
Version: Trunk → unspecified
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
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.