The default bug view has changed. See this FAQ.

When using postMessage to call a ChromeWorker method getting a "could not clone object" error

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: ddahl, Assigned: bholley)

Tracking

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 8 obsolete attachments)

1.21 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
5.84 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
8.10 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
7.06 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 6

6 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

6 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?

Comment 9

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

6 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

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

Comment 16

6 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

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

Comment 21

6 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?
(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
Created attachment 590684 [details] [diff] [review]
Tests WIP

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: 650353
Target Milestone: Future → ---
Created attachment 602101 [details] [diff] [review]
Tests. v1
Attachment #590684 - Attachment is obsolete: true
Created attachment 603502 [details] [diff] [review]
part 1 - Make the chrome-to-content Xray wrapper derive CrossCompartmentWrapper. v1

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)
Created attachment 603504 [details] [diff] [review]
part 2 - Clarify the security characteristics of Location objects. v1

I was getting confused by some of the naming and lack of comments here.
Attachment #603504 - Flags: review?(mrbkap)
Created attachment 603505 [details] [diff] [review]
part 3 - Introduce the PUNCTURE wrapper action. v2
Attachment #603505 - Flags: review?(mrbkap)
Created attachment 603506 [details] [diff] [review]
part 4 - Handle wrappers during structured clone. v2

We also remove the declared-but-never-implemented JSObject::getWrapperHandler.
Attachment #603506 - Flags: review?(mrbkap)
Created attachment 603507 [details] [diff] [review]
Tests. v3
Attachment #603507 - Flags: review?(mrbkap)

Updated

5 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
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)
Created attachment 605230 [details] [diff] [review]
part 1 - Make the chrome-to-content Xray wrapper derive CrossCompartmentWrapper. v1
Attachment #605230 - Flags: review?(mrbkap)
Created attachment 605231 [details] [diff] [review]
part 2 - Introduce the PUNCTURE wrapper action. v2
Attachment #605231 - Flags: review?(mrbkap)
Created attachment 605232 [details] [diff] [review]
part 3 - Handle wrappers during structured clone. v2
Attachment #605232 - Flags: review?(mrbkap)
Created attachment 605233 [details] [diff] [review]
part 4 - Tests. v3
Attachment #605233 - Flags: review?(mrbkap)
Created attachment 607865 [details] [diff] [review]
part 3 - Handle wrappers during structured clone. v3

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

5 years ago
Attachment #605230 - Flags: review?(mrbkap) → review+

Updated

5 years ago
Attachment #605231 - Flags: review?(mrbkap) → review+

Updated

5 years ago
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
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
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
> +    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'});

Updated

5 years ago
Depends on: 738956
Depends on: 739825

Updated

5 years ago
Depends on: 751858
You need to log in before you can comment on or make changes to this bug.