Last Comment Bug 706068 - IndexedDB: Keys placed on an object by an autoincremented objectStore ignore complex keyPaths
: IndexedDB: Keys placed on an object by an autoincremented objectStore ignore ...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
:
:
Mentors:
Depends on:
Blocks: idb
  Show dependency treegraph
 
Reported: 2011-11-29 05:11 PST by Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
Modified: 2012-03-22 11:50 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (9.87 KB, patch)
2011-11-29 05:11 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
no flags Details | Diff | Splinter Review
Patch (7.30 KB, patch)
2011-11-29 05:14 PST, Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)
jonas: review+
Details | Diff | Splinter Review
Followup fix (12.87 KB, patch)
2011-12-02 17:12 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
bent.mozilla: review+
Details | Diff | Splinter Review
Fix review comments of part 1 (4.12 KB, patch)
2011-12-02 17:13 PST, Jonas Sicking (:sicking) No longer reading bugmail consistently
jonas: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-29 05:11:10 PST
Created attachment 577553 [details] [diff] [review]
Patch

if we put {} into an autoincrementing objectStore with keyPath "foo.bar", we'll get back { foo.bar: keyValue } instead of { foo: { bar: keyValue } }.
Comment 1 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-29 05:14:40 PST
Created attachment 577554 [details] [diff] [review]
Patch

The right diff, this time.
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-02 11:12:31 PST
Comment on attachment 577554 [details] [diff] [review]
Patch

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +926,5 @@
>  
>      rv = GetKeyFromValue(aCx, aValue, mKeyPath, aKey);
> +    // If we're an autoincrementing objectStore, this call can fail.
> +    if (!mAutoIncrement) {
> +      NS_ENSURE_SUCCESS(rv, rv);

Nit: I'd prefer:

  if (NS_FAILED(rv) && !mAutoIncrement) {
    return rv;
  }

@@ +948,5 @@
>  
>    const jschar* keyPathChars =
>      reinterpret_cast<const jschar*>(mKeyPath.get());
>    const size_t keyPathLen = mKeyPath.Length();
> +  nsString firstToken, lastToken;

nsDependentSubstring I think

@@ +965,5 @@
> +
> +    // Recursively define empty objects on the object until we're at the
> +    // end of the keyPath.
> +    while(tokenizer.hasMoreTokens()) {
> +      nsString token(tokenizer.nextToken());

const nsDependentString&

@@ +989,5 @@
> +          break;
> +        }
> +
> +        obj = dummy;
> +      }

Hm, so, you're setting firstToken and lastToken before trying to define that property... If that fails, you'll still leave them set. I think you should probably set these after the prop set succeeds?

@@ +992,5 @@
> +        obj = dummy;
> +      }
> +    }
> +
> +    JSObject* dummy = JS_NewObject(aCx, &gDummyPropClass, nsnull, nsnull);

You probably want to put all this inside an |if (NS_SUCCEEDED(rv))| block in case something in the loop failed right?
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-02 11:40:14 PST
Comment on attachment 577554 [details] [diff] [review]
Patch

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

r=me with those changes.

Though would you mind if I land this on top of my other patches? I can do the merging if you attach a updated patch.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +927,5 @@
>      rv = GetKeyFromValue(aCx, aValue, mKeyPath, aKey);
> +    // If we're an autoincrementing objectStore, this call can fail.
> +    if (!mAutoIncrement) {
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    }

This change should no longer be needed

@@ +965,5 @@
> +
> +    // Recursively define empty objects on the object until we're at the
> +    // end of the keyPath.
> +    while(tokenizer.hasMoreTokens()) {
> +      nsString token(tokenizer.nextToken());

use |const nsDependentString&|

@@ +971,5 @@
> +      if (firstToken.IsEmpty()) {
> +        firstToken = token;
> +      }
> +
> +      lastToken = token;

You can wrap this in a

if (!tokenizer.hasMoreTokens())

Especially since you can then turn the below if-statement into an else

@@ +1001,5 @@
> +    jsval key = OBJECT_TO_JSVAL(dummy);
> +
> +    if (!JS_DefineUCProperty(aCx, obj, lastToken.get(), lastToken.Length(),
> +                             key, nsnull, nsnull, JSPROP_ENUMERATE)) {
> +      rv = NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR;

Actually, you can just get rid of lastToken completely and do this inside an |else| after the tokenizer.hasMoreTokens() branch.
Comment 4 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-12-02 11:48:15 PST
(In reply to Jonas Sicking (:sicking) from comment #3)
> Comment on attachment 577554 [details] [diff] [review] [diff] [details] [review]
> Patch
> 
> Review of attachment 577554 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> r=me with those changes.
> 
> Though would you mind if I land this on top of my other patches? I can do
> the merging if you attach a updated patch.

I'm not going to update this before I'm gone for the weekend.  Just land your stuff and I'll deal with the pain next week.
Comment 5 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-02 17:12:15 PST
Created attachment 578776 [details] [diff] [review]
Followup fix

This is a followup fix.

The problem with the previous patch is that it didn't handle the case when the keypath was partially there.

So if you have a keypath like "foo.id" and attempting to store an object like:

{ foo: {} }

There's also error conditions like

{ foo: { id: /x/ } }

To avoid walking the keypath multiple times (once to try to look for an existing key value, and once to find where to start inserting objects) this patch combines looking for a pre-set value with setting up the new value if it doesn't exist.
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-02 17:13:30 PST
Created attachment 578777 [details] [diff] [review]
Fix review comments of part 1

This is Kyle's patch with review comments from me and bent fixed, and rebased on top of other patches i'm about to land.
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-12-02 17:52:23 PST
Comment on attachment 578776 [details] [diff] [review]
Followup fix

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

r=me!

::: dom/indexedDB/IDBObjectStore.cpp
@@ +979,5 @@
> +        if (hasProp) {
> +          // Get if the property exists...
> +          jsval intermediate;
> +          JSBool ok = JS_GetUCProperty(aCx, obj,
> +                                       keyPathChars, keyPathLen, &intermediate);

Nit, rewrap these args.

@@ +983,5 @@
> +                                       keyPathChars, keyPathLen, &intermediate);
> +          NS_ENSURE_TRUE(ok, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +          // ...and if it's the last step in the path use it as key...
> +          if (!tokenizer.hasMoreTokens()) {

Nit: Since you're going to have a block for both cases please swap these and have the non-negated condition here.

@@ +988,5 @@
> +            aKey.SetFromJSVal(aCx, intermediate);
> +            if (aKey.IsUnset()) {
> +              return NS_ERROR_DOM_INDEXEDDB_DATA_ERR;
> +            }
> +            NS_ENSURE_SUCCESS(rv, rv);

This shouldn't be here, right? You're not changing rv anywhere...

@@ +1007,3 @@
>          }
>        }
> +      if (targetObject) {

Nit, add an empty line above here.
Comment 8 Ed Morley [:emorley] 2011-12-03 04:23:45 PST
This landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56aba64236d8

But was backed out again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a2106940402
Comment 9 Matt Brubeck (:mbrubeck) 2011-12-05 10:15:12 PST
Re-landed via inbound:
https://hg.mozilla.org/mozilla-central/rev/82e7bc80eb49

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