Comment on attachment 8634722 [details] [diff] [review] clone2.patch This needs mostly just better documentation so that it is clear for whoever uses this new API how it actually works. >+ >+ >+bool >+StructuredCloneHelper::WriteTransferCallback(JSContext* aCx, extra newline before the method >+ enum StructuredCloneHelperFlags { >+ eAll = 0, >+ >+ // Disable the cloning of blobs. If a blob is part of the transfered value, >+ // an exception will be thrown. >+ eBlobNotSupported = 1 << 0, Wait, what is this about. cloning or transferring? >+ >+ // FileList objects cloning is not supported. >+ eFileListNotSupported = 1 << 1, Why is the comment about eFileListNotSupported different to eBlobNotSupported? They are both among the cloneable types in the spec, so I'd except the same behavior. >+ MessagePortIdentifier* NewPortIdentifier(uint64_t* aPosition); This could use some comment. >+ nsTArray<nsCOMPtr<nsISupports>> mSupportsArray; >+ nsISupports* mParent; What is the lifetime management for this? You should add also some annotation to this, see the bugs bug 1114683 depend on. >+ bool mSubsumes; What subsumes what? >+ >+ // This hashtable contains the transferred ports - used to avoid duplicates. >+ nsTArray<nsRefPtr<MessagePortBase>> mTransferredPorts; >+ >+ // This array is populated when the ports are cloned. >+ nsTArray<nsRefPtr<MessagePortBase>> mClonedPorts; So ports aren't cloned, they are transferred, so what is cloned ports about? >+ >+ nsTArray<MessagePortIdentifier> mPortIdentifiers; This really needs some documentation. What are port identifiers? How are they related to transferred ports or cloned ports?

Comment on attachment 8635770 [details] [diff] [review] clone2.patch Ah, this is using also the eNothingSupported. So I think this should be reviewed after we have a setup without eNothingSupported > PostMessageEvent::Write(JSContext* aCx, JS::Handle<JS::Value> aMessage, > JS::Handle<JS::Value> aTransfer, bool aSubsumes, > nsPIDOMWindow* aWindow) > { > // We *must* clone the data here, or the JS::Value could be modified > // by script >- StructuredCloneInfo scInfo; >- scInfo.event = this; >- scInfo.window = aWindow; >- scInfo.subsumes = aSubsumes; >+ if (!aSubsumes) { >+ OverwriteFlags(StructuredCloneHelperFlags::eNothingSupported); >+ } Not related to this bug, but I couldn't find anything like this 'subsumes' behavior from the spec. Looks like it was added in Bug 673742. Could you file a bug to get rid of it. It is against the spec. >+StructuredCloneHelper::ReadCallback(JSContext* aCx, >+ JSStructuredCloneReader* aReader, >+ uint32_t aTag, >+ uint32_t aIndex) ... >+ return NS_DOMReadStructuredClone(aCx, aReader, aTag, aIndex, nullptr); I assume NS_DOMReadStructuredClone will be merged to this method. That is the current issue that we have code dealing with SC spread in so many places >+StructuredCloneHelper::WriteCallback(JSContext* aCx, >+ JSStructuredCloneWriter* aWriter, >+ JS::Handle<JSObject*> aObj) ... >+ return NS_DOMWriteStructuredClone(aCx, aWriter, aObj, nullptr); ditto >+class StructuredCloneHelper : public StructuredCloneHelperInternal >+{ >+public: >+ enum StructuredCloneHelperFlags { >+ eAll = 0, >+ >+ // Disable the cloning of blobs. If a blob is part of the cloning value, >+ // an exception will be thrown. >+ eBlobNotSupported = 1 << 0, >+ >+ // Disable the cloning of FileLists. If a FileList is part of the cloning >+ // value, an exception will be thrown. >+ eFileListNotSupported = 1 << 1, >+ >+ // MessagePort can just be transfered. Using this flag we do not support >+ // the transfering. >+ eMessagePortNotSupported = 1 << 2, >+ >+ // If nothing is supported, use this value. >+ eNothingSupported = UINT32_MAX - 1 so this is no no. >+ >+ // This raw pointer is set and unset into the ::Read(). It's always null >+ // outside that method. For this reason it's a raw pointe. pointer >+ nsISupports* mParent; This should still use one of those annotations ehsan has been adding in the bug I linked. >+ >+ // This hashtable contains the list of transferring ports - used to avoid >+ // duplicates into ::Write(). It's an empty array outside this method. >+ nsTArray<nsRefPtr<MessagePortBase>> mTransferringPort; >+ >+ // This array is populated when the ports are cloned. >+ nsTArray<nsRefPtr<MessagePortBase>> mTransferredPorts; The comments don't quite differentiate these two arrays. mTransferringPort contains ports while doing write (transferring and mapping transferred object to the objects in the clone), right? And mTransferredPorts contain the ports once we've finished write, right? >+ // This array contains the identifiers of the MessagePorts. Based on these we >+ // are able to reconnect the new transferred ports with the other >+ // MessageChannel ports. >+ nsTArray<MessagePortIdentifier> mPortIdentifiers; identifiers of which ports? The ones in mTransferredPorts list? One might ask why you can't get the same information out from mTransferredPorts? Sorry that I'm nitpicking with this StructuredClone handling, but it has been such a mess for too long, so would be great to have really nice and easy to understand setup. (and I think your setup is getting us there.)

1. eNothingSupported is not needed anymore. 2. mTransferredPorts is generated from mPortIdentifiers when ::Read() is called. From the identifiers we create a new port in the reading thread/context.

Comment on attachment 8635808 [details] [diff] [review] clone2.patch So either we need to keep subsumes stuff in this patch, or fix the other bug which removes it. Filelist handling is definitely not going to work as such, since it doesn't do proper cloning. Sorry that this code is so messy. Not sure why this has happened.

Comment on attachment 8635823 [details] [diff] [review] clone2.patch Sorry, I didn't see your review comments. Yes, you are right, FileList cloning is broken. I'll file a separate bug for that and I'll make it a blocker for this one.

Here we use the new FileList cloning in StructuredCloneHelper.

Comment on attachment 8635915 [details] [diff] [review] clone2.patch >+bool >+StructuredCloneHelper::WriteCallback(JSContext* aCx, >+ JSStructuredCloneWriter* aWriter, >+ JS::Handle<JSObject*> aObj) ... >+ StoreISupports(fileListClonedData); >+ } >+ } >+ >+ return NS_DOMWriteStructuredClone(aCx, aWriter, aObj, nullptr); >+} Ah, so we just use the same callback what nsJSEnvironment.cpp set to runtime by default. Some other bug will merge NS_DOMRead/WriteStructuredClone to to SCHelper, right? It is currently rather error prone that we have some SC read/write methods, and then also NS_DOMRead/WriteStructuredClone. >+ // This hashtable contains the ports while doing write (transferring and >+ // mapping transferred objects to the objects in the clone). It's an empty >+ // array outside this method. "outside this method" - to which method does 'this' refer to?

url: https://hg.mozilla.org/integration/mozilla-inbound/rev/f6d29009ae0a599059d750ced11a8654a39fe224 changeset: f6d29009ae0a599059d750ced11a8654a39fe224 user: Andrea Marchesini <amarchesini@mozilla.com> date: Wed Jul 22 19:37:18 2015 +0100 description: Bug 1184557 - StructuredCloneHelper class for window to window postMessage, r=smaug

> Some other bug will merge NS_DOMRead/WriteStructuredClone to to SCHelper, > right? Correct. At the end of the unification. Worker is still missing and some others here and there in the code. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c2ee02a61ce

Had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9be3d57c2e15 for wpt crashes: https://treeherder.mozilla.org/logviewer.html#?job_id=12019198&repo=mozilla-inbound

We must free the mBuffer in the inheriting class.

Comment on attachment 8637745 [details] [diff] [review] id.patch But mBuffer is nsAutoPtr. Why do we need to call it explicitly? And does it matter whether Shutdown is called on worker or main thread (thinking about ConsoleRunnable here)?

Because nsAutoPtr works in the DTOR of StructuredCloneHelperInternal, and that point we don't use the override methods but the base method of the base class. And we crash freeing the transferring objects because of a MOZ_CRASH. In release build we leak.

> And does it matter whether Shutdown is called on worker or main thread > (thinking about ConsoleRunnable here)? Doesn't matter. MessagePorts can be freed in any thread because this is done using PBackground.

Comment on attachment 8637745 [details] [diff] [review] id.patch I see. Rather unfortunate. Please add some comment explaining why Shutdown must be called on inheriting class.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6326eaf87f86 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/69542d63fcc8

Backed out for completely blowing up debug tests. Please verify that your patches are green on Try to avoid inconveniencing all other developers with tree closures. https://treeherder.mozilla.org/logviewer.html#?job_id=12052976&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/6793c7bd6ea4

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/7cf1d9d80be4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4febe3d4dfa6

https://hg.mozilla.org/mozilla-central/rev/7cf1d9d80be4 https://hg.mozilla.org/mozilla-central/rev/4febe3d4dfa6