Last Comment Bug 667652 - Make the fix for Bug 663479 less hacky
: Make the fix for Bug 663479 less hacky
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
Depends on:
Blocks: 663479
  Show dependency treegraph
 
Reported: 2011-06-27 15:54 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2011-06-30 09:02 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add a JS_FRIEND_API to get the current position in the buffer (1.70 KB, patch)
2011-06-27 15:57 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
jorendorff: review+
Details | Diff | Review
Change IDB code to be less hacky (14.78 KB, patch)
2011-06-27 15:57 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Review
Patch (15.89 KB, patch)
2011-06-29 09:13 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bent.mozilla: review+
Details | Diff | Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-27 15:54:50 PDT

    
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-27 15:57:10 PDT
Created attachment 542305 [details] [diff] [review]
Add a JS_FRIEND_API to get the current position in the buffer
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-27 15:57:35 PDT
Created attachment 542307 [details] [diff] [review]
Change IDB code to be less hacky
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-27 15:58:45 PDT
This implements Jonas's idea that instead of manually trawling through the clone buffer we store the offset and write directly to that offset later.
Comment 4 :Ms2ger 2011-06-28 04:31:34 PDT
Comment on attachment 542305 [details] [diff] [review]
Add a JS_FRIEND_API to get the current position in the buffer

namespace, pretty please with a cherry on top?
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-28 08:01:20 PDT
(In reply to comment #4)
> namespace, pretty please with a cherry on top?

That's not how the public/friend api works.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-28 08:45:29 PDT
(In reply to comment #5)
> (In reply to comment #4)
> > namespace, pretty please with a cherry on top?
> 
> That's not how the public/friend api works.

Right, which is why I didn't do that.
Comment 7 Jason Orendorff [:jorendorff] 2011-06-28 14:18:35 PDT
Comment on attachment 542305 [details] [diff] [review]
Add a JS_FRIEND_API to get the current position in the buffer

Sure, ok.
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-28 14:35:35 PDT
Shouldn't you be using writeDouble rather than writeBytes?
Comment 9 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-29 07:50:45 PDT
Comment on attachment 542307 [details] [diff] [review]
Change IDB code to be less hacky

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +76,5 @@
>              JSAutoStructuredCloneBuffer& aCloneBuffer,
>              const Key& aKey,
>              bool aOverwrite,
> +            nsTArray<IndexUpdateInfo>& aIndexUpdateInfo,
> +            PRUint64 aOffset)

Nit: This should be more descriptive. Offset to what?

@@ +112,5 @@
>    JSAutoStructuredCloneBuffer mCloneBuffer;
>    Key mKey;
>    const bool mOverwrite;
>    nsTArray<IndexUpdateInfo> mIndexUpdateInfo;
> +  PRUint64 mOffset;

Same here.

@@ +897,5 @@
>  
>  nsresult
>  IDBObjectStore::ModifyValueForNewKey(JSAutoStructuredCloneBuffer& aBuffer,
> +                                     Key& aKey,
> +                                     PRUint64 aOffset)

Same here.

@@ +932,5 @@
> +StructuredCloneWriteDummyProp(JSContext* aCx,
> +                              JSStructuredCloneWriter* aWriter,
> +                              JSObject* aObj,
> +                              void* aClosure)
> +{

Need to only do this for objects of your special class. Right now this hook will run for *any* unrecognized object, like security wrappers, canvas things, DOM elements, etc. You'll want to check based on JS_GET_CLASS(aCx, aObj) == &gDummyPropClass.

@@ +940,5 @@
> +  *closure = js_GetSCOffset(aWriter);
> +
> +  PRUint64 value = 0;
> +  return JS_WriteBytes(aWriter, &value, sizeof(value));
> +}

You need to call the main runtime's hooks if you didn't handle this. Otherwise you may miss globally understood types.

Also, if JS_WriteBytes fails you need to set a CLONE_ERR exception.

@@ +947,5 @@
> +{ "dummy", 0,
> +  JS_PropertyStub,  JS_PropertyStub,
> +  JS_PropertyStub,  JS_StrictPropertyStub,
> +  JS_EnumerateStub, JS_ResolveStub,
> +  JS_ConvertStub,   nsnull };

Please add these to the anonymous namespace, then drop the static on sDummyPropClass, then s/sDummy/gDummy/. Also swap the order so that you can access gDummyPropClass from the write hook.

@@ +1008,5 @@
>    if (!mKeyPath.IsEmpty() && aKey.IsUnset()) {
>      NS_ASSERTION(mAutoIncrement, "Should have bailed earlier!");
>  
> +    JSObject* obj = JS_NewObject(aCx, &sDummyPropClass, nsnull, nsnull);
> +    NS_ENSURE_TRUE(obj, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);

Nit: newline here.
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-06-29 09:13:03 PDT
Created attachment 542836 [details] [diff] [review]
Patch

Review comments addressed.
Comment 11 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-29 09:43:21 PDT
Comment on attachment 542836 [details] [diff] [review]
Patch

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

r=me with these addressed too. Thanks!

::: dom/indexedDB/IDBObjectStore.cpp
@@ +430,5 @@
> +{ "dummy", 0,
> +  JS_PropertyStub,  JS_PropertyStub,
> +  JS_PropertyStub,  JS_StrictPropertyStub,
> +  JS_EnumerateStub, JS_ResolveStub,
> +  JS_ConvertStub,   nsnull };

Oops, missed this before, use JSCLASS_NO_OPTIONAL_MEMBERS rather than 'nsnull' at the end there.

Nit: Put the closing brace on its own line at 0 offset.

@@ +935,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>  }
>  
> +JSBool
> +StructuredCloneWriteDummyProp(JSContext* aCx,

Please move this into the anonymous namespace too.

@@ +950,5 @@
> +    PRUint64 value = 0;
> +    return JS_WriteBytes(aWriter, &value, sizeof(value));
> +  }
> +
> +  // Something failed above, try using the runtime callbacks instead.

Nit: This comment isn't accurate ("something failed above"), just remove that clause.

@@ +968,5 @@
>                             jsval aKeyVal,
>                             JSAutoStructuredCloneBuffer& aCloneBuffer,
>                             Key& aKey,
> +                           nsTArray<IndexUpdateInfo>& aUpdateInfoArray,
> +                           PRUint64& aOffsetToKeyProp)

I'd prefer not to use a reference here, let's use a pointer instead. That way you can see at the call site that it's an out param.

@@ +1035,5 @@
>  
> +  JSStructuredCloneCallbacks callbacks =
> +  { nsnull, /* no special reading */
> +    StructuredCloneWriteDummyProp,
> +    nsnull  /* no special error handling */ };

Nit:

  JSSCC callbacks = {
    nsnull,
    SCWDP,
    nsnull
  };

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