Last Comment Bug 720083 - (transferables) Workers: add support for transferable objects from HTML5 spec
(transferables)
: Workers: add support for transferable objects from HTML5 spec
Status: RESOLVED FIXED
[need review][games:p2]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla18
Assigned To: Andrea Marchesini (:baku)
:
Mentors:
: 735474 (view as bug list)
Depends on: 720949 748309 795204 795205 811050
Blocks: gecko-games 783190
  Show dependency treegraph
 
Reported: 2012-01-20 23:01 PST by Michael[tm] Smith
Modified: 2013-10-07 12:47 PDT (History)
40 users (show)
dveditz: sec‑review? (dveditz)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch 1 (43.19 KB, patch)
2012-08-31 08:13 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 2 (43.21 KB, patch)
2012-09-01 02:15 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 3 (45.57 KB, patch)
2012-09-05 00:41 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 3a (45.91 KB, patch)
2012-09-05 00:50 PDT, Andrea Marchesini (:baku)
khuey: review+
sphink: review+
Details | Diff | Review
patch 3c (38.67 KB, patch)
2012-09-26 02:35 PDT, Andrea Marchesini (:baku)
amarchesini: review+
Details | Diff | Review
patch 3d (40.02 KB, patch)
2012-09-26 07:58 PDT, Andrea Marchesini (:baku)
no flags Details | Diff | Review
patch 4 (43.88 KB, patch)
2012-09-26 15:17 PDT, Andrea Marchesini (:baku)
sphink: review+
Details | Diff | Review
patch 4b (44.95 KB, patch)
2012-09-26 23:46 PDT, Andrea Marchesini (:baku)
amarchesini: review+
Details | Diff | Review
patch 4c (45.13 KB, patch)
2012-09-27 03:36 PDT, Andrea Marchesini (:baku)
amarchesini: review+
Details | Diff | Review
patch 4d (45.12 KB, patch)
2012-09-27 08:43 PDT, Andrea Marchesini (:baku)
amarchesini: review+
Details | Diff | Review
patch 4e (45.12 KB, patch)
2012-09-27 09:13 PDT, Andrea Marchesini (:baku)
amarchesini: review+
Details | Diff | Review
patch 4f (45.43 KB, patch)
2012-09-27 09:48 PDT, Andrea Marchesini (:baku)
bent.mozilla: review+
Details | Diff | Review
RIL patch (789 bytes, patch)
2012-10-02 10:27 PDT, Andrea Marchesini (:baku)
bent.mozilla: review+
Details | Diff | Review

Description Michael[tm] Smith 2012-01-20 23:01:08 PST
http://dev.w3.org/html5/spec/common-dom-interfaces.html#transferable-objects

https://developer.mozilla.org/En/Using_web_workers#Passing_data_by_transferring_.C2.A0ownership_(transferable_objects)

"additional way to pass data to/from a worker using Transferable Objects with high performance. With transferable objects, data is transferred from one context to another with a zero-copy operation. This means a vast performance improvement when sending large data. Think of it as pass-by-reference if you're from the C/C++ world. However, unlike pass-by-reference, the 'version' from the calling context is no longer available once transferred. It's ownership is transferred to the new context."

Supported in Chrome 17 (under a prefixed webkitPostMessage method for now)
Comment 1 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2012-08-12 05:14:04 PDT
Do we have plans for this?
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-08-12 09:44:29 PDT
Yes, I think Andrea is going to work on this. However, we need bug 720949 first.
Comment 3 Andrea Marchesini (:baku) 2012-08-31 08:13:34 PDT
Created attachment 657289 [details] [diff] [review]
patch 1

This patch changes many things:

1. PostSyncMessage and PostMessage have an optional argument that is an array of arraybuffers. This is the transferable array object.
http://www.whatwg.org/specs/web-apps/current-work/multipage/web-messaging.html#posting-messages
http://updates.html5rocks.com/2011/12/Transferable-Objects-Lightning-Fast

2. JSStructuredCloneBuffer supports the transferable array.

- it validates the jsval transferable array object and if something goes wrong, it uses reportError() callback with errorId == JS_SCERR_TRANSFERABLE

- the buffer created by the JSStructuredCloneBuffer starts with a list of  SCTAG_TRANSFER_MAP (only if the buffer contains some transferred object).
**This makes easier to know if the buffer contains transferable objects**

- The algorithm works as described here: http://dev.w3.org/html5/spec/common-dom-interfaces.html#transferable-object

- A JSStructuredCloneBuffer with transferred objects cannot be copied.

- Multiple read() calls are not allowed for JSStructuredCloneBuffer with transferred objects

- The memory is deallocated by the JSStructuredCloneBuffer destructor if any read() is called.

This patch builds on top of https://bugzilla.mozilla.org/show_bug.cgi?id=720949

Note: I'm not sure about writePtr/readPtr for 32/64bits and bit/little endian byte order.
Comment 4 Steve Fink [:sfink] [:s:] 2012-08-31 16:07:13 PDT
Comment on attachment 657289 [details] [diff] [review]
patch 1

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

::: dom/base/nsStructuredCloneContainer.cpp
@@ +62,2 @@
>    bool success = JS_WriteStructuredClone(aCx, jsData, &jsBytes, &mSize,
> +                                           nullptr, nullptr, transferable);

Why not pass in JSVAL_VOID directly?

::: dom/workers/test/sync_transferable_worker.js
@@ +5,5 @@
> +
> +onmessage = function(event) {
> +  if (event.data.data == 0) {
> +    try {
> +      ab = new ArrayBuffer(128);

Can you test very small buffers (eg 4 bytes) too? They'll start out with their data inline. (To be fair, it would be testing the transferable typedarray JSAPI, not the cloning.)

::: js/src/jsapi.cpp
@@ +6482,5 @@
>  bool
>  JSAutoStructuredCloneBuffer::copy(const uint64_t *srcData, size_t nbytes, uint32_t version)
>  {
> +    // transferable objects cannot be copied
> +    if (data_ && StructuredCloneHasTransferObjects(data_, nbytes_)) {

no curly brackets here

If you're checking data_ here instead of inside StructuredCloneHasTransferObjects, it seems like you'd want to check whether nbytes_ is big enough too.

::: js/src/jsclone.cpp
@@ +217,5 @@
> +bool
> +SCInput::getPair(uint32_t *tagp, uint32_t *datap)
> +{
> +    uint64_t u = 0;     /* initialize to shut GCC up */
> +    bool ok = get(&u);

I think in spidermonkey-land, you'd spell this

  uint64_t u;
  if (!get(&u))
    return false;
  ...
  return true;

@@ +421,5 @@
> +JSStructuredCloneWriter::parseTransferable()
> +{
> +    transferableObjects.clear();
> +
> +    if (!JSVAL_IS_NULL(transferable) && !JSVAL_IS_VOID(transferable)) {

Similarly, just do an |if (null or void) return true| early exit.

@@ +427,5 @@
> +            reportErrorTransferable();
> +            return false;
> +        }
> +
> +        JSObject* array = JSVAL_TO_OBJECT(transferable);

array = &transferable.toObject()

@@ +439,5 @@
> +            return false;
> +        }
> +
> +        for (uint32_t i = 0; i < length; ++i) {
> +            jsval v;

Value v;

@@ +449,5 @@
> +                reportErrorTransferable();
> +                return false;
> +            }
> +
> +            JSObject* tObj = JSVAL_TO_OBJECT(v);

&v.toObject()

@@ +450,5 @@
> +                return false;
> +            }
> +
> +            JSObject* tObj = JSVAL_TO_OBJECT(v);
> +            if (!tObj || !tObj->isArrayBuffer()) {

!tObj is unnecessary; v.isObject() doesn't allow NULL (unlike the old JSVAL_IS_OBJECT).

@@ +455,5 @@
> +                reportErrorTransferable();
> +                return false;
> +            }
> +
> +            // No duplicate:

Ugh. This really wants to be a map. It probably wouldn't even be much more code. And jsclone already has CloneMemory to use as an example, though you'd probably want HashSet instead of HashMap.

@@ +574,5 @@
> +
> +    // Transferable Array Buffer:
> +    if (!transferableObjects.empty()) {
> +        CloneMemory::AddPtr p = memory.lookupForAdd(obj);
> +        if (p) {

no brackets for single-line consequents in js/src

@@ +687,5 @@
> +        if (!memory.put(*begin, memory.count()))
> +            return false;
> +
> +        uint8_t *content;
> +        if (!JS_StealArrayBufferContents(context(), *begin, &content))

I changed this parameter to a void** as a result of luke's review, so content can now be void*.

@@ +1059,5 @@
>  bool
> +JSStructuredCloneReader::readTransferMap()
> +{
> +    uint32_t tag, data;
> +    while (in.getPair(&tag, &data) && tag == SCTAG_TRANSFER_MAP) {

readTransferMap() returns true if in.getPair() fails. Seems wrong.

@@ +1060,5 @@
> +JSStructuredCloneReader::readTransferMap()
> +{
> +    uint32_t tag, data;
> +    while (in.getPair(&tag, &data) && tag == SCTAG_TRANSFER_MAP) {
> +        uint8_t *content;

void*

::: js/src/shell/js.cpp
@@ +3427,5 @@
>      jsval v = argc > 0 ? JS_ARGV(cx, vp)[0] : JSVAL_VOID;
>      uint64_t *datap;
>      size_t nbytes;
> +    jsval t(JSVAL_VOID);
> +    if (!JS_WriteStructuredClone(cx, v, &datap, &nbytes, NULL, NULL, t))

Why the separate variable? Why not pass in JSVAL_VOID directly?
Comment 5 Andrea Marchesini (:baku) 2012-09-01 02:15:07 PDT
Created attachment 657543 [details] [diff] [review]
patch 2

This new patch does:

1. test for 1024*1024*32, 128 and 4 bytes

2. HashSet instead Vector

3. Any other comment from Steve's review

4. the previous patch was green on try. I'll post the results of this new one soon.
Comment 6 Steve Fink [:sfink] [:s:] 2012-09-04 11:54:59 PDT
Comment on attachment 657543 [details] [diff] [review]
patch 2

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

Everything looks good except for apparently pre-existing problems, and you don't have to fix those if you're not making them worse (though you're welcome to!), but I can't r+ the |if (!transferableObject.empty())| until I understand what it's supposed to be doing. So perhaps just an explanation is required for that part.

::: js/src/jsapi.h
@@ +5849,5 @@
>  /* Note: On success, the caller is responsible for calling js::Foreground::free(*datap). */
>  JS_PUBLIC_API(JSBool)
>  JS_WriteStructuredClone(JSContext *cx, jsval v, uint64_t **datap, size_t *nbytesp,
>                          const JSStructuredCloneCallbacks *optionalCallbacks,
> +                        void *closure, jsval transferable);

Don't you need a JS_ClearStructuredClone to avoid memory leaks when handling errors? You have JSAutoStructuredClone::clear(), but how do you free transferables within clone buffers when you're not using that class?

::: js/src/jsclone.cpp
@@ +465,5 @@
> +            reportErrorTransferable();
> +            return false;
> +        }
> +
> +        transferableObjects.putNew(tObj);

This is not infallible, so you'll need to check the return value.

@@ +572,5 @@
>      ArrayBufferObject &buffer = obj->asArrayBuffer();
> +
> +    // Transferable Array Buffer:
> +    if (!transferableObjects.empty()) {
> +        CloneMemory::AddPtr p = memory.lookupForAdd(obj);

You're not adding, right? Seems like this should be

  CloneMemory::Ptr p = memory.lookup(obj);

But also, I'm confused by the logic here. Why check !transferableObjects.empty()? If you have a non-transferred ArrayBuffer, you'd still want to do the |memory| check, no? Only you're also not adding objects to |memory| anywhere here like::startObject does; why not? Shouldn't multiple references to the same ArrayBuffer end up pointing to the same thing? (This question is also valid for the code before your change, btw.) And if transferrable array buffers should indeed be treated differently, why aren't you doing transferableObjects.has(obj) instead of just checking whether anything at all is supposed to be transferred?

Same problem with typed arrays. Structured cloning just seems broken (before this patch) for array buffers and typed arrays. Not to mention that DataViews are treated as plain objects... argh.

@@ +993,5 @@
>        }
>  
> +      case SCTAG_TRANSFER_MAP:
> +        // A map cannot be here but just at the beginning of the buffer.
> +        JS_ASSERT(false);

An assertion seems extreme for actual use. Shouldn't this just be reported as bad serialized data?
Comment 7 Steve Fink [:sfink] [:s:] 2012-09-04 13:16:58 PDT
The pre-existing problem with backreferences is bug 748309. Seems like there was a spec change that added the dag/cycle handling, and our implementation was never updated.
Comment 8 Andrea Marchesini (:baku) 2012-09-05 00:41:56 PDT
Created attachment 658401 [details] [diff] [review]
patch 3

2 new functions should fix the problem for the allocated memory.

/* Note: set freeTransferable to true if the data has never been read */      
JSBool JS_ClearStructuredClone(const uint64_t *data, size_t nbytes, JSBool freeTransferable);
                                                         
JSBool JS_StructuredCloneHasTransferables(const uint64_t *data, size_t nbytes,JSBool *hasTransferable);

For the other bug, I prefer to fix it in a separated patch.
Comment 9 Andrea Marchesini (:baku) 2012-09-05 00:50:28 PDT
Created attachment 658405 [details] [diff] [review]
patch 3a

JS_ClearStructuredBuffer used everywhere.
Comment 10 Andrea Marchesini (:baku) 2012-09-05 05:26:34 PDT
I meant: JS_ClearStructuredBuffer is used everywhere for freeing the buffers.
Comment 11 Andrea Marchesini (:baku) 2012-09-05 08:20:56 PDT
*** Bug 735474 has been marked as a duplicate of this bug. ***
Comment 12 Steve Fink [:sfink] [:s:] 2012-09-05 09:26:04 PDT
Carrying over Martin Best's [games:p2] whiteboard annotation from bug 735474.
Comment 13 Andrea Marchesini (:baku) 2012-09-10 06:10:46 PDT
I wrote a demo/test/benchmark: https://people.mozilla.com/~amarchesini/code/transferrable/

I think the performances are good:

transferrable: postMessage roundtrip took: 29 ms
transferrable: postMessage roundtrip rate: 2207 MB/s
non-transferrable: postMessage roundtrip took: 127 ms
non-transferrable: postMessage roundtrip rate: 504 MB/s
Comment 14 Steve Fink [:sfink] [:s:] 2012-09-13 21:09:49 PDT
Comment on attachment 658405 [details] [diff] [review]
patch 3a

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

There are two bugs that this is related to, but it looks like the performance hit is going to be avoidable (bug 789295) and I can fix up the object cloning after this lands (bug 748309), so I see no reason not to let this land. Sorry for the delay.

::: js/src/jsclone.cpp
@@ +132,5 @@
> +{
> +    SCInput in(cx, data, nbytes);
> +
> +    /* XXX disallow callers from using internal pointers to GC things. */
> +    SkipRoot skip(cx, &in);

Ouch. Properly rooting this is going to be a pain.

Not your problem, though; this is preexisting.

@@ +582,5 @@
> +        CloneMemory::Ptr p = memory.lookup(obj);
> +        if (p)
> +            return out.writePair(SCTAG_BACK_REFERENCE_OBJECT, p->value);
> +    }
> +

Ok, with the understanding that the current code barely handles repeated objects at all currently, this makes sense.

@@ +1002,5 @@
> +        // A map cannot be here but just at the beginning of the buffer.
> +        JS_ReportErrorNumber(context(), js_GetErrorMessage, NULL,
> +                             JSMSG_SC_BAD_SERIALIZED_DATA,
> +                             "invalid input");
> +        break;

return false
Comment 15 Andrea Marchesini (:baku) 2012-09-14 02:27:37 PDT
Thanks for the review.
We cannot land this patch yet because it's based on Bug 748309 - Typed Array structured clone doesn't do back-references.

If you think it's OK, I can rebase this patch without including your patch for 748309. Let me know.
Comment 16 Steve Fink [:sfink] [:s:] 2012-09-14 10:03:08 PDT
(In reply to Andrea Marchesini (:baku) from comment #15)
> Thanks for the review.
> We cannot land this patch yet because it's based on Bug 748309 - Typed Array
> structured clone doesn't do back-references.
> 
> If you think it's OK, I can rebase this patch without including your patch
> for 748309. Let me know.

Oh. I have a newer version of that patch that seems to work, unlike the one currently attached. But I'm still working on modifying the tests for it -- they're still testing the old behavior.

I am fine with you rebasing this patch so that it is not on top of 748309, but that shouldn't affect bent's review. I think we'll have to race -- if you get bent's review first, then you can rebase and land. If I both finish bug 748309 and get review on it first, I'll land that and you can rebase on top of it. Does that work?

Regardless, I'll update the patch in bug 748309 right now.
Comment 17 Andrea Marchesini (:baku) 2012-09-14 23:09:51 PDT
> I am fine with you rebasing this patch so that it is not on top of 748309,
> but that shouldn't affect bent's review. I think we'll have to race -- if
> you get bent's review first, then you can rebase and land. If I both finish
> bug 748309 and get review on it first, I'll land that and you can rebase on
> top of it. Does that work?

Sounds like a plan.
Comment 18 Andrea Marchesini (:baku) 2012-09-20 10:18:28 PDT
Comment on attachment 658405 [details] [diff] [review]
patch 3a

Kyle can you review this patch? I want to see this code landed :) Thanks!
Comment 19 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-25 20:29:56 PDT
Comment on attachment 658405 [details] [diff] [review]
patch 3a

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

r=me on the worker bits.

::: dom/workers/Worker.cpp
@@ +290,5 @@
>      if (!JS_ConvertArguments(aCx, aArgc, JS_ARGV(aCx, aVp), "v", &message)) {
>        return false;
>      }
>  
> +    // FIXME: the targetOrigin (argv[1]) is currently ignored

That seems bad.  Is there a bug on file for this?

::: js/src/jsapi.h
@@ +5856,5 @@
> +                        void *closure, jsval transferable);
> +
> +/* Note: set freeTransferable to true if the data has never been read */
> +JS_PUBLIC_API(JSBool)
> +JS_ClearStructuredClone(const uint64_t *data, size_t nbytes, JSBool freeTransferable);

I'm not a JS peer, but magic booleans are kind of annoying in APIs, since you end up having to look up the API to figure out what the boolean means.  Could we make this an enum instead?
Comment 20 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-25 20:30:35 PDT
One question I do have, is it possible for the embedding to specify that objects are transferable somehow?
Comment 21 Andrea Marchesini (:baku) 2012-09-26 02:35:25 PDT
Created attachment 664858 [details] [diff] [review]
patch 3c

yes, it's possible do specify which object you want to have transferred:

var ab = new ArrayBuffer(42);
var ab2 = new ArrayBuffer(42);
postMessage({a: ab, b: ab2 }, [ab]);

just ab will be transferred.

Can I mark this patch to be checked in? Or should I ask for another review?
Comment 22 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-26 06:11:16 PDT
I think you should check this in and talk to sfink about changing the booleans in the API to enums in another bug.
Comment 23 Andrea Marchesini (:baku) 2012-09-26 06:19:32 PDT
https://tbpl.mozilla.org/?tree=Try&rev=e557dc49ee49
/me waiting for green and comments if needed.
Comment 24 Andrea Marchesini (:baku) 2012-09-26 07:58:52 PDT
Created attachment 664963 [details] [diff] [review]
patch 3d

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

test_xhr_timeout failed because of the second argument of the postMessage.
Comment 25 Steve Fink [:sfink] [:s:] 2012-09-26 10:32:07 PDT
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #20)
> One question I do have, is it possible for the embedding to specify that
> objects are transferable somehow?

(In reply to Andrea Marchesini (:baku) from comment #21)
> Created attachment 664858 [details] [diff] [review]
> patch 3c
> 
> yes, it's possible do specify which object you want to have transferred:
> 
> var ab = new ArrayBuffer(42);
> var ab2 = new ArrayBuffer(42);
> postMessage({a: ab, b: ab2 }, [ab]);
> 
> just ab will be transferred.

Was that you were asking, khuey? I thought you were asking if there's a general mechanism for an embedding to label certain types of objects as Transferable, and therefore eligible for inclusion in the 2nd argument to postMessage(). The answer is no, currently; the ArrayBuffer handling is currently hardcoded into the clone algorithm. I guess we'd need to add another function pointer or 3 to JSClass or something if we wanted to generalize it. I don't see that happening until we have a need for it. Do you have a need for it, or am I just making up my own questions here? :-)

baku: have you rebased this on top of bug 748309? I won the race, and I thought I remembered that this patch would need to change to accommodate it. I want to look at this again, at any rate -- can you flag me for re-review once you've rebased on bug 748309 (or confirmed that you already have)?
Comment 26 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-26 10:34:59 PDT
(In reply to Steve Fink [:sfink] from comment #25)
> (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #20)
> > One question I do have, is it possible for the embedding to specify that
> > objects are transferable somehow?
> 
> (In reply to Andrea Marchesini (:baku) from comment #21)
> > Created attachment 664858 [details] [diff] [review]
> > patch 3c
> > 
> > yes, it's possible do specify which object you want to have transferred:
> > 
> > var ab = new ArrayBuffer(42);
> > var ab2 = new ArrayBuffer(42);
> > postMessage({a: ab, b: ab2 }, [ab]);
> > 
> > just ab will be transferred.
> 
> Was that you were asking, khuey? I thought you were asking if there's a
> general mechanism for an embedding to label certain types of objects as
> Transferable, and therefore eligible for inclusion in the 2nd argument to
> postMessage(). The answer is no, currently; the ArrayBuffer handling is
> currently hardcoded into the clone algorithm. I guess we'd need to add
> another function pointer or 3 to JSClass or something if we wanted to
> generalize it. I don't see that happening until we have a need for it. Do
> you have a need for it, or am I just making up my own questions here? :-)

Yes, you are correct about the question I posed.  I don't have a need for it, I was just wondering, since handling Gecko objects in the structured clone hooks requires a bunch of logic (to ensure that they live long enough, that we delete them when they're no longer needed, etc).
Comment 27 Andrea Marchesini (:baku) 2012-09-26 12:23:38 PDT
> baku: have you rebased this on top of bug 748309? I won the race, and I
> thought I remembered that this patch would need to change to accommodate it.
> I want to look at this again, at any rate -- can you flag me for re-review
> once you've rebased on bug 748309 (or confirmed that you already have)?

I confirm this patch has been rebased to the current central repo.
Comment 28 Steve Fink [:sfink] [:s:] 2012-09-26 13:36:07 PDT
Comment on attachment 664963 [details] [diff] [review]
patch 3d

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

::: dom/base/nsStructuredCloneContainer.cpp
@@ +63,5 @@
>    NS_ENSURE_STATE(success);
>    NS_ENSURE_STATE(jsBytes);
>  
>    // Copy jsBytes into our own buffer.
>    mData = (uint64_t*) malloc(mSize);

Nothing to do with your patch, I'm just curious:

Yuck! Is this really necessary? Is this just to avoid mixing up allocators/deallocators? If so, we probably ought to file a bug to pass in an allocator or something.

@@ +68,5 @@
>    if (!mData) {
>      mSize = 0;
>      mVersion = 0;
>  
> +    JS_ClearStructuredClone(jsBytes, mSize, false);

(hm, Splinter review doesn't seem to give me an option for commenting on a file, just a chunk)

The error handling in nsStructuredCloneContainer::DeserializeToVariant leaks. I think that instead of just NS_ENSURE_STATE(success), you need to free the structured clone data on failure.

Also, see the comments in jsapi.h. I think you want to leave these as JS_free.

::: js/src/jsapi.h
@@ +2038,5 @@
>   * This is called when JS_WriteStructuredClone finds that the object to be
> + * written is recursive or when the transferable object is not valid.
> + * To follow HTML5, the application must throw a
> + * DATA_CLONE_ERR DOMException. errorid is always JS_SCERR_RECURSION or
> + * JS_SCERR_TRANSFERABLE.

Oops, I should have removed this. Can you change the comment to just say:

This is called when JS_WriteStructuredClone is given an invalid transferable. To follow HTML5, the application must throw a DATA_CLONE_ERR DOMException with error set to one of the JS_SCERR_* values.

@@ +5718,5 @@
> +                        void *closure, jsval transferable);
> +
> +/* Note: set freeTransferable to true if the data has never been read */
> +JS_PUBLIC_API(JSBool)
> +JS_ClearStructuredClone(const uint64_t *data, size_t nbytes, JSBool freeTransferable);

Sorry, I hadn't looked closely at this before. This seems error-prone.

Initially, I couldn't figure out why you needed to pass in freeTransferable; shouldn't the buffer remember whether it has been read yet? But I guess this is for where you JS_WriteStructuredClone and make a copy of the resulting bytes -- you don't want to free the transferables when you deallocate the old buffer.

I think I would prefer if you removed this parameter and made the buffer remember whether it has been read yet, and use a plain JS_free on the clone buffer that you copied from. (Ok, I would *really* prefer if that memcpy were just nuked, but I wouldn't bog down this bug for that.)

Unless there's something I'm missing, which is totally possible? Let me know what you think.

::: js/src/jsclone.cpp
@@ +1093,5 @@
> +            return false;
> +
> +        JSObject *obj = JS_NewArrayBufferWithContents(context(), content);
> +        if (!obj || !allObjs.append(ObjectValue(*obj)))
> +            return false;

Yay, this is much nicer now that back references are handled properly so you don't have to treat the transferables differently.
Comment 29 Andrea Marchesini (:baku) 2012-09-26 15:17:17 PDT
Created attachment 665150 [details] [diff] [review]
patch 4

take a look.
Comment 30 Steve Fink [:sfink] [:s:] 2012-09-26 16:04:17 PDT
Comment on attachment 665150 [details] [diff] [review]
patch 4

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

::: dom/base/nsStructuredCloneContainer.cpp
@@ +68,5 @@
>    if (!mData) {
>      mSize = 0;
>      mVersion = 0;
>  
> +    JS_ClearStructuredClone(jsBytes, mSize);

I'm not sure whether this would be better as JS_free or JS_ClearStructuredClone, but I guess this is fine.

Can you add a MOZ_ASSERT that mData doesn't contain transferables somewhere? Both in the destructor and after the JS_ReadStructuredClone in DeserializeToVariant would be good, to avoid someone else haivng to figure out why it's ok. It's nonlocal knowledge where something like nsStructuredCloneContainer::InitFromBase64 could get its data.

::: js/src/jsclone.cpp
@@ +159,5 @@
> +                uint64_t u = SwapBytes(*point++);
> +                uint32_t tag = uint32_t(u >> 32);
> +                if (tag == SCTAG_TRANSFER_MAP) {
> +                    u = SwapBytes(*point++);
> +                    js_free((void *)u);

reinterpret_cast<void*>(u)

@@ +438,5 @@
>  
>  bool
> +SCOutput::writePtr(const void *p)
> +{
> +    return write((uint64_t)p);

reinterpret_cast<uint64_t>(p)
Comment 31 Andrea Marchesini (:baku) 2012-09-26 23:46:20 PDT
Created attachment 665274 [details] [diff] [review]
patch 4b

https://tbpl.mozilla.org/?tree=Try&rev=59ed69e26d92
Comment 32 Andrea Marchesini (:baku) 2012-09-27 03:36:07 PDT
Created attachment 665331 [details] [diff] [review]
patch 4c

https://tbpl.mozilla.org/?tree=Try&rev=699ad4790836
JSBool vs bool for some architecture breaks mochitests.
Comment 33 Boris Zbarsky [:bz] 2012-09-27 08:31:42 PDT
> Do you have a need for it

Won't we eventually want that for Blob or File or whatever?
Comment 34 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-09-27 08:33:12 PDT
I don't think so.  Transferring blobs/files doesn't require copying because they're immutable.
Comment 35 Andrea Marchesini (:baku) 2012-09-27 08:43:14 PDT
Created attachment 665484 [details] [diff] [review]
patch 4d
Comment 36 Andrea Marchesini (:baku) 2012-09-27 08:44:13 PDT
https://tbpl.mozilla.org/?tree=Try&rev=699ad4790836
Comment 37 Andrea Marchesini (:baku) 2012-09-27 09:13:22 PDT
Created attachment 665500 [details] [diff] [review]
patch 4e
Comment 38 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-09-27 09:18:39 PDT
Comment on attachment 665500 [details] [diff] [review]
patch 4e

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

::: dom/workers/Worker.cpp
@@ +289,5 @@
>      if (!JS_ConvertArguments(aCx, aArgc, JS_ARGV(aCx, aVp), "v", &message)) {
>        return false;
>      }
>  
> +    jsval transferable = JSVAL_VOID;

This should be wrapped into the JS_ConvertArguments call.

::: dom/workers/WorkerScope.cpp
@@ +842,5 @@
>      if (!JS_ConvertArguments(aCx, aArgc, JS_ARGV(aCx, aVp), "v", &message)) {
>        return false;
>      }
>  
> +    jsval transferable = JSVAL_VOID;

Here too.
Comment 39 Andrea Marchesini (:baku) 2012-09-27 09:48:00 PDT
Created attachment 665512 [details] [diff] [review]
patch 4f

is it enough: jsval transferable = JSVAL_VOID; to have it VOID if not set by the callee?
Comment 40 Ben Turner (not reading bugmail, use the needinfo flag!) 2012-09-27 09:49:36 PDT
Comment on attachment 665512 [details] [diff] [review]
patch 4f

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

I only looked at the JS_ConvertArguments changes, but r=me on that!
Comment 41 Ryan VanderMeulen [:RyanVM] 2012-09-27 20:21:31 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/b24a73b2c617
Comment 42 Phil Ringnalda (:philor) 2012-09-27 21:35:27 PDT
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9600a66b7bcc - the Windows build bustage in https://tbpl.mozilla.org/php/getParsedLog.php?id=15612554&tree=Mozilla-Inbound may just mean that the build system is busted like usual, and you'll have to land with a clobber; the assertion and failure in https://tbpl.mozilla.org/php/getParsedLog.php?id=15612800&tree=Mozilla-Inbound looks pretty directly related.
Comment 43 Andrea Marchesini (:baku) 2012-09-28 05:09:42 PDT
https://tbpl.mozilla.org/?tree=Try&rev=77b0627abb44 it looks green on try.
Any hints?
Comment 44 Ryan VanderMeulen [:RyanVM] 2012-09-28 05:57:28 PDT
I'll try pushing it again this weekend with a clobber first when things are quiet.
Comment 45 Jonas Sicking (:sicking) 2012-09-28 10:01:53 PDT
Actually, this is causing bug 795205, so please don't just reland as-is.

Andrea: Any ideas why bug 795205 happened? I wouldn't have thought that any existing code would be affected by this patch.
Comment 46 Phil Ringnalda (:philor) 2012-09-28 10:18:43 PDT
Among the many possibilities is the fact that we know it wasn't properly built as a dep build on one platform, and it's entirely possible that it wasn't properly built as a dep build on other platforms, it just didn't happen to cause the build to fail elsewhere. Does a clobber b2g build still hit bug 795205?
Comment 47 Ryan VanderMeulen [:RyanVM] 2012-09-28 10:31:25 PDT
(In reply to Andrea Marchesini (:baku) from comment #43)
> https://tbpl.mozilla.org/?tree=Try&rev=77b0627abb44 it looks green on try.

Jonas or Chris, you can you try this build out and see if it fails the same? Try builds are always clobbers.
Comment 48 Marshall Culpepper [:marshall_law] 2012-09-29 10:58:53 PDT
FWIW, I just ran a B2G clobber build on mozilla-central with the attached patch, and it looks like RIL is failing. Not sure if this patch is the cause though, I know Kyle Machulis was looking into / fixed some other RIL bug last night as well.

Some relevant messages I saw in logcat:

09-29 17:31:27.674   106      236           Gecko  I  RIL Worker: RIL Worker errorundefined
09-29 17:31:37.433   106      106    GeckoConsole  E  [JavaScript Error: "TypeError: conn.voice.network is null" {file: "app://sms.gaiamobile.org/js/background.js" line: 11}] 

Entire logcat link:
http://pastebin.mozilla.org/1849876

CCing Kyle in case he knows what's going on.
Comment 49 Andrea Marchesini (:baku) 2012-10-01 06:54:26 PDT
postMessage for workers doesn't have the origin target param.
Line: 3709 file: ril_worker.js - postMessage(message, "*") -> it should be postMessage(message)

Can we try to apply the patch again with this line changed?
Comment 50 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-10-01 12:44:20 PDT
We probably will want to be able to transfer ImageData objects at some point.
Comment 51 Andrea Marchesini (:baku) 2012-10-02 10:27:03 PDT
Created attachment 667045 [details] [diff] [review]
RIL patch

patch for RIL_worker.js
Comment 54 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-15 03:36:27 PST
This really needs to be documented on MDN.
Comment 55 David Bruant 2013-01-15 03:45:00 PST
(In reply to David Rajchenbach Teller [:Yoric] from comment #54)
> This really needs to be documented on MDN.
Agreed, it's a very important capability of the web platform.
Do you feel like you're capable of documenting it? If so, I can provide help and review. The upcoming doc sprint (~Feb 9th I heard) may be a good opportunity to do this.
Comment 56 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-15 03:45:47 PST
I'll try and do that this week.
Comment 57 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-15 05:27:08 PST
I may be wrong, but the tests don't seem to test the spec, and I have the impression that the implementation also doesn't quite implement it.

According to the spec, we should accept as second argument to |postMessage| an association list from transferable objects to substitution values, i.e. an array of arrays. Here, we test (and I believe we implement) a method that accepts a list of objects to be transferred. That looks fishy.
Comment 58 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-01-15 05:52:46 PST
Ah, looks like it's just a case of obfuscated specifications. The whatwg specs are slightly clearer insofar as they initialize the "transfer map" from an array provided by the user.
Comment 60 Andrea Marchesini (:baku) 2013-02-15 08:01:21 PST
Thank you for the documentation. Only a small issue:
"According to the spec, only MessagePort, ArrayBuffers and CanvasProxy objects can be transferred." Right. But we support only ArrayBuffers right now.
Comment 61 David Bruant 2013-02-15 08:15:54 PST
(In reply to Andrea Marchesini (:baku) from comment #60)
> Thank you for the documentation.
Most of it was already written ;-)

> Only a small issue:
> "According to the spec, only MessagePort, ArrayBuffers and CanvasProxy
> objects can be transferred." Right. But we support only ArrayBuffers right
> now.
WebKit does implement MessagePort as Transferable (and Chrome supports it), so that's valuable information.
CanvasProxy is spec-only at this point... There is a stub on Webkit, but apparently, it's not there yet. So you're right, I'm removing that from the doc.
Comment 62 Masatoshi Kimura [:emk] 2013-02-15 09:21:59 PST
(In reply to David Bruant from comment #61)
> (In reply to Andrea Marchesini (:baku) from comment #60)
> > Thank you for the documentation.
> Most of it was already written ;-)
> 
> > Only a small issue:
> > "According to the spec, only MessagePort, ArrayBuffers and CanvasProxy
> > objects can be transferred." Right. But we support only ArrayBuffers right
> > now.
> WebKit does implement MessagePort as Transferable (and Chrome supports it),
> so that's valuable information.
> CanvasProxy is spec-only at this point... There is a stub on Webkit, but
> apparently, it's not there yet. So you're right, I'm removing that from the
> doc.

A description about browser support should be added instead of deleting the text.
Comment 63 David Rajchenbach-Teller [:Yoric] (please use "needinfo") 2013-10-07 12:33:40 PDT
I have seen that it is somewhat documented on MDN.

Note You need to log in before you can comment on or make changes to this bug.