1184557 - StructuredCloneHelper class for window to window postMessage

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Attachment: clone2.patch

Comment 1

[Andrea Marchesini [:baku]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea56650b11f7

Comment 2

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

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 3

[Andrea Marchesini [:baku]]

Attachment: clone2.patch

Comment 4

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

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

Comment 5

[Andrea Marchesini [:baku]]

Attachment: clone2.patch

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 6

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

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 7

[Andrea Marchesini [:baku]]

Attachment: clone2.patch

https://treeherder.mozilla.org/#/jobs?repo=try&revision=981d05f19550

Comment 8

[Andrea Marchesini [:baku]]

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.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: clone2.patch

Here we use the new FileList cloning in StructuredCloneHelper.

Comment 11

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

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?

Comment 12

[Andrea Marchesini [:baku]]

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

Comment 13

[Andrea Marchesini [:baku]]

> 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

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Andrea Marchesini [:baku]]

Attachment: id.patch

We must free the mBuffer in the inheriting class.

Comment 16

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

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

Comment 17

[Andrea Marchesini [:baku]]

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.

Comment 18

[Andrea Marchesini [:baku]]

> 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 19

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

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.

Comment 20

[Andrea Marchesini [:baku]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Andrea Marchesini [:baku]]

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

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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