1186307 - Unify the StructuredCloneCallbacks in WorkerPrivate.cpp

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Andrea Marchesini [:baku]]

Attachment: worker1.patch

We have 4 callbacks in WorkerPrivate:

1. for normal workers
2. for chrome workers
3. for window
4. for main-thread chrome window

and they are all quite similar. We should unify them all.
This first patch is just a try.

Comment 1

[Andrea Marchesini [:baku]]

Comment on attachment 8636960 [details] [diff] [review]
worker1.patch

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

Comment 2

[Andrea Marchesini [:baku]]

With this patch we extend the transferring ports to any postMessage() and we allow the cloning of ImageData, FormData and other stuff everywhere. But this is correct from spec as far as I can see.

Comment 3

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

Do we have tests for the cases where we support cloning with the patch, but don't without?
(I didn't read the patch yet, more than that it doesn't have tests)

Comment 4

[Andrea Marchesini [:baku]]

In my mind, this is the first patch of 3. The next one will use StructuredCloneHelper and the last one will test all.

Comment 5

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

Comment on attachment 8636960 [details] [diff] [review]
worker1.patch

So are all the types NS_DOMRead/WriteStructuredClone support threadsafe?
Or do they actually work? RTCCertificate for example can't be read on non-main thread it seems.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 1 - Unify the WorkerPrivate callbacks

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 2 - StructuredCloneHelper in workers + tests

Comment 8

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

Comment on attachment 8639928 [details] [diff] [review]
part 2 - StructuredCloneHelper in workers + tests

>+WriteBlob(JSStructuredCloneWriter* aWriter,
>+          Blob* aBlob,
>+          StructuredCloneHelper* aHelper)
>+{
>+  MOZ_ASSERT(aWriter);
>+  MOZ_ASSERT(aBlob);
>+  MOZ_ASSERT(aHelper);
>+
>+  BlobImpl* blobImpl = aBlob->Impl();
>+  if (JS_WriteUint32Pair(aWriter, SCTAG_DOM_BLOB,
>+                         aHelper->BlobImpls().Length())) {
Please add some comment here about writing Length().
Something that it is the index of blobImpl in the array


>+ReadFileList(JSContext* aCx,
>+             JSStructuredCloneReader* aReader,
>+             uint32_t aCount,
>+             StructuredCloneHelper* aHelper)
>+{
>+  MOZ_ASSERT(aCx);
>+  MOZ_ASSERT(aReader);
>+
>+  JS::Rooted<JS::Value> val(aCx);
>+  {
>+    nsRefPtr<FileList> fileList = new FileList(aHelper->ParentDuringRead());
>+
>+    uint32_t tag, offset;
>+    if (!JS_ReadUint32Pair(aReader, &tag, &offset)) {
Also here, please add some comment what offset is about.


>+// The format of the FileList serialization is:
>+// - pair of ints: SCTAG_DOM_FILELIST, Length of the FileList
>+// - pair of ints: 0, The offset of the BlobImpl
"offset of the BlobImpl" could use some clarification. Offset from what?
Just mention the blobimpl array or so.


>+// Read the WriteFormData for the format.
>+JSObject*
>+ReadFormData(JSContext* aCx,
>+             JSStructuredCloneReader* aReader,
>+             uint32_t aCount,
>+             StructuredCloneHelper* aHelper)
>+{
>+  MOZ_ASSERT(aCx);
>+  MOZ_ASSERT(aReader);
>+  MOZ_ASSERT(aHelper);
>+
>+  // See the serialization of the FormData for the format.
>+  JS::Rooted<JS::Value> val(aCx);
>+  {
>+    nsRefPtr<nsFormData> formData =
>+      new nsFormData(aHelper->ParentDuringRead());
>+
>+    Optional<nsAString> thirdArg;
>+    for (uint32_t i = 0; i < aCount; ++i) {
>+      nsAutoString name;
>+      if (!ReadString(aReader, name)) {
>+        return nullptr;
>+      }
>+
>+      uint32_t tag, index;
>+      if (!JS_ReadUint32Pair(aReader, &tag, &index)) {
>+        return nullptr;
>+      }
>+
>+      if (tag == SCTAG_DOM_BLOB) {
>+        MOZ_ASSERT(index < aHelper->BlobImpls().Length());
>+
>+        nsRefPtr<BlobImpl> blobImpl = aHelper->BlobImpls()[index];
>+        MOZ_ASSERT(blobImpl->IsFile());
>+
>+        nsRefPtr<File> file =
>+          File::Create(aHelper->ParentDuringRead(), blobImpl);
>+        MOZ_ASSERT(file);
>+
>+        formData->Append(name, *file, thirdArg);
>+      } else {
>+        nsAutoString value;
>+        if (!ReadString(aReader, value)) {
I don't see how this can work.
You write either
  (BLOG, index)
or
  (stringlength, 0)
  (bytes in string)

So, ReadString expects to be able to read the length of the string, but
if (!JS_ReadUint32Pair(aReader, &tag, &index)) { has already taken that information out from the reader.

Do we not have tests for this?  


>+// The format of the FormData serialization is:
>+// - pair of ints: SCTAG_DOM_FORMDATA, Length of the FormData elements
>+// - for each Element element:
>+//   - name string
>+//   - if it's a blob:
>+//     - pair of ints: SCTAG_DOM_BLOB, ID of the BlobImpl in the array
>+//       mBlobImplArray.
ID? Not ID but index.



>-class ServiceWorkerClientPostMessageRunnable final : public nsRunnable
>+class ServiceWorkerClientPostMessageRunnable final
>+  : public nsRunnable
>+  , public StructuredCloneHelper
> {
>   uint64_t mWindowId;
>-  JSAutoStructuredCloneBuffer mBuffer;
>-  WorkerStructuredCloneClosure mClosure;
> 
> public:
>-  ServiceWorkerClientPostMessageRunnable(uint64_t aWindowId,
>-                                         JSAutoStructuredCloneBuffer&& aData,
>-                                         WorkerStructuredCloneClosure& aClosure)
>-    : mWindowId(aWindowId),
>-      mBuffer(Move(aData))
>-  {
>-    mClosure.mClonedObjects.SwapElements(aClosure.mClonedObjects);
>-    MOZ_ASSERT(aClosure.mMessagePorts.IsEmpty());
>-    mClosure.mMessagePortIdentifiers.SwapElements(aClosure.mMessagePortIdentifiers);
>-  }
>+  explicit ServiceWorkerClientPostMessageRunnable(uint64_t aWindowId)
>+    : StructuredCloneHelper(CloningSupported, TransferringNotSupported)
Why no transferring?
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#service-worker-postmessage-method
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/index.html#client-postmessage

Comment 9

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/875870cf6979

This is just the first patch. Leave the bug open.
Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19d605e3b4b6

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6926efcbf60c

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/875870cf6979
https://hg.mozilla.org/mozilla-central/rev/6926efcbf60c

Comment 12

[Andrea Marchesini [:baku]]

Attachment: worker2.patch

ServiceWorker transferable ports, it's a followup because it introduces a new functionality. At the moment we don't support it (and we should).

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 2 - StructuredCloneHelper in workers + tests

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - StructuredCloneHelper in workers + tests

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 2 - StructuredCloneHelper in workers + tests

There was a couple of WPT failures with the previous patch.

Comment 16

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

Comment on attachment 8652394 [details] [diff] [review]
part 2 - StructuredCloneHelper in workers + tests


>-  if (!runnable->Write(aCx, aMessage, transferable,
>-                       &gWorkerStructuredCloneCallbacks)) {
>-    aRv = NS_ERROR_DOM_DATA_CLONE_ERR;
>+  runnable->Write(aCx, aMessage, transferable, aRv);
>+  if (NS_WARN_IF(aRv.Failed())) {
>+    return;
>+  }
>+
>+  if (!runnable->AreBlobsClonableToOtherThreads()) {
>+    aRv.Throw(NS_ERROR_DOM_DATA_CLONE_ERR);
This is error prone API. I think Write() should take a param telling whether data will be used in a different thread and throw if needed.




>+        ErrorResult rv;
>         JS::Rooted<JS::Value> response(aCx);
>-        if (!responseBuffer.read(aCx, &response, callbacks, &closure)) {
>+        Read(nullptr, aCx, &response, rv);
>+        if (NS_WARN_IF(rv.Failed())) {
>+          rv.SuppressException();
So here we use SuppressException()



>+    Read(nullptr, cx, &body, rv);
>+    if (NS_WARN_IF(rv.Failed())) {
>+      return rv.StealNSResult();
...but here StealNSResult().
What should the Read() caller use?



>+  nsRefPtr<SendRunnable> sendRunnable =
>+    new SendRunnable(mWorkerPrivate, mProxy, NullString());
>+
>+  JSContext* cx = mWorkerPrivate->GetJSContext();
>+
>   // Nothing to clone.
>-  JSAutoStructuredCloneBuffer buffer;
>-  WorkerStructuredCloneClosure closure;
>-
>-  SendInternal(NullString(), Move(buffer), closure, aRv);
>+  SendInternal(sendRunnable, aRv);
> }

Why you add cx variable, but don't use it?



>+  nsRefPtr<SendRunnable> sendRunnable =
>+    new SendRunnable(mWorkerPrivate, mProxy, aBody);
>+
>+  JSContext* cx = mWorkerPrivate->GetJSContext();
>+
>   // Nothing to clone.
>-  JSAutoStructuredCloneBuffer buffer;
>-  WorkerStructuredCloneClosure closure;
>-
>-  SendInternal(aBody, Move(buffer), closure, aRv);
>+  SendInternal(sendRunnable, aRv);
> }
Why you add cx variable, but don't use it?

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 2 - StructuredCloneHelper in workers + tests

Comments applied

Comment 18

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

Comment on attachment 8652734 [details] [diff] [review]
part 2 - StructuredCloneHelper in workers + tests

>+  nsTArray<nsRefPtr<MessagePortBase>> ports;
>+  StealTransferredPorts(ports);
Nit, I'd call the method TakeTransferredPorts. "Take" is used more often than "Steal" in this kind of cases, I think.


>+
>+    // |aCount| is the number of BlobImpls to use + |offset|.
This comment isn't right. aCount is the number of BlobImpls to use from offset


>+  JS::Rooted<JS::Value> val(aCx);
>+  {
>+    nsRefPtr<nsFormData> formData =
>+      new nsFormData(aHelper->ParentDuringRead());
>+
>+    Optional<nsAString> thirdArg;
>+    for (uint32_t i = 0; i < aCount; ++i) {
>+      nsAutoString name;
>+      if (!ReadString(aReader, name)) {
>+        return nullptr;
>+      }
>+
>+      uint32_t tag, index;
>+      if (!JS_ReadUint32Pair(aReader, &tag, &index)) {
I'd call index indexOrLengthOfString or something




I wonder if we should have some helper method to dispatch message event with right ports or source. Something for a followup.

I would have been nice to split the code moves to some other patch.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: worker2.patch

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

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a19daae11647

Comment 21

[Nigel Babu [:nigelb]]

You have bustage on W4.

Comment 22

[Nigel Babu [:nigelb]]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f537fe2647

Comment 23

[Nigel Babu [:nigelb]]

I made a mistake while backing out. That is the wrong commit. That backout is for Bug 1184058.

The failures look like this: https://treeherder.mozilla.org/logviewer.html#?job_id=13360593&repo=mozilla-inbound

I've actually backed this one out in this commit

https://hg.mozilla.org/integration/mozilla-inbound/rev/f83d676130fc

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 2 - StructuredCloneHelper in workers + tests

Comment 25

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ed63658fcba416b196c8e1fc33b78b873e95883
Bug 1186307 - StructuredCloneHelper in workers.postMessage(), r=smaug

Comment 26

[Andrea Marchesini [:baku]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ed63658fcba