Closed
Bug 667388
Opened 13 years ago
Closed 13 years ago
When using postMessage to call a ChromeWorker method getting a "could not clone object" error
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: ddahl, Assigned: bholley)
References
Details
Attachments
(4 files, 8 obsolete files)
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
Seeing this error (with and without the ASSERTION):
###!!! ASSERTION: No scope has this global object!: 'OKIfNotInitialized', file /home/ddahl/code/moz/mozilla-central/js/src/xpconnect/src/xpcwrappednativescope.cpp, line 840
JavaScript error: , line 0: uncaught exception: [Exception... "The object could not be cloned." code: "25" nsresult: "0x80530019 (NS_ERROR_DOM_DATA_CLONE_ERR)" location: "resource://domcrypt/DOMCryptMethods.jsm Line: 686"]
--D
STR:
call a worker's postMessage in a JSM, passing in an object from content - in this case, the object was a JSON string in localStorage.
I have tried using XPCNativeWrapper.unwrap and creating a new object in the JSM before passing the object into the worker. I still get this error.
If the object is not saved to localStorage or saved as a property on the document, I do not get this error
Script that causes this:
function cryptLocalStorage()
{
// localStorage demo
// create data to save in localStorage after encrypting it
// decrypt and display the data
document.removeEventListener("DOMContentLoaded", cryptLocalStorage);
var plainText = "It was a bright cold day in April, and the clocks were striking thirteen. Winston Smith, his chin nuzzled into his breast in an effort to escape the vile wind, slipped quickly through the glass doors of Victory Mansions, though not quickly enough to prevent a swirl of gritty dust from entering along with him.";
mozCipher.sym.encrypt(plainText, function (cryptoObj){
var cryptoText = JSON.stringify(cryptoObj);
localStorage.setItem("openingText", cryptoText);
var openingTxt = localStorage.getItem("openingText");
console.log("JSON data in localStorage: ", openingTxt);
// decrypt it:
var _cryptoObj = JSON.parse(openingTxt);
mozCipher.sym.decrypt(_cryptoObj, function (plainText){
console.log("decrypted: ", plainText);
});
});
}
The code that ens up passing the object into the worker is here:
https://github.com/daviddahl/domcrypt/blob/master/extension/domcrypt/content/DOMCryptMethods.jsm#L678
note: all properties in the object are strings
Comment 1•13 years ago
|
||
bent, what can make objects fail to clone?
(In reply to comment #1)
> bent, what can make objects fail to clone?
An object with a class that the engine doesn't know about and for which we don't have custom clone hooks. Not sure what could be happening here, it looks like everything is a string. That assertion may have something to do with it.
My guess is that we're getting a security wrapper of some sort here. Even when you're asking to unwrap we generally return a wrapper, just a different type of wrapper that is more transparent.
Ben: How does the structured clone code determine if we have a plain JS object, or one which needs special handling (such as typedarray handling or i-don't-know-what-to-do-with-this-object-so-i'll-call-the-callbacks-and-see-if-they-know-how-to-deal handling)
There's just a list of stuff the engine knows about and then we call the clone hooks.
https://mxr.mozilla.org/mozilla-central/source/js/src/jsclone.cpp#542
Yeah, so the security wrappers don't count as "plain objects" to the structured clone code, which means that we'll loose. In particular, blake says that this test will return false:
https://mxr.mozilla.org/mozilla-central/source/js/src/jsclone.cpp#526
we need to figure out a security policy here.
Reporter | ||
Comment 6•13 years ago
|
||
(In reply to comment #5)
> Yeah, so the security wrappers don't count as "plain objects" to the
> structured clone code, which means that we'll loose. In particular, blake
> says that this test will return false:
Is there a good way to try and work around this for the time being? I did try wrapping the object in an array, I will try some other things as well, like just passing in all strings.
Reporter | ||
Comment 7•13 years ago
|
||
Apparently, unwrapping the object and passing 4 string properties in place of the object is working fine.
Not sure what to do with this bug... Retarget for maybe auto-unwrapping things before we clone them?
I think this is a serious showstopper when you want to read a file with the File API inside a Web Worker. It just does not work.
Chrome solves serialization/deserializaton seamlessly when passing File (Blob) object in the message to the thread, including native method *slice, etc..." and security attributes.
https://github.com/tcz/js-secure-uploader/blob/bc0fceee515310d2a2d720f21ae1a1d4f9bc2fb6/lib/js-secure-uploader.js#L155
Line 155.
The code above gives the error "The object could not be cloned." code: "25"
Comment 10•13 years ago
|
||
This will become an even more serious issue when
https://bugzilla.mozilla.org/show_bug.cgi?id=664783
gets implemented, since sync reads can only be done inside Workers (in order not to block GUI) and you will need to pass the File object there somehow.
Comment 11•13 years ago
|
||
See also: http://dev.w3.org/html5/spec/Overview.html
Internal structured cloning algorithm
"If input is a File object
Let output be a newly constructed File object corresponding to the same underlying data.
If input is a Blob object
Let output be a newly constructed Blob object corresponding to the same underlying data"
Bobby, this sounds like it would be up your aisle.
Assignee: general → bobbyholley+bmo
Assignee | ||
Comment 13•13 years ago
|
||
(In reply to Jonas Sicking (:sicking) from comment #12)
> Bobby, this sounds like it would be up your aisle.
Sounds good - I'll look into this after I finish with bug 655878.
Discussed this today with mrbkap and sicking, we agree that structured clone write hook on main thread should always call XPCWrapper::IsSecurityWrapper followed by XPCWrapper::Unwrap. The read hook shouldn't need any special treatment.
Assignee | ||
Comment 15•13 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #13)
> (In reply to Jonas Sicking (:sicking) from comment #12)
> > Bobby, this sounds like it would be up your aisle.
>
> Sounds good - I'll look into this after I finish with bug 655878.
Just a heads up - given the monday holiday and the ever-expanding scope of the aforementioned bug, I'm unlikely to get to this next week.
I'd love to look into this bug and learn about all this stuff, but if it's time urgent feel free to reassign.
Comment 16•13 years ago
|
||
I think we ran into this bug as well, when trying to do a postMessage of an array from a signed script.
Comment 17•13 years ago
|
||
Apparently it's not urgent :(
One thing that occurred to me. When we unwrap here, we want to make sure that that doesn't affect what security charecteristics we get when accessing getters.
We also might not want to unwrap chrome object wrappers.
Assignee | ||
Comment 19•13 years ago
|
||
(In reply to gphilip from comment #9)
> I think this is a serious showstopper when you want to read a file with the
> File API inside a Web Worker. It just does not work.
>
> https://github.com/tcz/js-secure-uploader/blob/
> bc0fceee515310d2a2d720f21ae1a1d4f9bc2fb6/lib/js-secure-uploader.js#L155
I dug into this testcase a little bit. The structured clone error that you were seeing doesn't match the one of the bug reporter (since your File object is in the same origin as the code calling postMesssage). It als doesn't appear on my machine, so I'm guessing it was something else that's been fixed in the intervening months. ;-)
There are 2 fixes needed to make the uploader work correctly:
1 - declare onmessage without the 'var', or use addEventListener.
2 - check for mozSlice in addition to webkitSlice in the slice code.
Other than that it seems to work fine. It's nice and clean code too, so I had a very easy time digging into it. :-)
Sorry about the delay on this one. Let me know if there's anything I can do to help you out!
Assignee | ||
Comment 20•13 years ago
|
||
I should clarify that I was testing on a nightly build.
Comment 21•13 years ago
|
||
Hey Bobby,
That's a lot for trying it, you're the man! I got so excited when you wrote the fix, but unfortunately it did not work for me :(
My Nightly version is the latest, 11.0a1 (2011-11-09), I changed onmessage to add addEventListener( 'message', ... and check for mozSlice.
Do you mind giving me a hand with a patch? We can continue the discussion here, if you think it's unrelated:
https://github.com/tcz/js-secure-uploader/issues/1
Thanks a lot so far!
bholley, where are we on this?
Assignee | ||
Comment 23•13 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #22)
> bholley, where are we on this?
A few months back I spent some time on this. I know what the fix is, but we're dealing with security–sensitive stuff here, so I wanted to make sure to write proper test coverage. So I wrote an exhaustive test that covered all the possibilities, and it didn't work like I expected. As it turns out, we don't force a document parent in the precreate hook for any of the DOM objects we can clone (file, filelist, etc), so each compartment just gets its own XPCWrappedNative for the underlying object. So we can't actually test security wrappers in those cases.
I'm pretty sure this only affects people writing chrome code. So at jst's advice I've stopped working on it for the moment, with the tentative plan to pick it back up once the new DOM bindings are in place (and thus when the precreate madness is eliminated).
Target Milestone: --- → Future
Assignee | ||
Comment 24•13 years ago
|
||
These were the tests I was working on, which current don't work for the reasons described in comment 23. Abdicating the bug for now.
Assignee | ||
Comment 25•13 years ago
|
||
Also, as a disclaimer, my understanding of what should and shouldn't work correctly evolved as I was working on this test, so some of the stuff it's trying to check might be totally wrong. Just posting for reference.
Assignee: bobbyholley+bmo → general
Assignee | ||
Comment 26•13 years ago
|
||
Per in-person discussion, I'm down to write the patch for this is bent writes a test for the behavior we care about. ;-)
Assignee: general → bent.mozilla
Assignee | ||
Comment 27•13 years ago
|
||
It looks like this is going to be a problem for compartment-per-global, so I started digging in.
I realized that, by ignoring the DOM stuff and cloning straight JS objects, I could put together a test that covers the important paths. I've got that working, so now I'm going to finally write this damn patch. ;-)
Assignee | ||
Comment 28•13 years ago
|
||
Attachment #590684 -
Attachment is obsolete: true
Assignee | ||
Comment 29•13 years ago
|
||
The current situation seems incorrect, especially given the behavior of CrossOriginWrapper and XrayProxy. Currently it doesn't matter, but it probably will in the future.
Attachment #603502 -
Flags: review?(mrbkap)
Assignee | ||
Comment 30•13 years ago
|
||
I was getting confused by some of the naming and lack of comments here.
Attachment #603504 -
Flags: review?(mrbkap)
Assignee | ||
Comment 31•13 years ago
|
||
Attachment #603505 -
Flags: review?(mrbkap)
Assignee | ||
Comment 32•13 years ago
|
||
We also remove the declared-but-never-implemented JSObject::getWrapperHandler.
Attachment #603506 -
Flags: review?(mrbkap)
Assignee | ||
Comment 33•13 years ago
|
||
Attachment #603507 -
Flags: review?(mrbkap)
Updated•13 years ago
|
Attachment #603504 -
Attachment description: part 1 - Clarify the security characteristics of Location objects. v1 → part 2 - Clarify the security characteristics of Location objects. v1
Assignee | ||
Comment 34•13 years ago
|
||
So I've changed my patch ordering slightly - I moved my Location object munging into bug 733984, and made this bug's patches depend on that one. So Blake, can you review bug 733984 first? I'm updating all the patches everywhere just to make sure they match what's in my branch.
Assignee | ||
Updated•13 years ago
|
Attachment #602101 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Attachment #603502 -
Attachment is obsolete: true
Attachment #603502 -
Flags: review?(mrbkap)
Assignee | ||
Updated•13 years ago
|
Attachment #603504 -
Attachment is obsolete: true
Attachment #603504 -
Flags: review?(mrbkap)
Assignee | ||
Updated•13 years ago
|
Attachment #603505 -
Attachment is obsolete: true
Attachment #603505 -
Flags: review?(mrbkap)
Assignee | ||
Updated•13 years ago
|
Attachment #603506 -
Attachment is obsolete: true
Attachment #603506 -
Flags: review?(mrbkap)
Assignee | ||
Updated•13 years ago
|
Attachment #603507 -
Attachment is obsolete: true
Attachment #603507 -
Flags: review?(mrbkap)
Assignee | ||
Comment 35•13 years ago
|
||
Attachment #605230 -
Flags: review?(mrbkap)
Assignee | ||
Comment 36•13 years ago
|
||
Attachment #605231 -
Flags: review?(mrbkap)
Assignee | ||
Comment 37•13 years ago
|
||
Attachment #605232 -
Flags: review?(mrbkap)
Assignee | ||
Comment 38•13 years ago
|
||
Attachment #605233 -
Flags: review?(mrbkap)
Assignee | ||
Comment 39•13 years ago
|
||
Updated part 3 to fix a cross-compartment GC hazard I discovered in further testing. I also moved the puncture unwrapping into its own function. Reflagging mrbkap.
Attachment #605232 -
Attachment is obsolete: true
Attachment #607865 -
Flags: review?(mrbkap)
Attachment #605232 -
Flags: review?(mrbkap)
Updated•13 years ago
|
Attachment #605230 -
Flags: review?(mrbkap) → review+
Updated•13 years ago
|
Attachment #605231 -
Flags: review?(mrbkap) → review+
Updated•13 years ago
|
Attachment #605233 -
Flags: review?(mrbkap) → review+
Comment 40•13 years ago
|
||
Comment on attachment 607865 [details] [diff] [review]
part 3 - Handle wrappers during structured clone. v3
Review of attachment 607865 [details] [diff] [review]:
-----------------------------------------------------------------
Beautiful.
Attachment #607865 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 41•13 years ago
|
||
I've pushed this to try along with the some of the patches in bug 733984 (the simpler ones, that this bug actually depends on):
https://tbpl.mozilla.org/?tree=Try&rev=c4d237b6fc8a
Assignee | ||
Comment 42•13 years ago
|
||
Ok, that was all green except for an assertion failure which was explicit fixed by one of the patches I forgot to include. Pushed to inbound with that included:
http://hg.mozilla.org/integration/mozilla-inbound/rev/66223f04fb55
http://hg.mozilla.org/integration/mozilla-inbound/rev/1fb77c1bb742
http://hg.mozilla.org/integration/mozilla-inbound/rev/419581bb1b90
http://hg.mozilla.org/integration/mozilla-inbound/rev/1742f60b4468
Target Milestone: --- → mozilla14
Comment 43•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1742f60b4468
https://hg.mozilla.org/mozilla-central/rev/419581bb1b90
https://hg.mozilla.org/mozilla-central/rev/1fb77c1bb742
https://hg.mozilla.org/mozilla-central/rev/66223f04fb55
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 44•13 years ago
|
||
> + window.blob = (new MozBlobBuilder()).getBlob('text/plain');
MozBlobBuilder is deprecated and will be removed in future.
Please replace it with Blob constructor:
window.blob = new Blob([], {type: 'text/plain'});
You need to log in
before you can comment on or make changes to this bug.
Description
•