663479 - Move all IDB structured clone serialization/deserialization to the main thread.

Status: RESOLVED FIXED

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

Description

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

Attachment: Patch

This means removing the deserialization/reserialization that happens in IDBObjectStore::ModifyValueForNewKey.

Instead of deserializing and reserializing and poking at the object through the JSAPI, we poke at the raw byte stream.  We serialize a dummy object to figure out what the relevant part of the byte stream looks like and then we modify it directly to have the key value in it.

Extremely hacky, but it gets the job done.

Comment 1

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

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

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

::: dom/indexedDB/IDBObjectStore.cpp
@@ +843,5 @@
>                                   JSAutoStructuredCloneBuffer& aBuffer,
>                                   jsval* aValue)
>  {
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(),
> +               "Should only be deserializing on the main thread!");

NS_ASSERTION

@@ +862,5 @@
>  IDBObjectStore::SerializeValue(JSContext* aCx,
>                                 JSAutoStructuredCloneBuffer& aBuffer,
>                                 jsval aValue)
>  {
> +  NS_ABORT_IF_FALSE(NS_IsMainThread(),

NS_ASSERTION

@@ +871,5 @@
>  
>    return aBuffer.write(aCx, aValue, nsnull);
>  }
>  
> +static jsdouble

static inline, and I don't understand why you're returning a jsdouble. Does that compile on windows anyway?

Also, let's get a bug on file to expose jsclone.cpp's swap functions to other callers so we don't have to duplicate this.

@@ +891,5 @@
> +
> +nsresult
> +IDBObjectStore::ModifyValueForNewKey(JSAutoStructuredCloneBuffer& aBuffer,
> +                                     Key& aKey)
> +{

Assert that this is only ever called on a non-main thread.

@@ +899,5 @@
> +  // that terminates the buffer
> +  const PRUint32 keyPropLen = mKeyPathSerialization.nbytes() -
> +                              mKeyPathSerializationOffset - 8;
> +
> +  PRInt32 idx = 0;

Nit: 'index' is a little less cryptic.

@@ +910,5 @@
> +
> +  union {
> +    jsdouble d;
> +    uint64_t u;
> +  } pun;

Why is this union necessary? Especially since jsdouble and uint64_t are supposed to be the same size? (Probably asserted in jseng).

@@ +988,5 @@
> +    reinterpret_cast<const jschar*>(mKeyPath.get());
> +  const size_t keyPathLen = mKeyPath.Length();
> +  JSBool ok;
> +
> +  bool setKeyValue = false;

No need for this, just use 'ok'.

@@ +995,5 @@
> +
> +#ifdef DEBUG
> +    jsval existingProp;
> +    ok = JS_GetUCProperty(aCx, JSVAL_TO_OBJECT(aValue), keyPathChars,
> +                          keyPathLen, &existingProp);

Don't do this, it could run a getter and then DEBUG builds would behave differently from non-DEBUG builds. Plus, we've already checked this when setting aKey.

@@ +1006,5 @@
> +    NS_ENSURE_TRUE(ok, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> + 
> +    ok = JS_DefineUCProperty(aCx, JSVAL_TO_OBJECT(aValue), keyPathChars,
> +                             keyPathLen, key, nsnull,
> +                             nsnull, JSPROP_ENUMERATE);

Actually, let's not make it enumerable. No need.

Nit: You can probably wrap those args better.

@@ +1018,5 @@
>  
> +  // We guard on rv being a success because we need to run the property
> +  // deletion code below even if we should not be serializing the value
> +  if (NS_SUCCEEDED(rv)) {
> +    if (!IDBObjectStore::SerializeValue(aCx, aCloneBuffer, aValue)) {

if (NS_SUCCEEDED(rv) && !Serialize(...))

@@ +1029,5 @@
> +    // appear on the object :-(
> +    jsval succeeded;
> +    ok = JS_DeleteUCProperty2(aCx, JSVAL_TO_OBJECT(aValue),
> +                              keyPathChars, keyPathLen,
> +                              &succeeded);

Nit: Arg wrapping can be better.

@@ +1088,5 @@
>    return NS_OK;
>  }
>  
> +nsresult
> +IDBObjectStore::EnsureKeyPathSerializationData(JSContext* aCx, PRUint64 aVal)

I don't think you should pass a value in here, just always use kTotallyRandomNumber.

Also, please assert that this happens on the main thread.

@@ +1919,2 @@
>  nsresult
>  AddHelper::ModifyValueForNewKey()

You've moved all of the guts elsewhere, just remove this function and call 'mObjectStore->ModifyValueForNewKey' from DoDatabaseWork.

::: dom/indexedDB/IDBObjectStore.h
@@ +177,5 @@
>                      PRUint8 aOptionalArgCount,
>                      nsIIDBRequest** _retval,
>                      bool aOverwrite);
>  
> +  nsresult EnsureKeyPathSerializationData(JSContext* aCx, PRUint64 aVal);

Nit: newline after this.

@@ +191,5 @@
>    PRBool mAutoIncrement;
>    PRUint32 mDatabaseId;
>    PRUint32 mStructuredCloneVersion;
>  
> +  // 

Nit: Empty comment needs some text!

::: xpcom/ds/nsCRT.cpp
@@ +163,5 @@
>    return 0;
>  }
>  
> +PRInt32 nsCRT::memmem(const char* haystack, PRUint32 haystackLen,
> +                      const char* needle, PRUint32 needleLen)

I don't think this is what we want. Standard memmem returns a char* pointing to the substring, or null if not found. Let's not deviate from that, especially since we want the substring pointer, not the number of characters we skipped.

@@ +176,5 @@
> +#else
> +  // No memmem means we need to roll our own.  This isn't really optimized
> +  // for performance ... if that becomes an issue we can take some inspiration
> +  // from the js string compare code in jsstr.cpp
> +  for (PRInt32 i = 0; i < haystackLen - needleLen; i++) {

Definitely needs some argument sanity checking (haystackLen >= needleLen, non-null pointers).

::: xpcom/ds/nsCRT.h
@@ +210,5 @@
>    static PRInt32 strncmp(const PRUnichar* s1, const PRUnichar* s2,
>                           PRUint32 aMaxLen);
>  
> +  static PRInt32 memmem(const char* haystack, PRUint32 haystackLen,
> +                        const char* needle, PRUint32 needleLen);

This will need a comment.

Comment 2

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

Attachment: Patch

Comment 3

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

(In reply to comment #1)
> 
> @@ +910,5 @@
> > +
> > +  union {
> > +    jsdouble d;
> > +    uint64_t u;
> > +  } pun;
> 
> Why is this union necessary? Especially since jsdouble and uint64_t are
> supposed to be the same size? (Probably asserted in jseng).

This is necessary to match the js engine's serialization code.

> @@ +1006,5 @@
> > +    NS_ENSURE_TRUE(ok, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> > + 
> > +    ok = JS_DefineUCProperty(aCx, JSVAL_TO_OBJECT(aValue), keyPathChars,
> > +                             keyPathLen, key, nsnull,
> > +                             nsnull, JSPROP_ENUMERATE);
> 
> Actually, let's not make it enumerable. No need.

The property must be enumerable to be structured cloned.

All other comments addressed.

Comment 4

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

Attachment: Patch

Right patch this time.

Though, if you want to review the jemalloc patch, I won't stop you ;-)

Comment 5

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

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

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

r=me with the following stuff fixed.

::: dom/indexedDB/IDBObjectStore.cpp
@@ +899,5 @@
> +
> +  // The minus 8 dangling off the end here is to account for the null entry
> +  // that terminates the buffer
> +  const PRUint32 keyPropLen = mKeyPathSerialization.nbytes() -
> +                              mKeyPathSerializationOffset - 8;

Nit: make that sizeof(PRUint64) to be less magic.

Also, we really don't need to calculate this every time, you could just have another member that gets set in EnsureKeyPathSerializationData.

@@ +902,5 @@
> +  const PRUint32 keyPropLen = mKeyPathSerialization.nbytes() -
> +                              mKeyPathSerializationOffset - 8;
> +
> +  char* location =
> +    const_cast<char*>(nsCRT::memmem((char*)aBuffer.data(),

Nit: make this const char* location, then use const_cast below on memcpy.

@@ +912,5 @@
> +
> +  // This is a duplicate of the js engine's byte munging here
> +  union {
> +    jsdouble d;
> +    uint64_t u;

Nit: Make this PRUint64 since we're in DOM

@@ +919,5 @@
> +  pun.d = SwapBytes(aKey.IntValue());
> +
> +  memcpy((char*)location + keyPropLen -
> +          sizeof(PRUint64), // We're overwriting the last 8 bytes
> +          &pun.u, sizeof(PRUint64));

Nit: sizeof(pun.u) since that's what we're copying.

@@ +988,5 @@
>  
> +  const jschar* keyPathChars =
> +    reinterpret_cast<const jschar*>(mKeyPath.get());
> +  const size_t keyPathLen = mKeyPath.Length();
> +  JSBool ok;

Initialize to false, so that you can...

@@ +1013,5 @@
> +      !IDBObjectStore::SerializeValue(aCx, aCloneBuffer, aValue)) {
> +    rv = NS_ERROR_DOM_DATA_CLONE_ERR;
> +  }
> +
> +  if (!mKeyPath.IsEmpty() && aKey.IsUnset() && ok) {

Make this test only 'ok'.

@@ +1081,5 @@
> +IDBObjectStore::EnsureKeyPathSerializationData(JSContext* aCx)
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread");
> +
> +  JSBool ok;

Nit: not needed outside the block below, move to where it's first used.

::: xpcom/ds/nsCRT.cpp
@@ +166,5 @@
> +const char* nsCRT::memmem(const char* haystack, PRUint32 haystackLen,
> +                          const char* needle, PRUint32 needleLen)
> +{
> +  NS_ASSERTION(haystack && needle && haystackLen && needleLen &&
> +               needleLen <= haystackLen, "Invalid inputs!");

Hm, it's my understanding that memmem will sanity check args for you, returning null if they don't make sense. This would mean that nsCRT::memmem would behave differently based on whether your system has memmem. I think instead of assertions we should just do the arg checking in all builds.

Comment 6

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

http://hg.mozilla.org/projects/build-system/rev/139cc50ebb22

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So, I really don't think it's a good idea to rely on chance here. While unlikely, this means that if the page gets unlucky we'll randomly corrupt its data.

What I think we should do is to look to see if the pattern appears more than once in the serialized data. If it does, have a slow-path which synchronously proxies a call to the main thread which deserializes the data, sets the property, and reserializes.

I really don't think we can ship this as-is.

(Note that this should be very easy to test too, just store an object like { foo: { keypath: <knownrandomvalue> } } )

Comment 8

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

I think we can rely on the property always being the last, and just searching backwards.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

That won't cover all cases. Consider:

o = { keypath: "", foo: { keypath: <magicnumber> } };
objectStore.put(o);

In this case the keypath property will still be the first property in the object and thus serialized first.

Additionally, once we support the full keypath syntax which allows the keypath to point to a member on a sub-object, there's even more ways to end up with the value we want to replace before other copies of the magic string.

Comment 10

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

(In reply to comment #9)
> That won't cover all cases. Consider:
> 
> o = { keypath: "", foo: { keypath: <magicnumber> } };
> objectStore.put(o);
> 
> In this case the keypath property will still be the first property in the
> object and thus serialized first.

Er, what?  The keypath property can't already exist, can it?

> Additionally, once we support the full keypath syntax which allows the
> keypath to point to a member on a sub-object, there's even more ways to end
> up with the value we want to replace before other copies of the magic string.

Yeah, that is harder :-/

Comment 11

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

http://hg.mozilla.org/mozilla-central/rev/139cc50ebb22