Last Comment Bug 767186 - Always return the same JSObject for IDB[ObjectStore|Index].keyPath
: Always return the same JSObject for IDB[ObjectStore|Index].keyPath
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on: 765834
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-21 15:51 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2012-06-24 12:45 PDT (History)
1 user (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (10.43 KB, patch)
2012-06-21 15:51 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bent.mozilla: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-21 15:51:58 PDT
Created attachment 635513 [details] [diff] [review]
Patch
Comment 1 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-21 20:50:35 PDT
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 [:khuey] (khuey@mozilla.com) 2012-06-21 20:57:28 PDT
IDBObjectStore/IDBIndex don't inherit from IDBWrapperCache.
Comment 3 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-06-21 21:06:34 PDT
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!) 2012-06-21 21:06:55 PDT
Uh... Splinter freaked out, sorry.

r=me!
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-06-24 12:45:33 PDT
https://hg.mozilla.org/mozilla-central/rev/4cbc78fd6eef

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