Make the fix for Bug 663479 less hacky

RESOLVED FIXED in mozilla7

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: khuey, Assigned: khuey)

Tracking

Trunk
mozilla7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
Created attachment 542305 [details] [diff] [review]
Add a JS_FRIEND_API to get the current position in the buffer
Attachment #542305 - Flags: review?(jorendorff)
Created attachment 542307 [details] [diff] [review]
Change IDB code to be less hacky
Attachment #542307 - Flags: review?(bent.mozilla)
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 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?
(In reply to comment #4)
> namespace, pretty please with a cherry on top?

That's not how the public/friend api works.
(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 on attachment 542305 [details] [diff] [review]
Add a JS_FRIEND_API to get the current position in the buffer

Sure, ok.
Attachment #542305 - Flags: review?(jorendorff) → review+
Shouldn't you be using writeDouble rather than writeBytes?
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.
Created attachment 542836 [details] [diff] [review]
Patch

Review comments addressed.
Attachment #542307 - Attachment is obsolete: true
Attachment #542836 - Flags: review?(bent.mozilla)
Attachment #542307 - Flags: review?(bent.mozilla)
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
  };
Attachment #542836 - Flags: review?(bent.mozilla) → review+
http://hg.mozilla.org/mozilla-central/rev/594b9b4cb1df
http://hg.mozilla.org/mozilla-central/rev/b2a1b2fd6d23
Status: NEW → RESOLVED
Last Resolved: 6 years ago
OS: Windows 7 → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Version: unspecified → Trunk
You need to log in before you can comment on or make changes to this bug.