765834 - Rework and unify keypath handling

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

Attachment: Patch

Our keypath handling is a mess.  This puts it all in one place.

Comment 1

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

Attachment: Patch

Fixed an assertion that was firing in certain situations.

Comment 2

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

Comment on attachment 634131 [details] [diff] [review]
Patch

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

::: dom/indexedDB/DatabaseInfo.h
@@ +18,5 @@
>  
>  BEGIN_INDEXEDDB_NAMESPACE
>  
>  class IndexedDBDatabaseChild;
> +class KeyPath;

Nit: nuke

::: dom/indexedDB/IDBCursor.cpp
@@ +715,5 @@
>  
>      return mObjectStore->Put(aValue, JSVAL_VOID, aCx, 0, _retval);
>    }
>  
> +  jsval keyVal = JSVAL_VOID;

Revert.

::: dom/indexedDB/IDBDatabase.cpp
@@ +483,5 @@
>      }
>  
> +    // We need a default value here, which the XPIDL dictionary stuff doesn't
> +    // support.  WebIDL shall save us all!
> +    JSBool hasProp = JS_FALSE;

'false'

@@ +486,5 @@
> +    // support.  WebIDL shall save us all!
> +    JSBool hasProp = JS_FALSE;
> +    JSObject* obj = JSVAL_TO_OBJECT(aOptions);
> +    if (!JS_HasProperty(aCx, obj, "keyPath", &hasProp)) {
> +      // Wtf?

No.

::: dom/indexedDB/IDBFactory.cpp
@@ +236,5 @@
>  
>      PRInt32 columnType;
>      nsresult rv = stmt->GetTypeOfIndex(2, &columnType);
>      NS_ENSURE_SUCCESS(rv, rv);
> +    // NB: We don't have to handle the NULL case, since that is the default

Nit: extra line above please

@@ +299,2 @@
>  
> +    indexInfo->keyPath = KeyPath::DeserializeFromString(keyPathSerialization);

Please assert that we have a non-empty keypath here.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +50,5 @@
>  namespace {
>  
> +inline
> +bool
> +IgnoreWhitespace(PRUnichar c)

Let's rename to 'IgnoreNothing'

@@ +563,1 @@
>  JSClass gDummyPropClass = {

Let's make this a static const member of IDBObjectStore.

@@ +646,1 @@
>    }

Missing return here. (!!!)

@@ +650,3 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  if (aMultiEntry && !JSVAL_IS_PRIMITIVE(val) &&

aMultiEntry is unnecessary now

@@ +1303,5 @@
>  
> +struct GetAddInfoClosure
> +{
> +  IDBObjectStore* mThis;
> +  StructuredCloneWriteInfo* mCloneWriteInfo;

Ref would look better.

@@ +1308,5 @@
> +  jsval mValue;
> +};
> +
> +static nsresult
> +GetAddInfoCallback(JSContext* aCx, void* aClosure)

Let's move this to the anonymous namespace, then lose 'static'. Same for the struct.

@@ +1311,5 @@
> +static nsresult
> +GetAddInfoCallback(JSContext* aCx, void* aClosure)
> +{
> +  GetAddInfoClosure* data =
> +    static_cast<GetAddInfoClosure*>(aClosure);

One line?

@@ +1350,5 @@
>        return rv;
>      }
>    }
>    else if (!mAutoIncrement) {
> +    nsresult rv = GetKeyPath().ExtractKey(aCx, aValue, aKey);

Nit: Already have an rv somewhere above

@@ +1352,5 @@
>    }
>    else if (!mAutoIncrement) {
> +    nsresult rv = GetKeyPath().ExtractKey(aCx, aValue, aKey);
> +	if (NS_FAILED(rv)) {
> +      return rv;

Nit: Some kind of indent problem

@@ +1378,5 @@
>    nsString targetObjectPropName;
>    JSObject* targetObject = nsnull;
>  
> +  GetAddInfoClosure data;
> +  data.mThis = this;

If it compiles let's use { } notation?

::: dom/indexedDB/KeyPath.cpp
@@ +72,5 @@
> +nsresult
> +GetJSValFromKeyPathString(JSContext* aCx,
> +                          const JS::Value& aValue,
> +                          const nsAString& aKeyPathString,
> +                          JS::Value& aKeyJSVal,

Let's make this a jsval*

@@ +125,5 @@
> +        // Treat explicitly undefined as an error.
> +        if (intermediate == JSVAL_VOID) {
> +          return NS_ERROR_DOM_INDEXEDDB_DATA_ERR;
> +        }
> +        else if (tokenizer.hasMoreTokens()) {

Nit: else after return

@@ +138,5 @@
> +          aKeyJSVal = intermediate;
> +        }
> +      }
> +
> +      if (!hasProp) {

Nit: else

@@ +214,5 @@
> +    NS_ENSURE_TRUE(JSVAL_TO_BOOLEAN(succeeded),
> +                   NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +  }
> +
> +  return NS_OK;

Return rv? And warn if failure

@@ +219,5 @@
> +}
> +
> +} // anonymous namespace
> +
> +/* static */

//

@@ +221,5 @@
> +} // anonymous namespace
> +
> +/* static */
> +bool
> +KeyPath::Parse(JSContext* aCx, const JS::Value& aValue, KeyPath& aKeyPath)

Nit: each arg on its own line

@@ +225,5 @@
> +KeyPath::Parse(JSContext* aCx, const JS::Value& aValue, KeyPath& aKeyPath)
> +{
> +  KeyPath keyPath;
> +
> +  aKeyPath = keyPath;

aKeyPath.SetType(Invalid)

@@ +249,5 @@
> +      jsval val;
> +      JSString* jsstr;
> +      nsDependentJSString str;
> +      if (!JS_GetElement(aCx, obj, index, &val) ||
> +          !(jsstr = JS_ValueToString(aCx, val)) ||

Let's double check that this is right.

@@ +260,5 @@
> +      }
> +    }
> +  }
> +  // Otherwise convert it to a string.
> +  else if (aValue != JSVAL_NULL) {

!JSVAL_IS_NULL. And what about undefined?

Nit: Please move comment inside if block.

@@ +283,5 @@
> +void
> +KeyPath::SetType(KeyPathType aType)
> +{
> +  NS_ASSERTION(aType == String || aType == Array, "Invalid type!");
> +  mType = aType;

Make sure to clear mStrings

@@ +295,5 @@
> +  }
> +
> +  if (IsString()) {
> +    NS_ASSERTION(mStrings.Length() == 0, "Too many strings!");
> +    return !!mStrings.AppendElement(aString);

Infallible now

@@ +299,5 @@
> +    return !!mStrings.AppendElement(aString);
> +  }
> +
> +  if (IsArray()) {
> +    return !!mStrings.AppendElement(aString);

And here

@@ +318,5 @@
> +  for (PRUint32 i = 0; i < len; ++i) {
> +    nsresult rv = GetJSValFromKeyPathString(aCx, aValue, mStrings[i], value,
> +                                            DoNotCreateProperties, nsnull,
> +                                            nsnull);
> +    NS_ENSURE_SUCCESS(rv, rv);

I think we don't want the warning here.

@@ +333,5 @@
> +}
> +
> +nsresult
> +KeyPath::ExtractKeyAsJSVal(JSContext* aCx, const JS::Value& aValue,
> +                           JS::Value& aOutVal) const

Let's replace this with a call to ExtractKey() followed by ToJSVal().

@@ +378,5 @@
> +
> +  nsresult rv = GetJSValFromKeyPathString(aCx, aValue, mStrings[0], value,
> +                                          CreateProperties, aCallback,
> +                                          aClosure);
> +  NS_ENSURE_SUCCESS(rv, rv);

No warning here.

@@ +382,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (NS_FAILED(aKey.AppendItem(aCx, false, value))) {
> +    NS_ASSERTION(aKey.IsUnset(), "Should be unset");
> +    return value == JSVAL_VOID ? NS_OK : NS_ERROR_DOM_INDEXEDDB_DATA_ERR;

JSVAL_IS_VOID

@@ +391,5 @@
> +  return NS_OK;
> +}
> +
> +bool
> +KeyPath::operator==(const KeyPath& aOther) const

inline in header, 'return mType == && mStrings =='

@@ +408,5 @@
> +  return true;
> +}
> +
> +nsString
> +KeyPath::SerializeToString() const

All our other string methods take outparams... Let's do that here.

@@ +435,5 @@
> +  NS_NOTREACHED("What?");
> +  return EmptyString();
> +}
> +
> +/* static */

//

@@ +442,5 @@
> +{
> +  KeyPath keyPath;
> +
> +  if (!aString.IsEmpty() && aString.First() == ',') {
> +    keyPath.mType = Array;

Should be SetType()

@@ +456,5 @@
> +
> +    return keyPath;
> +  }
> +
> +  keyPath.mType = String;

SetType()

@@ +516,5 @@
> +    return false;
> +  }
> +
> +  // Neither are empty strings.
> +  if (IsString() && mStrings[0] == EmptyString()) {

mStrings[0].IsEmpty()

::: dom/indexedDB/KeyPath.h
@@ +23,5 @@
> +{
> +  template<class T> friend struct IPC::ParamTraits;
> +
> +  enum KeyPathType {
> +    Invalid,

Nonexistant

@@ +26,5 @@
> +  enum KeyPathType {
> +    Invalid,
> +    String,
> +    Array,
> +    EndGuard

Nit: all caps to match the rest

@@ +31,5 @@
> +  };
> +
> +  void SetType(KeyPathType aType);
> +
> +  bool AppendStringWithValidation(JSContext* aCx, const nsAString& aString);

NIt: comment about not throwing

@@ +35,5 @@
> +  bool AppendStringWithValidation(JSContext* aCx, const nsAString& aString);
> +
> +public:
> +  KeyPath()
> +  : mType(Invalid)

Please make this take an arg of some kind so we can find bugs easier. You can have a private no-arg constructor, but the public one should require something.

@@ +44,5 @@
> +  KeyPath(const KeyPath& aOther)
> +  : mType(aOther.mType),
> +    mStrings(aOther.mStrings)
> +  {
> +    MOZ_COUNT_CTOR(KeyPath);

*this = aOther.

@@ +53,5 @@
> +    MOZ_COUNT_DTOR(KeyPath);
> +  }
> +
> +  static bool
> +  Parse(JSContext* aCx, const JS::Value& aValue, KeyPath& aKeyPath);

Let's do this as an nsresult

@@ +64,5 @@
> +                    JS::Value& aOutVal) const;
> +
> +  typedef nsresult
> +  (*ExtractOrCreateKeyCallback)(JSContext* aCx,
> +                                void* aClosure);

Nit: can fit on one line

@@ +71,5 @@
> +  ExtractOrCreateKey(JSContext* aCx, const JS::Value& aValue, Key& aKey,
> +                     ExtractOrCreateKeyCallback aCallback,
> +                     void* aClosure) const;
> +
> +  inline bool IsValid() const {

Let's nuke the 'inline' here and below

@@ +95,5 @@
> +
> +  nsresult ToJSVal(JSContext* aCx, JS::Value* aValue) const;
> +
> +  bool IsAllowedForObjectStore(bool aAutoIncrement) const;
> +private:

Nit: extra newline above here

::: dom/indexedDB/ipc/IndexedDBParent.cpp
@@ +959,5 @@
>  
>      const CreateIndexParams& params = aParams.get_CreateIndexParams();
>      const IndexInfo& info = params.info();
>  
>      // Copy...

No longer needed

::: dom/indexedDB/ipc/SerializationHelpers.h
@@ +42,5 @@
> +                        mozilla::dom::indexedDB::KeyPath::EndGuard>
> +{ };
> +
> +template <>
> +struct ParamTraits<mozilla::dom::indexedDB::KeyPath>

Please add KeyPath.h to includes here.

Comment 3

[Landry Breuil (:gaston)]

There's a leading utf-8 vomit at the beginning of first line of keypath.h and keypath.cpp.. that chokes gcc here.


dom/indexedDB/Key.cpp:     ASCII C program text
dom/indexedDB/Key.h:       ASCII C program text
dom/indexedDB/KeyPath.cpp: UTF-8 Unicode (with BOM) C program text
dom/indexedDB/KeyPath.h:   UTF-8 Unicode (with BOM) C program text

../../dist/include/mozilla/dom/indexedDB/KeyPath.h:1: error: stray '\357' in program
../../dist/include/mozilla/dom/indexedDB/KeyPath.h:1: error: stray '\273' in program
../../dist/include/mozilla/dom/indexedDB/KeyPath.h:1: error: stray '\277' in program

First line look like :

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Comment 4

[Landry Breuil (:gaston)]

Attachment: remove BOM

Comment 5

[Landry Breuil (:gaston)]

http://hg.mozilla.org/mozilla-central/rev/4d17545dba03 for the BOM removal

(http://hg.mozilla.org/mozilla-central/rev/f65133bc5723 for the initial commit)

Comment 6

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

https://hg.mozilla.org/mozilla-central/rev/f65133bc5723
https://hg.mozilla.org/mozilla-central/rev/0b7e8d848bff