Michael[tm] Smith [:sideshowbarker] Reporter
	Description • 13 years ago

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)

	David Teller [:Yoric] - still alive but not very active
	Comment 1 • 12 years ago

Do we have plans for this?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 2 • 12 years ago

Yes, I think Andrea is going to work on this. However, we need bug 720949 first.

	Andrea Marchesini [:baku] Assignee
	Comment 3 • 12 years ago

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.

	Steve Fink [:sfink] [:s:]
	Comment 4 • 12 years ago

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?

	Andrea Marchesini [:baku] Assignee
	Comment 5 • 12 years ago

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.

	Steve Fink [:sfink] [:s:]
	Comment 6 • 12 years ago

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?

	Steve Fink [:sfink] [:s:]
	Comment 7 • 12 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 8 • 12 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 9 • 12 years ago

JS_ClearStructuredBuffer used everywhere.

	Andrea Marchesini [:baku] Assignee
	Comment 10 • 12 years ago

I meant: JS_ClearStructuredBuffer is used everywhere for freeing the buffers.

	Steve Fink [:sfink] [:s:]
	Comment 12 • 12 years ago

Carrying over Martin Best's [games:p2] whiteboard annotation from bug 735474.

	Andrea Marchesini [:baku] Assignee
	Comment 13 • 12 years ago

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

	Steve Fink [:sfink] [:s:]
	Comment 14 • 12 years ago

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

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 12 years ago

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.

	Steve Fink [:sfink] [:s:]
	Comment 16 • 12 years ago

(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.

	Andrea Marchesini [:baku] Assignee
	Comment 17 • 12 years ago

> 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.

	Andrea Marchesini [:baku] Assignee
	Comment 18 • 12 years ago

Comment on attachment 658405 [details] [diff] [review]
patch 3a

Kyle can you review this patch? I want to see this code landed :) Thanks!

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 19 • 12 years ago

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?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 20 • 12 years ago

One question I do have, is it possible for the embedding to specify that objects are transferable somehow?

	Andrea Marchesini [:baku] Assignee
	Comment 21 • 12 years ago

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?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 22 • 12 years ago

I think you should check this in and talk to sfink about changing the booleans in the API to enums in another bug.

	Andrea Marchesini [:baku] Assignee
	Comment 23 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=e557dc49ee49
/me waiting for green and comments if needed.

	Andrea Marchesini [:baku] Assignee
	Comment 24 • 12 years ago

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

test_xhr_timeout failed because of the second argument of the postMessage.

	Steve Fink [:sfink] [:s:]
	Comment 25 • 12 years ago

(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)?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 26 • 12 years ago

(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).

	Andrea Marchesini [:baku] Assignee
	Comment 27 • 12 years ago

> 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.

	Steve Fink [:sfink] [:s:]
	Comment 28 • 12 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 29 • 12 years ago

take a look.

	Steve Fink [:sfink] [:s:]
	Comment 30 • 12 years ago

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)

	Andrea Marchesini [:baku] Assignee
	Comment 31 • 12 years ago

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

	Andrea Marchesini [:baku] Assignee
	Comment 32 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=699ad4790836
JSBool vs bool for some architecture breaks mochitests.

	Boris Zbarsky [:bzbarsky]
	Comment 33 • 12 years ago

> Do you have a need for it

Won't we eventually want that for Blob or File or whatever?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 34 • 12 years ago

I don't think so.  Transferring blobs/files doesn't require copying because they're immutable.

	Andrea Marchesini [:baku] Assignee
	Comment 36 • 12 years ago

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

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 38 • 12 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 39 • 12 years ago

is it enough: jsval transferable = JSVAL_VOID; to have it VOID if not set by the callee?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 40 • 12 years ago

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!

	Ryan VanderMeulen [:RyanVM]
	Comment 41 • 12 years ago

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

	Phil Ringnalda (:philor)
	Comment 42 • 12 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 43 • 12 years ago

https://tbpl.mozilla.org/?tree=Try&rev=77b0627abb44 it looks green on try.
Any hints?

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 12 years ago

I'll try pushing it again this weekend with a clobber first when things are quiet.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 45 • 12 years ago

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.

	Phil Ringnalda (:philor)
	Comment 46 • 12 years ago

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?

	Ryan VanderMeulen [:RyanVM]
	Comment 47 • 12 years ago

(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.

	Marshall Culpepper [:marshall_law]
	Comment 48 • 12 years ago

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.

	Andrea Marchesini [:baku] Assignee
	Comment 49 • 12 years ago

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?

	Kyle Huey (Exited; not receiving bugmail, old account, do not use)
	Comment 50 • 12 years ago

We probably will want to be able to transfer ImageData objects at some point.

	Andrea Marchesini [:baku] Assignee
	Comment 51 • 12 years ago

patch for RIL_worker.js

	Ryan VanderMeulen [:RyanVM]
	Comment 52 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/db347c212f80
https://hg.mozilla.org/integration/mozilla-inbound/rev/439eafb9c71e

	Ryan VanderMeulen [:RyanVM]
	Comment 53 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/db347c212f80
https://hg.mozilla.org/mozilla-central/rev/439eafb9c71e

	David Teller [:Yoric] - still alive but not very active
	Comment 54 • 12 years ago

This really needs to be documented on MDN.

	David Bruant
	Comment 55 • 12 years ago

(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.

	David Teller [:Yoric] - still alive but not very active
	Comment 56 • 12 years ago

I'll try and do that this week.

	David Teller [:Yoric] - still alive but not very active
	Comment 57 • 12 years ago

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.

	David Teller [:Yoric] - still alive but not very active
	Comment 58 • 12 years ago

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.

	David Bruant
	Comment 59 • 11 years ago

I went ahead and edited some doc related to transferable objects:
https://developer.mozilla.org/en-US/docs/DOM/Worker#postMessage%28%29
https://developer.mozilla.org/en-US/docs/DOM/Using_web_workers#Passing_data_by_transferring_.C2.A0ownership_%28transferable_objects%29
Tell me what you think.

	Andrea Marchesini [:baku] Assignee
	Comment 60 • 11 years ago

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.

	David Bruant
	Comment 61 • 11 years ago

(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.

	Masatoshi Kimura [:emk]
	Comment 62 • 11 years ago

(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.

	David Teller [:Yoric] - still alive but not very active
	Comment 63 • 11 years ago

I have seen that it is somewhat documented on MDN.