1198795 - ipc/StructuredCloneUtils should be merged with StructuredCloneHelper

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: ipc1.patch

The reason why this patch changes from const StructuredClone* to StructuredCloneHelper (removing 'const') is because the Read needs to set the mParent internally.
I'll rename StructuredCloneUtils to StructuredCloneIPCHelper if you agree with the current patch.

Comment 2

[Andrea Marchesini [:baku]]

I would probably rename "DifferentProcessDifferenThread" to DifferentProcess.

Comment 3

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

Comment on attachment 8653600 [details] [diff] [review]
ipc1.patch



>+  StructuredCloneIPCHelper helper;
IPC helper is of course used also with in-process message managers, but I guess the naming still makes sense given
that in-process MMs should behave as similar to cross-process MMs as possible


> nsSameProcessAsyncMessageBase::nsSameProcessAsyncMessageBase(JSContext* aCx,
>                                                              const nsAString& aMessage,
>-                                                             const StructuredCloneData& aData,
>+                                                             StructuredCloneIPCHelper& aHelper,
>                                                              JS::Handle<JSObject*> aCpows,
>                                                              nsIPrincipal* aPrincipal)
>   : mRuntime(js::GetRuntime(aCx)),
>     mMessage(aMessage),
>     mCpows(aCx, aCpows),
>     mPrincipal(aPrincipal)
> {
>-  if (aData.mDataLength && !mData.copy(aData.mData, aData.mDataLength)) {
>+  if (mHelper.Copy(aHelper)) {
Not about this bug, but we really should figure out a way to reuse the data here.
The OOM here is one of the top crashers.


>+StructuredCloneIPCHelper::Read(JSContext* aCx,
>+                               JS::MutableHandle<JS::Value> aValue,
>+                               ErrorResult &aRv)
> {
>-  return aBuffer.write(aCx, aSource, &gCallbacks, &aClosure);
>+  MOZ_ASSERT(mData);
>+
>+  nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(aCx));
nsIGlobalObject* global


>   SendRunnable(WorkerPrivate* aWorkerPrivate, Proxy* aProxy,
>                const nsAString& aStringBody)
>   : WorkerThreadProxySyncRunnable(aWorkerPrivate, aProxy)
>-  , StructuredCloneHelper(CloningSupported, TransferringNotSupported)
>+  , StructuredCloneHelper(CloningSupported, TransferringNotSupported,
>+                          SameProcessSameThread)
...
>   EventRunnable(Proxy* aProxy, bool aUploadEvent, const nsString& aType,
>                 bool aLengthComputable, uint64_t aLoaded, uint64_t aTotal)
>   : MainThreadProxyRunnable(aProxy->mWorkerPrivate, aProxy),
>-    StructuredCloneHelper(CloningSupported, TransferringNotSupported),
>+    StructuredCloneHelper(CloningSupported, TransferringNotSupported,
>+                          SameProcessSameThread),
...

>   EventRunnable(Proxy* aProxy, bool aUploadEvent, const nsString& aType)
>   : MainThreadProxyRunnable(aProxy->mWorkerPrivate, aProxy),
>-    StructuredCloneHelper(CloningSupported, TransferringNotSupported),
>+    StructuredCloneHelper(CloningSupported, TransferringNotSupported,
>+                          SameProcessSameThread),
Are these really SameProcessSameThread ? Should these use SameProcessDifferentThread.

I think we need an assertion somewhere to make sure that if SameProcessSameThread is used, write and read must happen on the same thread.

"DifferentProcessDifferenThread" to DifferentProcess sounds ok.




You could just upload another patch which is on top of this one, given that the changes should be rather small.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: ipc2.patch

Comment 5

[Andrea Marchesini [:baku]]

Attachment: ipc3.patch

This 3rd patch mainly implements the ::Write function.
What I don't like is that fact that I have to allocate the buffer again in order to use normal free/malloc instead js_free.
I could have a enum like:

enum {
  eDataNotOwned,
  eDataJSOwned,
  eDataOwned
} mDataOwned;

to avoid the double allocation. What do you think about it?

Comment 6

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

Comment on attachment 8655515 [details] [diff] [review]
ipc3.patch

I think we should try to avoid extra memory allocations in this code whenever possible. SC is causing OOMs already.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: ipc3.patch

Same patch but with a better allocation policy.

Comment 8

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=677e2a5cee3c
green on try.

Comment 9

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

Comment on attachment 8655668 [details] [diff] [review]
ipc3.patch

This seems to leak if one uses Adopt, since mDataOwned is eNone by default, and Adopt doesn't update it.
I would expect Adopt to take some param telling which way the data should be released.

Comment 10

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

Hmm, or is the name "Adopt" just wrong here, since it hints moving ownership of the data, but that doesn't seem to happen here. Maybe UseData(), or maybe even UseExternalData() ?

Comment 11

[Andrea Marchesini [:baku]]

It's just the name. UseExternalData sounds better than 'Adopt'. The ownership is correct to be eNone.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: id.patch

Comment 13

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

Comment on attachment 8655668 [details] [diff] [review]
ipc3.patch

Ok, with that r+ and assert in UseExternalData that mDataOwned is eNone

Comment 14

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

Comment on attachment 8656022 [details] [diff] [review]
id.patch

So, add the assertion to UseExternalData, as I just said in previous comment
(and gave r+ already.)

Comment 15

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 16

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed6830f48c58

Comment 17

[Carsten Book [:Tomcat]]

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