972973 - Meta-Bug: Unify the use of StructureCloneBuffer in the DOM code (MessagePort, BroadcastChannel, Workers, window, DataStore, IDB, etc)

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

Currently we have 6 implementations of the callbacks:

1. nsGlobalWindow.cpp
2. MessagePort.cpp
3. WorkerPrivate - for workers
4. WorkerPrivate - for main thread
5. WorkerPrivate - for chrome workers
6. WorkerPrivate - for chrome main thread window

Would be nice to have 1 single helper class. 
This is needed for MessagePort, AsyncMessagePort and Broadcast API.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: unify.patch

Comment 2

[Andrea Marchesini [:baku]]

Attachment: unify.patch

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8377597 [details] [diff] [review]
unify.patch

Review of attachment 8377597 [details] [diff] [review]:
-----------------------------------------------------------------

I'm having a hard time with this patch... It doesn't unify things as much as I was hoping (e.g. NS_DOMReadStructuredClone in nsJSEnvironment.cpp) and it doesn't seem to make the calling code all that much better...

I also think we should rename StructuredCloneHelper to MessagePortStructuredCloneHelper or something. It's very specific to same-process message ports.

::: dom/base/StructuredCloneHelper.cpp
@@ +282,5 @@
> +    MOZ_ASSERT(dataArray.isObject());
> +
> +    // Construct the ImageData.
> +    nsRefPtr<ImageData> imageData = new ImageData(width, height,
> +                                                  dataArray.toObject());

Can't the imagedata stuff be unified with the main thread callbacks now?

@@ +383,5 @@
> +    return clone;
> +  }
> +
> +  return StructuredCloneHelperWorkerCallback::Read(aCx, aReader, aTag, aData,
> +                                                   aClosure);

Wait, what is this doing? Calling a worker callback from inside the chrome window callback seems wrong. Also, why do we need a separate callback for chrome windows?

@@ +418,5 @@
> +                             JS::Handle<JS::Value> aMessage,
> +                             JS::Handle<JS::Value> aTransfer)
> +{
> +  JSStructuredCloneCallbacks* callbacks;
> +  switch (aTarget) {

This needs a default branch with an assertion.

@@ +476,5 @@
> +bool
> +StructuredCloneHelper::StoreISupports(nsISupports* aSupports)
> +{
> +  mSupportsArray.AppendElement(aSupports);
> +  return true;

This shouldn't return a value.

@@ +483,5 @@
> +void
> +StructuredCloneHelper::ReleaseISupports()
> +{
> +  nsTArray<nsCOMPtr<nsISupports>> supportsArray;
> +  supportsArray.SwapElements(mSupportsArray);

Just do mSupportsArray.Clear().

@@ +489,5 @@
> +
> +void
> +StructuredCloneHelper::Swap(StructuredCloneHelper& aDestination)
> +{
> +  aDestination.mBuffer = Move(mBuffer);

I'm not sure you can do a move here... Ask khuey.

@@ +493,5 @@
> +  aDestination.mBuffer = Move(mBuffer);
> +  mSupportsArray.SwapElements(aDestination.mSupportsArray);
> +  aDestination.mWindow = mWindow;
> +  aDestination.mSubsumes = mSubsumes;
> +  aDestination.mImmutable = mImmutable;

You're not swapping these three, you're just adopting. Either the name of this method is wrong or this is a bug.

::: dom/base/StructuredCloneHelper.h
@@ +26,5 @@
> +    TargetChromeWorker = TargetWorker,
> +  };
> +
> +  StructuredCloneHelper();
> +  ~StructuredCloneHelper();

You should disable copy constructor, operator= also, right?

@@ +33,5 @@
> +  {
> +    mSubsumes = aSubsumes;
> +  }
> +
> +  void SetImmutable(bool aImmutable)

Let's remove all the immutable stuff. There's no need to send mutable blobs through a MessagePort, even between windows.

@@ +42,5 @@
> +  bool Write(Target aTarget, JSContext* aCx, JS::Handle<JS::Value> aMessage,
> +             JS::Handle<JS::Value> aTransfer);
> +
> +  bool Read(Target aTarget, nsPIDOMWindow* aWindow, JSContext* aCx,
> +            JS::MutableHandle<JS::Value> aMessage);

Read and Write should return an nsresult I think.

@@ +50,5 @@
> +  uint64_t* Data() const;
> +
> +  void ReleaseISupports();
> +
> +  void Swap(StructuredCloneHelper& aDestination);

khuey should look over this to make sure the move stuff works correctly.

@@ +54,5 @@
> +  void Swap(StructuredCloneHelper& aDestination);
> +
> +  // CC methods
> +  void Unlink();
> +  void Traverse(nsCycleCollectionTraversalCallback &cb);

You should pass the callback into unlink also so that the unlink method can use the cc macros to clear mSupportsArray/mWindow. That will make cc logging/debugging much easier.

Comment 4

[Andrea Marchesini [:baku]]

When bug 911972 and bug 1047483 will land, this will be much much easier to do.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 1 - StructuredCloneHelper and Console API

This is more a feedback request. Basically the idea is to have a StructuredCloneHelper class as we discussed on IRC. Here the first patch and the first use of it for the Console API

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 2: StructuredCloneFullHelper and window.postMessage

This second patch introduces a StructuredCloneFullHelper and it uses it for window.postMessage. In the constructor of the helper class we can define what we want to transfer/clone. Maybe we can have a default value but for now I prefer to specify all the things. The default value can be implemented in a separate patch.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 3: BroadcastChannel and DataStore

Comment 8

[Andrea Marchesini [:baku]]

Before continuing I really appreciate a feedback. The next step would be to migrate MessagePort and Workers.

Comment 9

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

Comment on attachment 8634188 [details] [diff] [review]
part 1 - StructuredCloneHelper and Console API


>+class StructuredCloneHelper
>+{
>+public:
>+  bool Write(JSContext* aCx,
>+             JS::Handle<JS::Value> aValue);
>+
>+  bool Write(JSContext* aCx,
>+             JS::Handle<JS::Value> aValue,
>+             JS::Handle<JS::Value> aTransfer);
>+
>+  bool Read(JSContext* aCx,
>+            JS::MutableHandle<JS::Value> aValue);
So I assume these shouldn't be reimplemented? Could these be private then or something? Or marked final?



>+
>+  // These methods should be implemented in order to clone data.
>+  // Read more documentation in js/public/StructuredClone.h.
>+
>+  virtual JSObject* ReadCallback(JSContext* aCx,
>+                                 JSStructuredCloneReader* aReader,
>+                                 uint32_t aTag,
>+                                 uint32_t aIndex) = 0;
>+
>+  virtual bool WriteCallback(JSContext* aCx,
>+                             JSStructuredCloneWriter* aWriter,
>+                             JS::Handle<JSObject*> aObj) = 0;
So if these two methods are the ones inheriting class needs to implement, perhaps move these to be the first methods in the class.

>+
>+  // If these 3 methods are not implement, transfering objects will not be
>+  // allowed.
>+
>+  virtual bool
>+  ReadTransferCallback(JSContext* aCx,
>+                       JSStructuredCloneReader* aReader,
>+                       uint32_t aTag,
>+                       void* aContent,
>+                       uint64_t aExtraData,
>+                       JS::MutableHandleObject aReturnObject);
>+
>+  virtual bool
>+  WriteTransferCallback(JSContext* aCx,
>+                        JS::Handle<JSObject*> aObj,
>+                        // Output:
>+                        uint32_t* aTag,
>+                        JS::TransferableOwnership* aOwnership,
>+                        void** aContent,
>+                        uint64_t* aExtraData);
>+
>+  virtual void
>+  FreeTransferCallback(uint32_t aTag,
>+                       JS::TransferableOwnership aOwnership,
>+                       void* aContent,
>+                       uint64_t aExtraData);
and I guess then these should be next to those 2 methods.

Comment 10

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

Comment on attachment 8634194 [details] [diff] [review]
part 2: StructuredCloneFullHelper and window.postMessage

>+class StructuredCloneFullHelper : public StructuredCloneHelper
>+{
>+public:
>+  enum StructuredCloneFullHelperFlags {
>+    eNothing = 1 << 0,
>+    eBlob = 1 << 1,
>+    eFileList = 1 << 2,
>+    eMessagePort = 1 << 3,
>+  };
>+
>+  // aFlags is a bitmap of StructuredCloneFullHelperFlags.
>+  StructuredCloneFullHelper(uint32_t aFlags);
So this setup is guaranteed to go wrong over time.
We need to have all flags set by default, and explicitly disable those which aren't supported in some case.


Naming is still hard.
What is StructuredCloneFullHelper, and when should it be used, and what is StructuredCloneHelper and when should it be used?
Need better names, or at least some good documentation.

Comment 11

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

Comment on attachment 8634195 [details] [diff] [review]
part 3: BroadcastChannel and DataStore

>+class BroadcastChannelMessage final : public StructuredCloneFullHelper

So it is not clear why *Full* is needed here.
(because I've forgotten what you said on IRC when Full should be used and when the normal Helper)

>-    if (!mObjBuffer.write(aCx, aObj)) {
>+    if (!Write(false, aCx, aObj)) {
>       JS_ClearPendingException(aCx);
>       mRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
>     }
It would be really great if exception handling could be somehow integrated to the helper stuff.
Perhaps some enum param to Write/Read? Otherwise it is pretty much guaranteed that exception handling goes wrong at some point.

Comment 12

[Andrea Marchesini [:baku]]

After discussing this bug with smaug, we decided to split this issue in small pieces and land them separately.
This bug is going to be a meta-bug.

Comment 13

[Andrea Marchesini [:baku]]

I would consider this as fixed.