Last Comment Bug 822094 - Implement transfer parameter of window.postMessage
: Implement transfer parameter of window.postMessage
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla20
Assigned To: Masatoshi Kimura [:emk]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-12-16 05:12 PST by Masatoshi Kimura [:emk]
Modified: 2013-08-10 02:16 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Unwrap the transfable parameter if allowed (1.02 KB, patch)
2012-12-16 07:08 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 2: Add transfer parameter to window.postMessage (6.82 KB, patch)
2012-12-16 07:09 PST, Masatoshi Kimura [:emk]
jonas: review+
Details | Diff | Splinter Review
Part 1: Unwrap the transfable parameter if allowed, v2 (971 bytes, patch)
2012-12-17 13:34 PST, Masatoshi Kimura [:emk]
sphink: review+
Details | Diff | Splinter Review
Part 2: Add transfer parameter to window.postMessage. r=jonas (6.81 KB, patch)
2012-12-17 13:36 PST, Masatoshi Kimura [:emk]
VYV03354: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2012-12-16 05:12:01 PST
I'm a bit surprised that Firefox doesn't support it yet.
Comment 1 Masatoshi Kimura [:emk] 2012-12-16 07:08:55 PST
Created attachment 692733 [details] [diff] [review]
Part 1: Unwrap the transfable parameter if allowed

We can't post transferable to other window without this fix.
Comment 2 Masatoshi Kimura [:emk] 2012-12-16 07:09:28 PST
Created attachment 692734 [details] [diff] [review]
Part 2: Add transfer parameter to window.postMessage
Comment 3 Masatoshi Kimura [:emk] 2012-12-16 07:09:53 PST
https://tbpl.mozilla.org/?tree=Try&rev=65c7343e374c
Comment 4 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-12-17 10:05:10 PST
Comment on attachment 692733 [details] [diff] [review]
Part 1: Unwrap the transfable parameter if allowed

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

::: js/src/jsclone.cpp
@@ +490,5 @@
>  
>          JSObject* tObj = &v.toObject();
> +        // If the object is a security wrapper, see if we're allowed to unwrap it.
> +        // If we aren't, throw.
> +        if (tObj->isWrapper())

Don't bother with the comment or the isWrapper() call. Just do

  JSObject* tObj = UnwrapObjectChecked(&v.toObject());

and do your null check. (The v.isObject() above already guarantees that the original object is non-null.)

Also, if the security check fails, it would be more in line with existing code to do

            JS_ReportError(context(), "Permission denied to access object");

instead of reportErrorTransferable(). (I'm not saying the existing code is right, just that I'd rather match it for now.)
Comment 5 Masatoshi Kimura [:emk] 2012-12-17 13:34:56 PST
Created attachment 693089 [details] [diff] [review]
Part 1: Unwrap the transfable parameter if allowed, v2

Resolved review comments.
Comment 6 Masatoshi Kimura [:emk] 2012-12-17 13:36:09 PST
Created attachment 693090 [details] [diff] [review]
Part 2: Add transfer parameter to window.postMessage. r=jonas

Patch for check-in.
I forgot updating the bug number after copy-and-paste, so I've done it now.
Comment 9 Kevin Ring 2012-12-21 10:57:02 PST
Apologies if this is already old news, but as of Aurora 19.0a2 (2012-12-20), passing an ArrayBuffer in the transfer parameter array neuters the ArrayBuffer but also passes a neutered version to the other thread.  I have a simple test case for this, and can attach it to this issue or as a separate one if that would be helpful.
Comment 10 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-12-21 13:15:55 PST
This is known (will be fixed by bug 789593) if you're passing a typed array with an ArrayBuffer in the transfer parameter array. It is not known if no typed arrays (or any kind of ArrayBufferView) is involved. If that is the case, can you give me a test case? I don't know how to set up worker-y stuff.

Thanks!
Comment 11 Kevin Ring 2012-12-21 13:21:02 PST
My test case passes a Float32Array with the arrayInstance.buffer in the transferable object array.  So it sounds like it's a known issue.  Thanks!
Comment 12 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2012-12-21 13:47:09 PST
It's a known issue, but nobody had actually needed to do it yet, so it's been languishing as a mentored bug for quite a while. Let me know if you need this to work, and I can prioritize that bug higher (i.e., give up and fix it myself.)
Comment 13 Kevin Ring 2012-12-21 18:22:40 PST
Transferable objects are a pretty big performance win in our app, but I think we should be able to pass the ArrayBuffer instead of the wrapping Float32Array as a fairly painless workaround.  If so, there's no real hurry from me.  I'll let you know of that doesn't work out for some reason.  Thanks again.
Comment 14 Jasper 2013-03-01 10:20:36 PST
(In reply to Steve Fink [:sfink] from comment #12)
> It's a known issue, but nobody had actually needed to do it yet, so it's
> been languishing as a mentored bug for quite a while. Let me know if you
> need this to work, and I can prioritize that bug higher (i.e., give up and
> fix it myself.)

I'd love to take you up on that. I've been trying to work around the issue, but haven't found a satisfying way yet. In particular, I'm trying to pass an ImageData object (imageData) around, and as far as I can tell the only thing that is accepted by Firefox (and Chrome) is to specify imageData.data.buffer as transferable object, except that this results in the buffer ending up lost, inaccessible to either party. Also, as far as I can tell I cannot "reassemble" an ImageData object from just an ArrayBuffer, so the only viable option is to just let Firefox copy the object...
Comment 15 Steve Fink [:sfink] [:s:] (PTO Sep23-28) 2013-03-01 10:45:26 PST
It's done and awaiting review in bug 789593.

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