Closed Bug 667652 Opened 11 years ago Closed 11 years ago

Make the fix for Bug 663479 less hacky

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: khuey, Assigned: khuey)

References

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
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.
Attached patch PatchSplinter Review
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
Closed: 11 years ago
OS: Windows 7 → All
Hardware: x86 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Version: unspecified → Trunk
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.