912456 - The behavior of the postMessage method differs from the specification and all other engines wrt MessagePort handling

Status: RESOLVED FIXED

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

Description

[SUN Haitao]

According to http://www.whatwg.org/specs/web-apps/current-work/multipage/web-messaging.html#posting-messages (Step 2, 4.2, 7, 10),
a MessageEvent object should have a 'ports' property, which is an array of all MessagePort objects listed in the 'transferable' argument of the postMessage method.

But currently, this property does not exist on Gecko.

All other engines also disallow the 'message' argument contains any MessagePort object by throwing a data clone error, but Gecko doesn't mind.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

the MessageEvent.ports is in a separated patch.

Comment 2

[Andrea Marchesini [:baku]]

Attachment: patch

I forgot a mochitest.

Comment 3

[SUN Haitao]

I'm afraid the behavior still is not compatible. 

The |postMessage| method must can transfer ports ONLY appearing in the transferable list, because the signature of |window.postMessage| used to be |postMessage(message, domain, ports)|.

And all other engines throws a clone error if the 'message' argument is or contains a MessagePort object.

Comment 4

[Andrea Marchesini [:baku]]

> The |postMessage| method must can transfer ports ONLY appearing in the
> transferable list, because the signature of |window.postMessage| used to be
> |postMessage(message, domain, ports)|.
> 
> And all other engines throws a clone error if the 'message' argument is or
> contains a MessagePort object.

Yep. This is exactly what that patch meant to do. Which mochitest or piece of code is not going in this direction?
Thanks for your comments.

Keep in mind that MessageEvent.ports is not implemented yet. I proposed a patch for it here: Bug 848294.
The integration of MessagePort and MessageEvent.ports in postMessage() will be done in a follow up.

Comment 5

[SUN Haitao]

Attachment: MessageEvent.ports

It seems that I found a working solution for this (Although it is still suffering from Bug 913761).

The implementation may be a little rough. But the tests are ensuring the right behavior.

Comment 6

[SUN Haitao]

Attachment: postMessage

The basic idea is prepare two sets of structured clone callbacks. One for data, accepts everything except MessagePort; the other for ports, accepts nothing but MessagePort.

Comment 7

[SUN Haitao]

Attachment: Tests

This is just an adjusted version of existing tests. Maybe more testcases should be introduced to ensure that the 'message' argument could no longer contain any MessagePort object.

Comment 8

[SUN Haitao]

> Yep. This is exactly what that patch meant to do. Which mochitest or piece
> of code is not going in this direction?
> Thanks for your comments.

Sorry, I'm afraid I was mislead by existing fragments like |window.parent.postMessage({ status: "FINISH", port: a.port2 }, '*', [a.port2]);| in tests. Without |MessageEvent.ports| implemented, such usages are temporarily required.

Comment 9

[SUN Haitao]

And I'm sorry for not noticing your progress while uploading my experimental patches. Synchronization issues.

Comment 10

[Andrea Marchesini [:baku]]

Comment on attachment 801099 [details] [diff] [review]
MessageEvent.ports

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

We cannot change nsIDOMMessageEvent.idl in this way. Actually the spec removes initMessageEvent fully.

If you don't mind, I would like to have my patches reviewed and fix this issue here.
About MessageEvent.ports, there is another bug with another patch waiting for review.
When these 2 bugs are gone, we can implement MessagePort.ports connected to PostMessage().

Comment 11

[SUN Haitao]

(In reply to Andrea Marchesini (:baku) from comment #10)
> Comment on attachment 801099 [details] [diff] [review]

> If you don't mind, I would like to have my patches reviewed and fix this
> issue here.
> About MessageEvent.ports, there is another bug with another patch waiting
> for review.
> When these 2 bugs are gone, we can implement MessagePort.ports connected to
> PostMessage().

I believe this is the right plan, as long as it won't take too much time.

If we can't land all those patches in Gecko 26, I'm afraid that some temporarily solution may be helpful.

And I suggest leaving this bug as new until MessageEvent.ports is available.

Comment 12

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

Comment on attachment 800731 [details] [diff] [review]
patch


> 
> JSStructuredCloneCallbacks kPostMessageCallbacks = {
>   PostMessageReadStructuredClone,
>   PostMessageWriteStructuredClone,
>-  nullptr
>+  nullptr,
>+  PostMessageParseTransferStructuredClone
> };

So this patch seems to rely on some other patch which hasn't landed yet.
Hard to review this way
(Currently JSStructuredCloneCallbacks takes 3 callbacks, not 4,
http://mxr.mozilla.org/mozilla-central/source/js/public/StructuredClone.h?rev=9e98958b5e50&mark=52-55#52)

PostMessageParseTransferStructuredClone is also quite oddly named.

Could you explain the patch a bit and change the name of PostMessageParseTransferStructuredClone.

Comment 13

[SUN Haitao]

Is there any update in progress? MessageEvent.ports seems available now.

Comment 14

[Andrea Marchesini [:baku]]

I should have something ready for this week.

Comment 15

[SUN Haitao]

Will it also land on Aurora channel?

Comment 16

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

No. MessagePort and MessageChannel are disabled by default, so there isn't any need to
change their behavior on Aurora.

Comment 17

[Andrea Marchesini [:baku]]

Attachment: patch

I'm still checking if it's green on try, but the patch is almost ready.

Comment 18

[SUN Haitao]

Although |portA.postMessage({port:portB}, [portB]);| is acceptable, I suspect that |portA.postMessage(null, [portB]);| is more preferred.

So It may be better if we update most of related tests to use the less verbose form.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: patch

https://tbpl.mozilla.org/?tree=Try&rev=5b3ed655dfae

Comment 20

[Steve Fink [:sfink] [:s:]]

So, rather than review your patch, I went off and finished up bug 861925, which will bit-rot this. :(

Sorry, but from skimming through your patch, I think it'll work out pretty well. The main problem with your patch is that it follows the current transferring mechanism in the structured clone implementation, which unfortunately is wrong: if your Transferable does something when it's neutered, then any cloned objects that refer to your transferred objects are going to see them in their neutered state. For ArrayBuffers and ArrayBufferViews, that means that all of the views ended up zero-length if they used a transferred ArrayBuffer. That's bad. More subtly, it caused the contents of the transferred stuff to get baked in at the beginning of the cloning process, which means that any changes made during the clone might (or might not!) be ignored. The spec says that neutering happens at the end, so mutations in the middle (eg from getters with side effects) should be reflected in the cloned data.

Anyway, I think it'll be very easy to rebase this on top of my bug 861925 patch. Your 3 extension points are the right ones, though the details will need to change slightly. I'll review the parts of this patch that won't change first, though.

Comment 21

[Andrea Marchesini [:baku]]

(In reply to Steve Fink [:sfink] from comment #20)
> So, rather than review your patch, I went off and finished up bug 861925,
> which will bit-rot this. :(

I'm fine with this. The only concern is the timing of your patch plus my patch on top of it.
When do you think your patch can land?

This patch is part of a bigger picture where I have to implement:
1. MessagePort Proxy
2. porting MessagePort to workers
3. ShareWorkers + MessagePorts
4. something else that I don't remember :)

And I want to do all of this in the next 3 months. If you think that your patch can land in 1 or 2 weeks, this is fine for me. Otherwise we can proceed with my patch and then you rebase your code on top of mine. You decide :)
How does it sound?

Comment 22

[Steve Fink [:sfink] [:s:]]

Comment on attachment 807841 [details] [diff] [review]
patch

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

I will look more later.

::: js/public/StructuredClone.h
@@ +50,5 @@
> +// This is called when JS_ReadStructuredClone receives an unknown transferable
> +// object. If this hook doesn't exists or it returns false, JS engine throws a
> +// DATA_CLONE_ERR DOM Exception. This method is called before any other callback.
> +typedef bool (*ReadTransferStructuredCloneOp)(JSContext *cx, JSStructuredCloneReader *r,
> +                                              uint32_t tag, uint32_t data, void *closure);

I think data will change to be 64 bits in all of these. The rest is good, I think.

::: js/src/vm/StructuredClone.cpp
@@ +72,1 @@
>      SCTAG_TYPED_ARRAY_OBJECT,

We can't shift SCTAG_TYPED_ARRAY_OBJECT up. We need to be backwards-compatible with old clone buffer stored in IndexedDB or wherever.

This doesn't apply to anything specific to transferables, though, since they cannot be written to persistent storage. I think we should probably change SCTAG_TRANSFER_MAP_HEADER and SCTAG_TRANSFER_MAP's values to unused placeholders, and create a new range (starting at 0xFFFFF0200, maybe?) for all of the transferable-specific stuff. That'll allow us to change them freely, since there are no backcompat issues.

@@ +263,5 @@
>  
>      friend bool JS_WriteTypedArray(JSStructuredCloneWriter *w, JS::Value v);
>  };
>  
> +struct JSStructuredCloneFreer {

I need to look at this more closely. I hope it can be simplified.

@@ +915,5 @@
> +                    reportErrorTransferable();
> +                    return false;
> +                }
> +
> +                continue;

The continue doesn't seem right, since it's skipping the insertion into memory. Seems like this breaks cycle detection (or plain duplicate detection.)

Comment 23

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

Comment on attachment 807841 [details] [diff] [review]
patch


>+PostMessageFreeTransferStructuredClone(JSStructuredCloneFreer* freer,
>+                                       uint32_t tag, uint32_t data,
>+                                       void *aClosure)
aFoo for param names. Here and elsewhere in dom/ code.


>+{
>+  StructuredCloneInfo* scInfo = static_cast<StructuredCloneInfo*>(aClosure);
>+  NS_ASSERTION(scInfo, "Must have scInfo!");
>+
>+  if (tag == SCTAG_DOM_MAP_MESSAGEPORT) {
>+    MessagePort* port;
>+    if (JS_ReadBytes(freer, &port, sizeof(port))) {
>+      scInfo->mPorts.Remove(port);
>+      port->Release();
nsRefPtr for port, so that manual Release isn't needed.


>+PopulateMessagePortList(MessagePort *aKey, MessagePort *aValue, void *aClosure)
* goes with the type. Same also elsewhere.


>+MessagePort*
> MessagePort::Clone()
> {
>-  nsRefPtr<MessagePort> newPort = new MessagePort(nullptr);
>+  MessagePort* newPort = new MessagePort(nullptr);
> 
>   // Move all the events in the port message queue of original port.
>   newPort->mMessageQueue.SwapElements(mMessageQueue);
> 
>   if (mEntangledPort) {
>     nsRefPtr<MessagePort> port = mEntangledPort;
>     mEntangledPort = nullptr;
> 
>     newPort->Entangle(port);
>     port->Entangle(newPort);
>   }
> 
>-  return newPort.forget();
>+  return newPort;
> }
Why you stop returning already_AddRefed? I would really prefer already_AddRefed in this kind of case.

>-  message->SetTrusted(mTrustedCaller);
>-  nsEvent *internalEvent = message->GetInternalNSEvent();
>+  event->SetTrusted(mTrustedCaller);
>+  nsEvent *internalEvent = event->GetInternalNSEvent();
So the event should always be trusted here.

Comment 24

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 25

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

Comment on attachment 809162 [details] [diff] [review]
patch

Sorry, this took time. The patch is hard to understand, partially because of the API being silly and inconsistent when handling MessagePorts

We should somehow reduce code duplication in MessagePort and nsGlobalWindow. Followup please, if there isn't such bug filed already.

>+PostMessageReadTransferStructuredClone(JSContext* aCx,
>+                                       JSStructuredCloneReader* reader,
>+                                       uint32_t tag, uint32_t data,
>+                                       void* aClosure, JSObject** returnObject)
Please be consistent. Parameter names should be aFoo.
Same also elsewhere in the dom/ code.
And * goes with the type, not with the variable or param name (few occasions in the patch)


>+      JS::Rooted<JSObject*> global(aCx, JS::CurrentGlobalOrNull(aCx));
>+      if (global) {
>+        JS::Rooted<JSObject*> obj(aCx, port->WrapObject(aCx, global));
>+        if (JS_WrapObject(aCx, obj.address())) {
>+          MOZ_ASSERT(port->GetOwner() == scInfo->mPort->GetOwner());
>+          *returnObject = obj;
Shouldn't we return true only here

Hmm, closure should take MutableHandle, not JSObject**

>+PostMessageReadTransferStructuredClone(JSContext* aCx,
>+                                       JSStructuredCloneReader* reader,
>+                                       uint32_t tag, uint32_t data,
>+                                       void* aClosure, JSObject** returnObject)
>+{
>+  StructuredCloneInfo* scInfo = static_cast<StructuredCloneInfo*>(aClosure);
>+  NS_ASSERTION(scInfo, "Must have scInfo!");
>+
>+  if (MessageChannel::PrefEnabled() && tag == SCTAG_DOM_MAP_MESSAGEPORT) {
Perhaps check first the tag and then pref. Pref checking is slower


Would be nice if there was some documentation about the setup. Perhaps in MessagePort.cpp and then link to that documentation in nsGlobalWindow.cpp

Comment 26

[SUN Haitao]

Anything blocking this to land?

Comment 27

[Andrea Marchesini [:baku]]

(In reply to SUN Haitao from comment #26)
> Anything blocking this to land?

Yes, a second review from :sfink. Any news?

Comment 28

[Steve Fink [:sfink] [:s:]]

I started looking at this today. Reviewing the current patch is not entirely meaningful; it really needs to be rebased on top of my latest changes. Everything but js/src/vm/StructuredClone.cpp rebases more or less painlessly, but there are some semantic conflicts in that file. I think I'd better continue to work on the "rebase", since it's really the application to my latest cloning scheme to extensible transferables. I won't be able to work on it tomorrow, but I'll get back to it on Wednesday. Sorry for the long delay.

Comment 29

[Steve Fink [:sfink] [:s:]]

Attachment: bug-912456-patch

FYI, this is the rebased patch that I'm trying to get working right now.

There's one part that's rather odd. Here's the main comment for it:

+// This is called when JS_ClearStructuredClone, freeing data, has to manage an
+// unknown transferable object. Note that this is handed the content directly,
+// rather than using JS_ReadBytes, because when freeing a buffer containing a
+// custom Transferable object that has already been consumed, we have to be
+// able to skip over the custom data. (An alternative would be to pass in a
+// parameter saying whether to skip or free the data.)
+typedef bool (*FreeTransferStructuredCloneOp)(JSStructuredCloneData *d,
+                                              uint32_t tag, uint32_t ownership, void *content, void *closure);
+

That comment could be made more clear.

So the main functionality I want is for JSAutoStructuredCloneBuffer and JS_ClearStructuredCloneBuffer to be able to clean up/release custom transferables properly. The original patch didn't do this; there were cases where the destructor wouldn't have the right callbacks and it'd just clean up the stuff it knew about.

Then there's the nasty case where some of the stuff in the buffer has been cleared out and some hasn't. To traverse the buffer correctly, that requires the cleanup function to be called and not do anything but call JS_ReadBytes properly to skip over the transferable data.

So for now, I simplified it by restricting the interface to only have a tag, an "ownership indicator", a 64-bit pointer, and a 64-bit "userdata" value to play with. If a Transferable needs more than this, it'll have to heap-allocate it and pass it via pointer.

The alternative would be to tell the freeTransfer callback that it must always read past all of the data, but only actually free the data if the ownership value says to.

I'm kinda thinking now that that's a better approach, but I have this mostly working at the moment so I'm going to plow ahead with it for now. ;-) Opinions encouraged.

Current try push is https://tbpl.mozilla.org/?tree=Try&rev=2a2f977ae0de but I doubt it'll be the last.

Comment 30

[Steve Fink [:sfink] [:s:]]

Attachment: transferable MessagePorts

Just managed to get back to this. One question -- I was failing several mochitests because my previous patch was forcing events to be trusted. I assume I got that part from your patch, since I've never heard of event.isTrusted nor do I really understand what's going on.

Anyway, I reverted that part of the patch. This patch passes at least 4 mochitests that were failing with the unconditional event->SetTrusted(true) call. But maybe there's a reason why this doesn't make sense. Can you look at the following patch and let me know if something needs to be done?

I'll also push this to try: https://tbpl.mozilla.org/?tree=Try&rev=865b7e8057c1

Comment 31

[Steve Fink [:sfink] [:s:]]

Argh, I really need to make bzexport not grab bugs when they're already assigned.

Comment 32

[Steve Fink [:sfink] [:s:]]

Attachment: imported patch trusted

This is the part of the patch that I removed to get those mochitests to work.

Comment 33

[Florian Bender]

I guess that was unintentional as well.

Comment 34

[SUN Haitao]

Any chance for this to land on Gecko 28?

Comment 35

[Andrea Marchesini [:baku]]

Comment on attachment 831882 [details] [diff] [review]
transferable MessagePorts

I don't understand how it can work. |writeTranfer| is never called.
Do I miss something?

Comment 36

[Steve Fink [:sfink] [:s:]]

(In reply to Andrea Marchesini (:baku) from comment #35)
> Comment on attachment 831882 [details] [diff] [review]
> transferable MessagePorts
> 
> I don't understand how it can work. |writeTranfer| is never called.
> Do I miss something?

Oh! Sorry, I should have cleared that feedback flag. Yes, as you noticed, this never even calls writeTransfer. My current patch fixes this, but it also introduces an error on 32-bit platforms that I haven't tracked down yet.

Comment 37

[Andrea Marchesini [:baku]]

when do you think we can have that patch landed? or ready for a review?

Comment 38

[Steve Fink [:sfink] [:s:]]

Attachment: Remove cx from SCInput so it can be used for JSStructuredCloneData

Preparation for the following patch, because it clutters up the diff too much.

Comment 39

[Steve Fink [:sfink] [:s:]]

Attachment: transferable MessagePorts

jorendorff, requesting review of the JSAPI side. baku, I think you had an r+ on your original version of this patch, so requesting review to check that I didn't mess anything up when rewriting it. I believe I fixed some cleanup handling (previously, there were paths that would discard a structured clone buffer without passing in the transferable freers.)

https://tbpl.mozilla.org/?tree=Try&rev=ff24212abb0c is a slightly different version of this patch. (Specifically, I removed the closure data from the clone buffer after that push.)

https://tbpl.mozilla.org/?tree=Try&rev=9b0f8e4f4164 is the exact patches attached here.

Comment 40

[Andrea Marchesini [:baku]]

Comment on attachment 8342785 [details] [diff] [review]
transferable MessagePorts

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

Looks good! f+ because I cannot review JS code.

::: js/src/vm/StructuredClone.cpp
@@ +202,5 @@
>  } /* namespace js */
>  
> +struct JSStructuredCloneData : public js::SCInput {
> +    JSStructuredCloneData(uint64_t *data, size_t nbytes)
> +      : SCInput(data, nbytes)

should this be 4 spaces?

@@ +388,5 @@
> +
> +        void *content;
> +        JS_ALWAYS_TRUE(buffer->readPtr(nullcx, &content));
> +
> +        if (ownership >= JS::SCTAG_TMO_FIRST_OWNED) {

I don't know the guideline for the JS module, but probably it's better to write:

if (ownership < JS::SCTAG_TMO_FIRST_OWNED)
  continue;

..

Comment 41

[Jason Orendorff [:jorendorff]]

Comment on attachment 8342781 [details] [diff] [review]
Remove cx from SCInput so it can be used for JSStructuredCloneData

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

::: js/src/vm/StructuredClone.cpp
@@ +1778,5 @@
>  
>  JS_PUBLIC_API(bool)
>  JS_ReadUint32Pair(JSStructuredCloneReader *r, uint32_t *p1, uint32_t *p2)
>  {
> +    return r->input().readPair(r->context(), (uint32_t *) p1, (uint32_t *) p2);

Drop the unnecessary casts while you're in here?

Comment 42

[Steve Fink [:sfink] [:s:]]

(In reply to Andrea Marchesini (:baku) from comment #40)
> Comment on attachment 8342785 [details] [diff] [review]
> transferable MessagePorts
> 
> Review of attachment 8342785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! f+ because I cannot review JS code.

Sorry, I meant for you to only review the code outside of js/. I changed your changes quite a bit. jorendorff is flagged for review of the js/ changes.

> 
> ::: js/src/vm/StructuredClone.cpp
> @@ +202,5 @@
> >  } /* namespace js */
> >  
> > +struct JSStructuredCloneData : public js::SCInput {
> > +    JSStructuredCloneData(uint64_t *data, size_t nbytes)
> > +      : SCInput(data, nbytes)
> 
> should this be 4 spaces?

Not in spidermonkeyland. At least, I think our standard is a half-indent for these.

> @@ +388,5 @@
> > +
> > +        void *content;
> > +        JS_ALWAYS_TRUE(buffer->readPtr(nullcx, &content));
> > +
> > +        if (ownership >= JS::SCTAG_TMO_FIRST_OWNED) {
> 
> I don't know the guideline for the JS module, but probably it's better to
> write:
> 
> if (ownership < JS::SCTAG_TMO_FIRST_OWNED)
>   continue;
> 
> ..

You're right, that's better.

Comment 43

[Andrea Marchesini [:baku]]

if I have to review my own code... than it's a r+ :)
Happy to see this patch landed \o/

Comment 44

[Andrea Marchesini [:baku]]

Steve, do you have any news about this bug?

Comment 45

[Steve Fink [:sfink] [:s:]]

Just periodically pestering :jorendorff. I'll re-point the needinfo at him as punishment.

Comment 46

[Steve Fink [:sfink] [:s:]]

Attachment: Remove cx from SCInput so it can be used for JSStructuredCloneData

Rebased

Comment 47

[Steve Fink [:sfink] [:s:]]

Attachment: transferable MessagePorts

Whoa, this rebase was surprisingly easy. Makes me nervous. I guess all I know is that it still compiles.

Comment 48

[Jason Orendorff [:jorendorff]]

Comment on attachment 8375785 [details] [diff] [review]
Remove cx from SCInput so it can be used for JSStructuredCloneData

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

I'm missing something. It seems like all that's really needed here is:

> struct JSStructuredCloneReader {
>   public:
> [...]
>     js::SCInput &input() { return in; }
>+    JSContext *context() { return in->context(); }

With that, is the rest needed?

Clearing review.

Feel free to rename eof() to reportTruncated() regardless. Quite an improvement.

>+  protected:
>     uint64_t *point;
>     uint64_t *end;

I didn't understand what this was for, either.

::: js/src/vm/StructuredClone.cpp
@@ +151,5 @@
>      js::Vector<uint64_t> buf;
>  };
>  
>  struct SCInput {
>    public:

Change `struct` to `class` while you're in here?

Comment 49

[Steve Fink [:sfink] [:s:]]

(In reply to Jason Orendorff [:jorendorff] from comment #48)
> Comment on attachment 8375785 [details] [diff] [review]
> Remove cx from SCInput so it can be used for JSStructuredCloneData
> 
> Review of attachment 8375785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm missing something. It seems like all that's really needed here is:
> 
> > struct JSStructuredCloneReader {
> >   public:
> > [...]
> >     js::SCInput &input() { return in; }
> >+    JSContext *context() { return in->context(); }
> 
> With that, is the rest needed?

The point is not getting access to the cx. It's because the following patch needs to be able to construct a JSStructuredCloneData at a time it doesn't have access to a cx (during teardown). Sorry; I just split out the cx removal because it produced a lot of code churn that obscured the rest of the patch.

Either that, or I'm missing something. I suppose I could have left the cx in but set it to NULL. Would that be better?

> 
> Clearing review.
> 
> Feel free to rename eof() to reportTruncated() regardless. Quite an
> improvement.
> 
> >+  protected:
> >     uint64_t *point;
> >     uint64_t *end;
> 
> I didn't understand what this was for, either.

Oops, sorry. That logically belongs in the next patch. I need to be able to do random access updates to a JSStructuredCloneData.

> ::: js/src/vm/StructuredClone.cpp
> @@ +151,5 @@
> >      js::Vector<uint64_t> buf;
> >  };
> >  
> >  struct SCInput {
> >    public:
> 
> Change `struct` to `class` while you're in here?

ok

Comment 50

[Jason Orendorff [:jorendorff]]

Comment on attachment 8375786 [details] [diff] [review]
transferable MessagePorts

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

Reviewed only the bits in js/src. Everything looks good.

::: js/public/StructuredClone.h
@@ +77,5 @@
> +// object. If this hook doesn't exists or it returns false, JS engine throws a
> +// DATA_CLONE_ERR DOM Exception. This method is called before any other callback.
> +typedef bool (*ReadTransferStructuredCloneOp)(JSContext *cx, JSStructuredCloneReader *r,
> +                                              uint32_t tag, void *content, void *closure,
> +                                              JS::MutableHandleObject returnObject);

Might want to comment that returnObject has to be non-null on a successful return.

@@ +127,5 @@
>                         JS::MutableHandleValue vp,
>                         const JSStructuredCloneCallbacks *optionalCallbacks, void *closure);
>  
>  // Note: On success, the caller is responsible for calling
> +// JS_ClearStructuredClone(*datap, nbytesp, optionalCallbacks, closure).

Pre-existing nit: there should be a star, *nbytesp; or else the p should be dropped, like:

    JS_ClearStructuedClone(data, nbytes, optionalCallbacks, closure)

::: js/src/vm/StructuredClone.cpp
@@ +205,5 @@
> +    JSStructuredCloneData(uint64_t *data, size_t nbytes)
> +      : SCInput(data, nbytes)
> +    { }
> +
> +    size_t nbytes() const { return end - point; }

Need to multiply by sizeof(uint64_t) to get bytes here.

@@ +360,5 @@
>      return r.read(vp.address());
>  }
>  
>  // This may acquire new ways of discarding transfer map entries as new
>  // Transferables are implemented.

This comment was written for DiscardEntry, not Discard; I think it can be deleted now.

It might be nice to have a comment here, though.

@@ +1538,5 @@
> +            if (!callbacks || !callbacks->readTransfer ||
> +                !callbacks->readTransfer(context(), this, tag, content, closure, &obj))
> +            {
> +                return false;
> +            }

The error handling isn't right here. Each failure path needs to report an error.

    if (!callbacks) {
        js_ReportError...(cx, ...);
        return false;
    }
    if (!callbacks->readTransfer) {
        // Here in particular, the nice comment you wrote promises
        // a very particular callback, to wit:
        callbacks->reportError(cx, ...);
        return false;
    }
    if (!callbacks->readTransfer(...))
        return false;
    if (!obj) {
        js_ReportError...(cx, ...);
        return false;
    }

Comment 51

[Steve Fink [:sfink] [:s:]]

Attachment: transferable MessagePorts

Re-requesting review because of a pretty major rebase across sstangl's SharedArrayBufferObject changes. That, combined with merging in portions of the other patch, ended up changing this a fair amount.

I applied all of your review comments except for:

(In reply to Jason Orendorff [:jorendorff] from comment #50)
> The error handling isn't right here. Each failure path needs to report an
> error.
> 
>     if (!callbacks) {
>         js_ReportError...(cx, ...);
>         return false;
>     }
>     if (!callbacks->readTransfer) {
>         // Here in particular, the nice comment you wrote promises
>         // a very particular callback, to wit:
>         callbacks->reportError(cx, ...);
>         return false;
>     }

So this is a case where you are reading a structured clone buffer containing a Transferable, and you don't have a handler for custom transferables (i.e., anything other than ArrayBuffers or SharedArrayBuffers.) While cloning is supposed to be robust against bogus data when there are no Transferables involved, it really can't be when there are Transferables (because you could always just feed it a bogus pointer and there's no way it could detect it.)

In that case, it seems like !callbacks or !callbacks->readTransfer is a programming error. You shouldn't see transferables that you don't know how to handle. At the very least, that'd probably be a memory leak (because you most likely also don't know how to free them either.)

The attached patch just asserts in this case. (An opt build would seg fault on the callbacks->readTransfer() call.) Is that too risky?

We still need to re-robustify clone buffers against bogus data when they *don't* have any transferables, and probably make the two flavors of buffers more distinguishable. But that's a separate task.

Comment 52

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8375785 [details] [diff] [review]
Remove cx from SCInput so it can be used for JSStructuredCloneData

Not using this patch anymore.

Comment 53

[Jason Orendorff [:jorendorff]]

Comment on attachment 8383512 [details] [diff] [review]
transferable MessagePorts

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

Sorry I didn't get this done Friday.

r=me with a bunch of annoying comments.

::: js/public/StructuredClone.h
@@ +40,5 @@
> +    // Data is a SharedArrayBufferObject's buffer
> +    SCTAG_TMO_SHARED_BUFFER = 3,
> +
> +    // General marker usable by custom transferables, for saying "yes, I own
> +    // this, and I know what that means"

This comment is baffling. Maybe it means

    // Data is embedding-specific. The engine can free it by calling
    // the freeTransfer op.

@@ +75,5 @@
>  // with error set to one of the JS_SCERR_* values.
>  typedef void (*StructuredCloneErrorOp)(JSContext *cx, uint32_t errorid);
>  
> +// This is called when JS_ReadStructuredClone receives an unknown transferable
> +// object. If this hook doesn't exists or it returns false, JS engine throws a

"Embedding-defined" would be better than "unknown", or perhaps "receives a transferable that isn't an ArrayBuffer".

Grammar nit: "doesn't exist"

@@ +76,5 @@
>  typedef void (*StructuredCloneErrorOp)(JSContext *cx, uint32_t errorid);
>  
> +// This is called when JS_ReadStructuredClone receives an unknown transferable
> +// object. If this hook doesn't exists or it returns false, JS engine throws a
> +// DATA_CLONE_ERR DOM Exception. This method is called before any other

This isn't quite right: what happens is the StructuredCloneErrorOp callback is called next.

@@ +84,5 @@
> +                                              JS::MutableHandleObject returnObject);
> +
> +// This is called when JS_WriteStructuredClone receives an unknown transferable
> +// object. If this hook doesn't exists or it returns false, JS engine throws a
> +// DATA_CLONE_ERR DOM Exception. This method is called before any other

Same comments here.

@@ +88,5 @@
> +// DATA_CLONE_ERR DOM Exception. This method is called before any other
> +// callback.
> +//
> +//  tag: indicates what type of transferable this is. Must be >
> +//       SCTAG_TRANSFER_MAP_PENDING_ENTRY

The ">" is confusing there. Maybe spell out "greater than".

SCTAG_TRANSFER_MAP_PENDING_ENTRY is confusing too, considering that constant isn't in the public header.

@@ +125,1 @@
>  };

BTW, it would be OK to refactor this to be a C++ abstract base class, if that would make client code look nicer. Possible followup bug...

@@ +135,4 @@
>  JS_PUBLIC_API(bool)
>  JS_WriteStructuredClone(JSContext *cx, JS::HandleValue v, uint64_t **datap, size_t *nbytesp,
>                          const JSStructuredCloneCallbacks *optionalCallbacks,
>                          void *closure, JS::HandleValue transferable);

Another: make JSAutoStructuredCloneBuffer the sole API for this, and ditch the C stuff.

::: js/src/vm/StructuredClone.cpp
@@ +141,5 @@
>      bool writeArray(const T *p, size_t nbytes);
>  
> +    static void setPair(uint64_t *buffer, uint32_t tag, uint32_t data) {
> +        *buffer = PairToUInt64(tag, data);
> +    }

This doesn't belong in SCOutput. It's used only while reading from an SCInput.

How about private methods in SCInput:

    void markItemTransferred(uint64_t *itemp, uint32_t tag) {
        MOZ_ASSERT(point <= itemp && itemp < end);
        *itemp = PairToUint64(tag, JS::SCTAG_TMO_UNOWNED);
    }

    void markAllTransferred(uint64_t *headerp) {
        MOZ_ASSERT(point <= itemp && itemp < end);
        MOZ_ASSERT(TAG(headerp) == SCTAG_TRANSFER_MAP_HEADER);
        MOZ_ASSERT(TransferableMapHeader(DATA(headerp)) != SCTAG_TM_TRANSFERRED);
        *headerp = PairToUint64(SCTAG_TRANSFER_MAP_HEADER, SCTAG_TM_TRANSFERRED);
    }

If you don't want that nicety, I think just inline setPair at the two call sites; in any case, not here.

@@ +351,2 @@
>  static void
> +Discard(uint64_t *buffer, size_t nbytes, const JSStructuredCloneCallbacks *cb, void *cbClosure)

This whole function reads very nicely now.

@@ +389,1 @@
>          }

...else it's a horrible bug, because we should have had one of these three cases, right? MOZ_ASSERT_UNREACHED if so.

@@ -384,2 @@
>  {
> -    JS_ASSERT(nbytes % 8 == 0);

Why remove this assertion? It still has to be true, right? Particularly, an attacker must not be able to control the arguments here...?

(I'm now actually unsure about the security properties we want this to have.)

@@ +396,3 @@
>  {
> +    Discard(data, nbytes, cb, cbClosure);
> +    js_free((void *)data);

Remove the unnecessary cast?

@@ +469,5 @@
>  
>  bool
>  SCInput::getPair(uint32_t *tagp, uint32_t *datap)
>  {
> +    uint64_t u = 0;

Bah. Is this "= 0" necessary even with the "to shut GCC up" initialization in readNativeEndian? (Just curious how annoyed I should be at GCC, no action needed.)

@@ +551,5 @@
> +SCInput::getPtr(const uint64_t *p, void **ptr)
> +{
> +    // No endianness conversion is used for pointers, since they are not sent
> +    // across address spaces anyway.
> +    *ptr = reinterpret_cast<void*>(*p);

Your call, but I think it would be better to use little endian consistently for everything. Pointers are such a small fraction of what's in the buffer, and all the platforms we care about are little-endian anyway, right?

@@ +569,5 @@
>  
>  SCOutput::~SCOutput()
>  {
>      // Free any transferable data left lying around in the buffer
> +    Discard(rawBuffer(), count() * sizeof(uint64_t), nullptr, nullptr);

Can this be right? I think it is necessary to pass correct values for arguments 3 and 4, since transferOwnership() can fail partway through.

@@ +974,5 @@
>  
>      for (JS::AutoObjectVector::Range tr = transferableObjects.all(); !tr.empty(); tr.popFront()) {
> +        RootedObject obj(context(), tr.front());
> +
> +        obj = CheckedUnwrap(obj);

Interesting change. Please consider landing this in a separate patch, before everything else, with a shell-only test.

@@ +976,5 @@
> +        RootedObject obj(context(), tr.front());
> +
> +        obj = CheckedUnwrap(obj);
> +        if (!obj) {
> +            JS_ReportError(context(), "Permission denied to access object");

JS_ReportErrorNumber(context(), js_GetErrorMessage, nullptr, JSMSG_UNWRAP_DENIED);

@@ +1540,5 @@
> +            obj = SharedArrayBufferObject::New(context(), (SharedArrayRawBuffer *)content);
> +        } else {
> +            JS_ASSERT(callbacks && callbacks->readTransfer);
> +            if (!callbacks->readTransfer(cx, this, tag, content, closure, &obj))
> +                return false;

Please JS_ASSERT(obj) and JS_ASSERT(!cx->isExceptionPending()) after this, to blame the callback right away for any misbehavior.

@@ +1550,3 @@
>  
>          if (!obj)
>              return false;

I think the pre-existing code had the order right: try to transfer; on OOM, return false; then (only in the success path, but before the call to append()) mark the entry in the buffer as having been transferred.

This way is a leak, right?

@@ +1594,1 @@
>  using namespace js;

Pre-existing, but please remove this redundant using-declaration.

Comment 54

[Steve Fink [:sfink] [:s:]]

(In reply to Jason Orendorff [:jorendorff] from comment #53)
> @@ +76,5 @@
> >  typedef void (*StructuredCloneErrorOp)(JSContext *cx, uint32_t errorid);
> >  
> > +// This is called when JS_ReadStructuredClone receives an unknown transferable
> > +// object. If this hook doesn't exists or it returns false, JS engine throws a
> > +// DATA_CLONE_ERR DOM Exception. This method is called before any other
> 
> This isn't quite right: what happens is the StructuredCloneErrorOp callback
> is called next.

Well, not quite that either, but I cleaned this up.

> @@ +125,1 @@
> >  };
> 
> BTW, it would be OK to refactor this to be a C++ abstract base class, if
> that would make client code look nicer. Possible followup bug...

Bug 991857.

> 
> @@ +135,4 @@
> >  JS_PUBLIC_API(bool)
> >  JS_WriteStructuredClone(JSContext *cx, JS::HandleValue v, uint64_t **datap, size_t *nbytesp,
> >                          const JSStructuredCloneCallbacks *optionalCallbacks,
> >                          void *closure, JS::HandleValue transferable);
> 
> Another: make JSAutoStructuredCloneBuffer the sole API for this, and ditch
> the C stuff.

Bug 992422.

> ::: js/src/vm/StructuredClone.cpp
> @@ +141,5 @@
> >      bool writeArray(const T *p, size_t nbytes);
> >  
> > +    static void setPair(uint64_t *buffer, uint32_t tag, uint32_t data) {
> > +        *buffer = PairToUInt64(tag, data);
> > +    }
> 
> This doesn't belong in SCOutput. It's used only while reading from an
> SCInput.
> 
> How about private methods in SCInput:
> 
>     void markItemTransferred(uint64_t *itemp, uint32_t tag) {
>         MOZ_ASSERT(point <= itemp && itemp < end);
>         *itemp = PairToUint64(tag, JS::SCTAG_TMO_UNOWNED);
>     }
> 
>     void markAllTransferred(uint64_t *headerp) {
>         MOZ_ASSERT(point <= itemp && itemp < end);
>         MOZ_ASSERT(TAG(headerp) == SCTAG_TRANSFER_MAP_HEADER);
>         MOZ_ASSERT(TransferableMapHeader(DATA(headerp)) !=
> SCTAG_TM_TRANSFERRED);
>         *headerp = PairToUint64(SCTAG_TRANSFER_MAP_HEADER,
> SCTAG_TM_TRANSFERRED);
>     }
> 
> If you don't want that nicety, I think just inline setPair at the two call
> sites; in any case, not here.

I inlined it. I think it reads a little better that way, since it's more immediately obvious what the asserts are looking at.

> @@ -384,2 @@
> >  {
> > -    JS_ASSERT(nbytes % 8 == 0);
> 
> Why remove this assertion? It still has to be true, right? Particularly, an
> attacker must not be able to control the arguments here...?

Oh, sorry. I had an intermediate version where this could legitimately happen because there was another way to hand a buffer to this function. I put the assert back.

There was in interim state in this patch, where custom transferables were allowed to write stuff into the clone buffer.

> (I'm now actually unsure about the security properties we want this to have.)

I think if a clone buffer has transferables, then we have to assume the buffer hasn't been tampered with. Otherwise, we should really go back to having these things be safe.

> @@ +469,5 @@
> >  
> >  bool
> >  SCInput::getPair(uint32_t *tagp, uint32_t *datap)
> >  {
> > +    uint64_t u = 0;
> 
> Bah. Is this "= 0" necessary even with the "to shut GCC up" initialization
> in readNativeEndian? (Just curious how annoyed I should be at GCC, no action
> needed.)

It is. (Maybe it goes away in an opt build?)

> @@ +569,5 @@
> >  
> >  SCOutput::~SCOutput()
> >  {
> >      // Free any transferable data left lying around in the buffer
> > +    Discard(rawBuffer(), count() * sizeof(uint64_t), nullptr, nullptr);
> 
> Can this be right? I think it is necessary to pass correct values for
> arguments 3 and 4, since transferOwnership() can fail partway through.

No, this can't be right. It makes no sense for SCOutput to even have a finalizer. Moved to JSStructuredCloneWriter, which knows about the callbacks.

> @@ +974,5 @@
> >  
> >      for (JS::AutoObjectVector::Range tr = transferableObjects.all(); !tr.empty(); tr.popFront()) {
> > +        RootedObject obj(context(), tr.front());
> > +
> > +        obj = CheckedUnwrap(obj);
> 
> Interesting change. Please consider landing this in a separate patch, before
> everything else, with a shell-only test.

"Interesting" is right. I attempted to split this out, and quickly got totally confused about how to make an observable difference and what was going on.

It turns out that this unwrap is totally redundant. This is iterating over transferableObjects, whose contents have already been unwrapped in parseTransferable().

> @@ +976,5 @@
> > +        RootedObject obj(context(), tr.front());
> > +
> > +        obj = CheckedUnwrap(obj);
> > +        if (!obj) {
> > +            JS_ReportError(context(), "Permission denied to access object");
> 
> JS_ReportErrorNumber(context(), js_GetErrorMessage, nullptr,
> JSMSG_UNWRAP_DENIED);

Thanks. Found another one of these too.

> @@ +1540,5 @@
> > +            obj = SharedArrayBufferObject::New(context(), (SharedArrayRawBuffer *)content);
> > +        } else {
> > +            JS_ASSERT(callbacks && callbacks->readTransfer);
> > +            if (!callbacks->readTransfer(cx, this, tag, content, closure, &obj))
> > +                return false;
> 
> Please JS_ASSERT(obj) and JS_ASSERT(!cx->isExceptionPending()) after this,
> to blame the callback right away for any misbehavior.

Good idea.

> @@ +1550,3 @@
> >  
> >          if (!obj)
> >              return false;
> 
> I think the pre-existing code had the order right: try to transfer; on OOM,
> return false; then (only in the success path, but before the call to
> append()) mark the entry in the buffer as having been transferred.
> 
> This way is a leak, right?

Doh! Yes, yes it is.

Comment 55

[Steve Fink [:sfink] [:s:]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11273b06bb8d

Comment 56

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11273b06bb8d

Comment 57

[Cameron Kaiser [:spectre]]

Is this code live for certain kinds of workers? I'm getting what I suspect is an endian-related assertion on the new Google Maps; it asserts in debug builds with a null content pointer when it tries to call JS_NewArrayBufferWithContents. If I wallpaper the assertion, the browser doesn't crash, but Google Maps doesn't update anymore. However, I was under the impression that MessagePort wasn't even turned on?