Last Comment Bug 694145 - IndexedDB: various methods should accept both keys and KeyRanges
: IndexedDB: various methods should accept both keys and KeyRanges
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Ben Turner (not reading bugmail, use the needinfo flag!)
:
: Hsin-Yi Tsai [:hsinyi]
Mentors:
Depends on:
Blocks: idb 692669
  Show dependency treegraph
 
Reported: 2011-10-12 14:28 PDT by Jonas Sicking (:sicking) No longer reading bugmail consistently
Modified: 2015-12-01 02:45 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Remove nsIVariant, v1 (151.53 KB, patch)
2011-10-24 22:41 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
no flags Details | Diff | Splinter Review
Remove nsIVariant, v1 (157.22 KB, patch)
2011-10-24 22:59 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
jonas: review+
Details | Diff | Splinter Review
Remove nsIVariant, v1.1 (165.02 KB, patch)
2011-10-27 15:17 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
jonas: review+
Details | Diff | Splinter Review
Patch, v1 (94.67 KB, patch)
2011-11-02 17:15 PDT, Ben Turner (not reading bugmail, use the needinfo flag!)
jonas: review+
Details | Diff | Splinter Review

Description Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-12 14:28:30 PDT
All three of them
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-12 14:38:12 PDT
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 2011-10-17 16:28:53 PDT
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!) 2011-10-24 22:41:53 PDT
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.
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-10-24 22:46:06 PDT
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!) 2011-10-24 22:59:40 PDT
Created attachment 569286 [details] [diff] [review]
Remove nsIVariant, v1

Oops, that was an old patch. This is the right one.
Comment 6 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-10-25 04:52:02 PDT
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!) 2011-10-25 08:26:51 PDT
(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 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-26 11:21:26 PDT
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!) 2011-10-27 15:17:27 PDT
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.
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-10-27 15:33:02 PDT
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!) 2011-11-01 09:06:12 PDT
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!) 2011-11-02 17:15:51 PDT
Created attachment 571518 [details] [diff] [review]
Patch, v1

This should fix all of the things mentioned in comment 0.
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-11-03 13:19:37 PDT
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!) 2011-11-03 15:35:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/311497d5b2d1
Comment 15 Ed Morley [:emorley] 2011-11-04 03:10:00 PDT
https://hg.mozilla.org/mozilla-central/rev/311497d5b2d1

Note You need to log in before you can comment on or make changes to this bug.