Implement transfer parameter of window.postMessage

RESOLVED FIXED in mozilla20

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

({dev-doc-complete})

unspecified
mozilla20
dev-doc-complete
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
I'm a bit surprised that Firefox doesn't support it yet.
(Assignee)

Comment 1

5 years ago
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.
Attachment #692733 - Flags: review?(sphink)
(Assignee)

Comment 2

5 years ago
Created attachment 692734 [details] [diff] [review]
Part 2: Add transfer parameter to window.postMessage
Attachment #692734 - Flags: review?(jonas)
(Assignee)

Comment 3

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=65c7343e374c
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.)
Attachment #692733 - Flags: review?(sphink)
Attachment #692734 - Flags: review?(jonas) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 693089 [details] [diff] [review]
Part 1: Unwrap the transfable parameter if allowed, v2

Resolved review comments.
Attachment #692733 - Attachment is obsolete: true
Attachment #693089 - Flags: review?(sphink)
(Assignee)

Comment 6

5 years ago
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.
Attachment #692734 - Attachment is obsolete: true
Attachment #693090 - Flags: review+
Attachment #693089 - Flags: review?(sphink) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
Keywords: dev-doc-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/56f2d3fbcf49
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d5cc2e8c111
Assignee: nobody → VYV03354
Flags: in-testsuite+
Keywords: checkin-needed

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/56f2d3fbcf49
https://hg.mozilla.org/mozilla-central/rev/7d5cc2e8c111
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20

Comment 9

5 years ago
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.
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

5 years ago
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!
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

5 years ago
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

4 years ago
(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...
It's done and awaiting review in bug 789593.
I've updated:
https://developer.mozilla.org/en-US/docs/Web/API/window.postMessage
and
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/20
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.