ipc/StructuredCloneUtils should be merged with StructuredCloneHelper

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

No description provided.
Posted patch ipc1.patch (obsolete) — Splinter Review
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.
Attachment #8653600 - Flags: review?(bugs)
I would probably rename "DifferentProcessDifferenThread" to DifferentProcess.
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.
Attachment #8653600 - Flags: review?(bugs) → review-
Posted patch ipc2.patch (obsolete) — Splinter Review
Attachment #8654804 - Flags: review?(bugs)
Attachment #8654804 - Flags: review?(bugs) → review+
Posted patch ipc3.patch (obsolete) — Splinter Review
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?
Attachment #8655515 - Flags: review?(bugs)
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.
Attachment #8655515 - Flags: review?(bugs)
Posted patch ipc3.patch (obsolete) — Splinter Review
Same patch but with a better allocation policy.
Attachment #8655515 - Attachment is obsolete: true
Attachment #8655668 - Flags: review?(bugs)
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.
Attachment #8655668 - Flags: review?(bugs) → review-
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() ?
It's just the name. UseExternalData sounds better than 'Adopt'. The ownership is correct to be eNone.
Posted patch id.patch (obsolete) — Splinter Review
Attachment #8656022 - Flags: review?(bugs)
Comment on attachment 8655668 [details] [diff] [review]
ipc3.patch

Ok, with that r+ and assert in UseExternalData that mDataOwned is eNone
Attachment #8655668 - Flags: review- → review+
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.)
Attachment #8656022 - Flags: review?(bugs)
Posted patch patchSplinter Review
Attachment #8653600 - Attachment is obsolete: true
Attachment #8654804 - Attachment is obsolete: true
Attachment #8655668 - Attachment is obsolete: true
Attachment #8656022 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ed6830f48c58
Status: NEW → RESOLVED
Closed: 4 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.