Closed Bug 1184557 Opened 4 years ago Closed 4 years ago

StructuredCloneHelper class for window to window postMessage

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached patch clone2.patch (obsolete) — Splinter Review
No description provided.
Attachment #8634722 - Flags: review?(bugs)
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?
Attachment #8634722 - Flags: review?(bugs) → review-
Attached patch clone2.patch (obsolete) — Splinter Review
Attachment #8634722 - Attachment is obsolete: true
Attachment #8635770 - Flags: review?(bugs)
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.)
Attachment #8635770 - Flags: review?(bugs) → review-
Depends on: 1185360
Attached patch clone2.patch (obsolete) — Splinter Review
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.
Attachment #8635770 - Attachment is obsolete: true
Attachment #8635808 - Flags: review?(bugs)
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.
Attachment #8635808 - Flags: review?(bugs) → review-
Attached patch clone2.patch (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=981d05f19550
Attachment #8635808 - Attachment is obsolete: true
Attachment #8635823 - Flags: review?(bugs)
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.
Attachment #8635823 - Flags: review?(bugs)
Depends on: 1185381
Attached patch clone2.patchSplinter Review
Here we use the new FileList cloning in StructuredCloneHelper.
Attachment #8635823 - Attachment is obsolete: true
Attachment #8635915 - Flags: review?(bugs)
Blocks: 1179396
Duplicate of this bug: 1179396
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?
Attachment #8635915 - Flags: review?(bugs) → review+
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
Attached patch id.patchSplinter Review
We must free the mBuffer in the inheriting class.
Flags: needinfo?(amarchesini)
Attachment #8637745 - Flags: review?(bugs)
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.
Attachment #8637745 - Flags: review?(bugs) → review+
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
https://hg.mozilla.org/mozilla-central/rev/7cf1d9d80be4
https://hg.mozilla.org/mozilla-central/rev/4febe3d4dfa6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.