667652 - Make the fix for Bug 663479 less hacky

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

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

Attachment: Add a JS_FRIEND_API to get the current position in the buffer

Comment 2

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

Attachment: Change IDB code to be less hacky

Comment 3

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

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 (he/him; ⌚ UTC+1/+2)]

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!)]

(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 (Exited; not receiving bugmail, old account, do not use)]

(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]]

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) No longer reading bugmail consistently]

Shouldn't you be using writeDouble rather than writeBytes?

Comment 9

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

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 (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Review comments addressed.

Comment 11

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

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
  };

Comment 12

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

http://hg.mozilla.org/mozilla-central/rev/594b9b4cb1df
http://hg.mozilla.org/mozilla-central/rev/b2a1b2fd6d23