Closed Bug 972973 Opened 6 years ago Closed 4 years ago

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

Categories

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

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: baku, Assigned: baku)

References

(Depends on 1 open bug)

Details

Attachments

(5 obsolete files)

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.
Attached patch unify.patch (obsolete) — Splinter Review
Attachment #8376394 - Flags: feedback?(bent.mozilla)
Attached patch unify.patch (obsolete) — Splinter Review
Attachment #8376394 - Attachment is obsolete: true
Attachment #8376394 - Flags: feedback?(bent.mozilla)
Attachment #8377597 - Flags: review?(bent.mozilla)
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.
Attachment #8377597 - Flags: review?(bent.mozilla)
When bug 911972 and bug 1047483 will land, this will be much much easier to do.
Depends on: 911972, 1047483
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
Attachment #8377597 - Attachment is obsolete: true
Attachment #8634188 - Flags: review?(bugs)
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.
Attachment #8634194 - Flags: review?(bugs)
Attachment #8634195 - Flags: review?(bugs)
Before continuing I really appreciate a feedback. The next step would be to migrate MessagePort and Workers.
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.
Attachment #8634188 - Flags: review?(bugs) → feedback+
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.
Attachment #8634194 - Flags: review?(bugs) → feedback-
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.
Attachment #8634195 - Flags: review?(bugs) → feedback+
Attachment #8634188 - Attachment is obsolete: true
Attachment #8634194 - Attachment is obsolete: true
Attachment #8634195 - Attachment is obsolete: true
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.
Summary: Unify StructureCloneBuffer callbacks in workers and main-thread for postMessage() → Meta-Bug: Unify the use of StructureCloneBuffer in the DOM code (MessagePort, BroadcastChannel, Workers, window, DataStore, IDB, etc)
Depends on: 1184541
Depends on: 1184557
Depends on: 1184995
Depends on: 1185569
Depends on: 1186307
Depends on: 1186544
Depends on: 1188265
Depends on: 1188612
Depends on: 1189389
Depends on: 1198795
Depends on: 1198814
Depends on: 1201806
Depends on: 1203426
Depends on: 1203561
Depends on: 1203916
Depends on: 1203920
Depends on: 1204107
Depends on: 1209919
I would consider this as fixed.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.