767186 - Always return the same JSObject for IDB[ObjectStore|Index].keyPath

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

Attachment: Patch

Comment 1

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

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

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

All the comments I made for IDBIndex apply to IDBObjectStore.

Need some answers below.

::: dom/indexedDB/IDBIndex.cpp
@@ +410,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>    NS_ASSERTION(!mActorParent, "Actor parent owns us, how can we be dying?!");
> +
> +  if (mRooted) {
> +    NS_DROP_JS_OBJECTS(this, IDBIndex);

Wait... Didn't we just have a bug about calling NS_DROP_JSOBJECTS on idb objects?

@@ +665,3 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IDBIndex)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +

Nit: remove extra line

@@ +673,5 @@
> +
> +  tmp->mCachedKeyPath = JSVAL_VOID;
> +
> +  if (tmp->mRooted) {
> +    NS_DROP_JS_OBJECTS(tmp, IDBIndex);

Hm... Weren't we planning on moving this to IDBWrapperCache?

@@ +717,5 @@
> +    *aVal = mCachedKeyPath;
> +    return NS_OK;
> +  }
> +
> +  nsresult rv = GetKeyPath().ToJSVal(aCx, &mCachedKeyPath);

First assert PreservingWrapper()

@@ +720,5 @@
> +
> +  nsresult rv = GetKeyPath().ToJSVal(aCx, &mCachedKeyPath);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!JSVAL_IS_NULL(mCachedKeyPath)) {

JSVAL_IS_GCTHING will be better.

@@ +722,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!JSVAL_IS_NULL(mCachedKeyPath)) {
> +    mRooted = true;
> +    NS_HOLD_JS_OBJECTS(this, IDBIndex);

Nit: Put mRooted second here.

::: dom/indexedDB/IDBIndex.h
@@ +148,5 @@
>  
>    PRInt64 mId;
>    nsString mName;
>    KeyPath mKeyPath;
> +  JS::Value mCachedKeyPath;

I can't decide if I like this... We should file a followup to replace all jsval with JS::Value I guess.

@@ +149,5 @@
>    PRInt64 mId;
>    nsString mName;
>    KeyPath mKeyPath;
> +  JS::Value mCachedKeyPath;
> +  bool mRooted;

Nit: Stick this below with the other bools so that they'll pack.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1763,3 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IDBObjectStore)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +

Nit: extra line...

Comment 2

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

IDBObjectStore/IDBIndex don't inherit from IDBWrapperCache.

Comment 3

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

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

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

All the comments I made for IDBIndex apply to IDBObjectStore.

Need some answers below.

::: dom/indexedDB/IDBIndex.cpp
@@ +410,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>    NS_ASSERTION(!mActorParent, "Actor parent owns us, how can we be dying?!");
> +
> +  if (mRooted) {
> +    NS_DROP_JS_OBJECTS(this, IDBIndex);

Wait... Didn't we just have a bug about calling NS_DROP_JSOBJECTS on idb objects?

@@ +665,3 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IDBIndex)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +

Nit: remove extra line

@@ +673,5 @@
> +
> +  tmp->mCachedKeyPath = JSVAL_VOID;
> +
> +  if (tmp->mRooted) {
> +    NS_DROP_JS_OBJECTS(tmp, IDBIndex);

Hm... Weren't we planning on moving this to IDBWrapperCache?

@@ +717,5 @@
> +    *aVal = mCachedKeyPath;
> +    return NS_OK;
> +  }
> +
> +  nsresult rv = GetKeyPath().ToJSVal(aCx, &mCachedKeyPath);

First assert PreservingWrapper()

@@ +720,5 @@
> +
> +  nsresult rv = GetKeyPath().ToJSVal(aCx, &mCachedKeyPath);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!JSVAL_IS_NULL(mCachedKeyPath)) {

JSVAL_IS_GCTHING will be better.

@@ +722,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (!JSVAL_IS_NULL(mCachedKeyPath)) {
> +    mRooted = true;
> +    NS_HOLD_JS_OBJECTS(this, IDBIndex);

Nit: Put mRooted second here.

::: dom/indexedDB/IDBIndex.h
@@ +148,5 @@
>  
>    PRInt64 mId;
>    nsString mName;
>    KeyPath mKeyPath;
> +  JS::Value mCachedKeyPath;

I can't decide if I like this... We should file a followup to replace all jsval with JS::Value I guess.

@@ +149,5 @@
>    PRInt64 mId;
>    nsString mName;
>    KeyPath mKeyPath;
> +  JS::Value mCachedKeyPath;
> +  bool mRooted;

Nit: Stick this below with the other bools so that they'll pack.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +1763,3 @@
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(IDBObjectStore)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +

Nit: extra line...

Comment 4

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

Uh... Splinter freaked out, sorry.

r=me!

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/4cbc78fd6eef