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)

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().

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 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.

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

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.

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

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 on attachment 570103 [details] [diff] [review] Remove nsIVariant, v1.1 I moved this patch to bug 692669 where it belongs.

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

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!

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