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)

defect
Not set
normal

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
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.
(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.
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"
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.
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
(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.
(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.
I think we ran into this bug as well, when trying to do a postMessage of an array from a signed script.
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.
(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!
I should clarify that I was testing on a nightly build.
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!
(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
Attached patch Tests WIP (obsolete) (deleted) — Splinter Review
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.
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
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
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: bent.mozilla → bobbyholley+bmo
Blocks: cpg
Target Milestone: Future → ---
Attached patch Tests. v1 (obsolete) (deleted) — Splinter Review
Attachment #590684 - Attachment is obsolete: true
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)
I was getting confused by some of the naming and lack of comments here.
Attachment #603504 - Flags: review?(mrbkap)
Attachment #603505 - Flags: review?(mrbkap)
We also remove the declared-but-never-implemented JSObject::getWrapperHandler.
Attachment #603506 - Flags: review?(mrbkap)
Attached patch Tests. v3 (obsolete) (deleted) — Splinter Review
Attachment #603507 - Flags: review?(mrbkap)
Attachment #603504 - Attachment description: part 1 - Clarify the security characteristics of Location objects. v1 → part 2 - Clarify the security characteristics of Location objects. v1
Blocks: 734215
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.
Attachment #602101 - Attachment is obsolete: true
Attachment #603502 - Attachment is obsolete: true
Attachment #603502 - Flags: review?(mrbkap)
Attachment #603504 - Attachment is obsolete: true
Attachment #603504 - Flags: review?(mrbkap)
Attachment #603505 - Attachment is obsolete: true
Attachment #603505 - Flags: review?(mrbkap)
Attachment #603506 - Attachment is obsolete: true
Attachment #603506 - Flags: review?(mrbkap)
Attachment #603507 - Attachment is obsolete: true
Attachment #603507 - Flags: review?(mrbkap)
Attachment #605231 - Flags: review?(mrbkap)
Attachment #605232 - Flags: review?(mrbkap)
Attached patch part 4 - Tests. v3 (deleted) — Splinter Review
Attachment #605233 - Flags: review?(mrbkap)
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)
Attachment #605230 - Flags: review?(mrbkap) → review+
Attachment #605231 - Flags: review?(mrbkap) → review+
Attachment #605233 - Flags: review?(mrbkap) → review+
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+
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
Target Milestone: --- → mozilla14
> + 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'});
Depends on: 738956
Depends on: 739825
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: