1189389 - nsIStructuredCloneContainer should use StructuredCloneHelper

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Attachment: nsi.patch

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 2

[Andrea Marchesini [:baku]]

> 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).

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: nsi.patch

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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.?

Comment 6

[Andrea Marchesini [:baku]]

Attachment: nsi.patch

Comment 7

[Andrea Marchesini [:baku]]

> Why do we check only BlobImpls? Why not also ClonedImages etc.?

Because when I submitted the patch clonedImages didn't exist :)

Comment 8

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f7893bbf4cb3809fe88a78587e9e23a7a400ecd4
Bug 1189389 - nsIStructuredCloneContainer should use StructuredCloneHelper, r=smaug

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f7893bbf4cb3