Closed Bug 1189389 Opened 5 years ago Closed 5 years ago

nsIStructuredCloneContainer should use StructuredCloneHelper

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 2 obsolete files)

Attached patch nsi.patch (obsolete) — Splinter Review
No description provided.
Attachment #8641113 - Flags: review?(bugs)
Blocks: 972973
Comment on attachment 8641113 [details] [diff] [review]
nsi.patch

> nsStructuredCloneContainer::InitFromJSVal(JS::Handle<JS::Value> aData,
>                                           JSContext* aCx)
> {
>   NS_ENSURE_STATE(!mData);
> 
>-  uint64_t* jsBytes = nullptr;
>-  bool success = JS_WriteStructuredClone(aCx, aData, &jsBytes, &mSize,
>-                                           nullptr, nullptr,
>-                                           JS::UndefinedHandleValue);
>-  NS_ENSURE_STATE(success);
>-  NS_ENSURE_STATE(jsBytes);
>+  StructuredCloneHelper helper(StructuredCloneHelper::CloningNotSupported,
>+                               StructuredCloneHelper::TransferringNotSupported);
This doesn't look right. We definitely should support cloning here. And the current code should use the default callbacks anyway, which do have support for cloning.

>+nsStructuredCloneContainer::DeserializeToVariant(JSContext* aCx,
>+                                                 nsIVariant** aData)
> {
>   NS_ENSURE_STATE(mData);
>   NS_ENSURE_ARG_POINTER(aData);
>   *aData = nullptr;
> 
>   // Deserialize to a JS::Value.
>   JS::Rooted<JS::Value> jsStateObj(aCx);
>-  bool hasTransferable = false;
>-  bool success = JS_ReadStructuredClone(aCx, mData, mSize, mVersion,
>-                                        &jsStateObj, nullptr, nullptr) &&
>-                 JS_StructuredCloneHasTransferables(mData, mSize,
>-                                                    &hasTransferable);
>-  // We want to be sure that mData doesn't contain transferable objects
>-  MOZ_ASSERT(!hasTransferable);
>-  NS_ENSURE_STATE(success && !hasTransferable);
>+
>+  StructuredCloneHelper helper(StructuredCloneHelper::CloningNotSupported,
>+                               StructuredCloneHelper::TransferringNotSupported);
ditto

>+
>+  ErrorResult rv;
>+  helper.ReadFromBuffer(nullptr, aCx, mData, mSize, mVersion, &jsStateObj, rv);
>+  if (NS_WARN_IF(rv.Failed())) {
>+    return rv.StealNSResult();
>+  }
> 
>   // Now wrap the JS::Value as an nsIVariant.
>   nsCOMPtr<nsIVariant> varStateObj;
>   nsCOMPtr<nsIXPConnect> xpconnect = do_GetService(nsIXPConnect::GetCID());
>   NS_ENSURE_STATE(xpconnect);
>   xpconnect->JSValToVariant(aCx, jsStateObj, getter_AddRefs(varStateObj));
>   NS_ENSURE_STATE(varStateObj);
> 
>-  NS_ADDREF(*aData = varStateObj);
>+  varStateObj.forget(aData);
>   return NS_OK;


>+NS_IMETHODIMP
> nsStructuredCloneContainer::GetDataAsBase64(nsAString &aOut)
> {
>   NS_ENSURE_STATE(mData);
>   aOut.Truncate();
> 
>   nsAutoCString binaryData(reinterpret_cast<char*>(mData), mSize);
>   nsAutoCString base64Data;
>   nsresult rv = Base64Encode(binaryData, base64Data);
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  aOut.Assign(NS_ConvertASCIItoUTF16(base64Data));
>+  CopyASCIItoUTF16(base64Data, aOut);
>   return NS_OK;
> }
Oh, hmm, is this just broken API. I can't see any sane way to deal with structure clone in base64 format in case DOM objects are also cloned.
I think we need to somehow detect here whether non-JS objects were cloned and throw exception. So base64 would be available to JS only stuff.
Does that make sense to you?
Attachment #8641113 - Flags: review?(bugs) → review-
> This doesn't look right. We definitely should support cloning here. And the
> current code should use the default callbacks anyway, which do have support
> for cloning.

Currently we don't support cloning of DOM objects. If we support it, how to keep blobs alive if the caller takes the base64 buffer and stores somewhere, how we actually do in many JS uses of this API?

> I think we need to somehow detect here whether non-JS objects were cloned
> and throw exception. So base64 would be available to JS only stuff.
> Does that make sense to you?

by default we support the cloning of JS obects. When we enable the 'cloning' we support DOM stuff too (blobs, fileList, etc).
Flags: needinfo?(bugs)
This was discussed on IRC. We do support cloning of certain objects because we
use the default cloning set in nsJSEnvironment.
And push/replaceState should support full cloning.
Flags: needinfo?(bugs)
Attached patch nsi.patch (obsolete) — Splinter Review
Attachment #8641113 - Attachment is obsolete: true
Attachment #8646442 - Flags: review?(bugs)
Comment on attachment 8646442 [details] [diff] [review]
nsi.patch

> nsStructuredCloneContainer::GetDataAsBase64(nsAString &aOut)
> {
>-  NS_ENSURE_STATE(mData);
>   aOut.Truncate();
> 
>-  nsAutoCString binaryData(reinterpret_cast<char*>(mData), mSize);
>+  if (mState == eNotInitialized) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  uint64_t* data;
>+  size_t size;
>+
>+  if (mState == eInitializedFromJSVal) {
>+    if (BlobImpls().Length()) {
>+      return NS_ERROR_FAILURE;
>+    }
Why do we check only BlobImpls? Why not also ClonedImages etc.?
Attachment #8646442 - Flags: review?(bugs) → review-
Attached patch nsi.patchSplinter Review
Attachment #8646442 - Attachment is obsolete: true
Attachment #8652536 - Flags: review?(bugs)
> Why do we check only BlobImpls? Why not also ClonedImages etc.?

Because when I submitted the patch clonedImages didn't exist :)
Attachment #8652536 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f7893bbf4cb3809fe88a78587e9e23a7a400ecd4
Bug 1189389 - nsIStructuredCloneContainer should use StructuredCloneHelper, r=smaug
https://hg.mozilla.org/mozilla-central/rev/f7893bbf4cb3
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.