Unify the StructuredCloneCallbacks in WorkerPrivate.cpp

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: baku, Assigned: baku)

Tracking

Trunk
mozilla43
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 affected, firefox43 fixed)

Details

Attachments

(2 attachments, 8 obsolete attachments)

Posted patch worker1.patch (obsolete) — Splinter Review
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.
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.
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)
In my mind, this is the first patch of 3. The next one will use StructuredCloneHelper and the last one will test all.
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.
Attachment #8636960 - Flags: review?(bugs)
Attachment #8636960 - Attachment is obsolete: true
Attachment #8639927 - Flags: review?(bugs)
Attachment #8639928 - Flags: review?(bugs)
Attachment #8639927 - Flags: review?(bugs) → review+
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
Attachment #8639928 - Flags: review?(bugs) → review-
Posted patch worker2.patch (obsolete) — Splinter Review
ServiceWorker transferable ports, it's a followup because it introduces a new functionality. At the moment we don't support it (and we should).
Attachment #8639928 - Attachment is obsolete: true
Attachment #8650467 - Flags: review?(bugs)
Attachment #8650467 - Attachment is obsolete: true
Attachment #8650467 - Flags: review?(bugs)
Attachment #8651050 - Flags: review?(bugs)
Attachment #8651050 - Attachment is obsolete: true
Attachment #8651050 - Flags: review?(bugs)
Attachment #8651838 - Flags: review?(bugs)
There was a couple of WPT failures with the previous patch.
Attachment #8651838 - Attachment is obsolete: true
Attachment #8651838 - Flags: review?(bugs)
Attachment #8652394 - Flags: review?(bugs)
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?
Attachment #8652394 - Flags: review?(bugs) → review-
Comments applied
Attachment #8652394 - Attachment is obsolete: true
Attachment #8652734 - Flags: review?(bugs)
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.
Attachment #8652734 - Flags: review?(bugs) → review+
You have bustage on W4.
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ed63658fcba
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1199265
Depends on: 1199756
You need to log in before you can comment on or make changes to this bug.