Last Comment Bug 667388 - When using postMessage to call a ChromeWorker method getting a "could not clone object" error
: When using postMessage to call a ChromeWorker method getting a "could not clo...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: mozilla14
Assigned To: Bobby Holley (busy)
:
Mentors:
Depends on: 738956 739825 751858
Blocks: cpg 734215
  Show dependency treegraph
 
Reported: 2011-06-26 22:39 PDT by David Dahl :ddahl
Modified: 2012-05-04 05:22 PDT (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Tests WIP (6.12 KB, patch)
2012-01-23 05:52 PST, Bobby Holley (busy)
no flags Details | Diff | Review
Tests. v1 (6.26 KB, patch)
2012-03-01 14:01 PST, Bobby Holley (busy)
no flags Details | Diff | Review
part 1 - Make the chrome-to-content Xray wrapper derive CrossCompartmentWrapper. v1 (1.30 KB, patch)
2012-03-06 15:57 PST, Bobby Holley (busy)
no flags Details | Diff | Review
part 2 - Clarify the security characteristics of Location objects. v1 (11.54 KB, patch)
2012-03-06 15:58 PST, Bobby Holley (busy)
no flags Details | Diff | Review
part 3 - Introduce the PUNCTURE wrapper action. v2 (5.84 KB, patch)
2012-03-06 15:59 PST, Bobby Holley (busy)
no flags Details | Diff | Review
part 4 - Handle wrappers during structured clone. v2 (2.86 KB, patch)
2012-03-06 15:59 PST, Bobby Holley (busy)
no flags Details | Diff | Review
Tests. v3 (8.10 KB, patch)
2012-03-06 15:59 PST, Bobby Holley (busy)
no flags Details | Diff | Review
part 1 - Make the chrome-to-content Xray wrapper derive CrossCompartmentWrapper. v1 (1.21 KB, patch)
2012-03-12 17:06 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
part 2 - Introduce the PUNCTURE wrapper action. v2 (5.84 KB, patch)
2012-03-12 17:06 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
part 3 - Handle wrappers during structured clone. v2 (2.86 KB, patch)
2012-03-12 17:06 PDT, Bobby Holley (busy)
no flags Details | Diff | Review
part 4 - Tests. v3 (8.10 KB, patch)
2012-03-12 17:07 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review
part 3 - Handle wrappers during structured clone. v3 (7.06 KB, patch)
2012-03-20 23:04 PDT, Bobby Holley (busy)
mrbkap: review+
Details | Diff | Review

Description David Dahl :ddahl 2011-06-26 22:39:15 PDT
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 David Mandelin [:dmandelin] 2011-06-27 14:33:26 PDT
bent, what can make objects fail to clone?
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-27 14:40:48 PDT
(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.
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-27 15:12:43 PDT
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)
Comment 4 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-06-27 15:26:28 PDT
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
Comment 5 Jonas Sicking (:sicking) PTO Until July 5th 2011-06-27 17:25:39 PDT
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.
Comment 6 David Dahl :ddahl 2011-06-27 17:54:59 PDT
(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.
Comment 7 David Dahl :ddahl 2011-06-27 19:21:36 PDT
Apparently, unwrapping the object and passing 4 string properties in place of the object is working fine.
Comment 8 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-07-31 12:14:38 PDT
Not sure what to do with this bug... Retarget for maybe auto-unwrapping things before we clone them?
Comment 9 gphilip 2011-08-01 17:03:22 PDT
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 gphilip 2011-08-01 17:05:52 PDT
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 gphilip 2011-08-02 14:20:59 PDT
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"
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2011-08-30 09:55:35 PDT
Bobby, this sounds like it would be up your aisle.
Comment 13 Bobby Holley (busy) 2011-08-30 11:04:55 PDT
(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.
Comment 14 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-08-30 15:16:48 PDT
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.
Comment 15 Bobby Holley (busy) 2011-09-02 13:02:35 PDT
(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 Sjoerd Visscher 2011-09-08 07:35:04 PDT
I think we ran into this bug as well, when trying to do a postMessage of an array from a signed script.
Comment 17 spam 2011-09-12 06:26:48 PDT
Apparently it's not urgent :(
Comment 18 Jonas Sicking (:sicking) PTO Until July 5th 2011-09-12 07:17:40 PDT
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.
Comment 19 Bobby Holley (busy) 2011-11-09 11:40:47 PST
(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!
Comment 20 Bobby Holley (busy) 2011-11-09 12:08:08 PST
I should clarify that I was testing on a nightly build.
Comment 21 gphilip 2011-11-09 14:33:25 PST
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!
Comment 22 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-11 00:54:12 PST
bholley, where are we on this?
Comment 23 Bobby Holley (busy) 2012-01-11 11:14:34 PST
(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).
Comment 24 Bobby Holley (busy) 2012-01-23 05:52:33 PST
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.
Comment 25 Bobby Holley (busy) 2012-01-23 05:54:06 PST
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.
Comment 26 Bobby Holley (busy) 2012-01-23 07:13:25 PST
Per in-person discussion, I'm down to write the patch for this is bent writes a test for the behavior we care about. ;-)
Comment 27 Bobby Holley (busy) 2012-03-01 13:51:10 PST
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. ;-)
Comment 28 Bobby Holley (busy) 2012-03-01 14:01:32 PST
Created attachment 602101 [details] [diff] [review]
Tests. v1
Comment 29 Bobby Holley (busy) 2012-03-06 15:57:29 PST
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.
Comment 30 Bobby Holley (busy) 2012-03-06 15:58:42 PST
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.
Comment 31 Bobby Holley (busy) 2012-03-06 15:59:01 PST
Created attachment 603505 [details] [diff] [review]
part 3 - Introduce the PUNCTURE wrapper action. v2
Comment 32 Bobby Holley (busy) 2012-03-06 15:59:40 PST
Created attachment 603506 [details] [diff] [review]
part 4 - Handle wrappers during structured clone. v2

We also remove the declared-but-never-implemented JSObject::getWrapperHandler.
Comment 33 Bobby Holley (busy) 2012-03-06 15:59:56 PST
Created attachment 603507 [details] [diff] [review]
Tests. v3
Comment 34 Bobby Holley (busy) 2012-03-12 16:57:11 PDT
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.
Comment 35 Bobby Holley (busy) 2012-03-12 17:06:23 PDT
Created attachment 605230 [details] [diff] [review]
part 1 - Make the chrome-to-content Xray wrapper derive CrossCompartmentWrapper. v1
Comment 36 Bobby Holley (busy) 2012-03-12 17:06:39 PDT
Created attachment 605231 [details] [diff] [review]
part 2 - Introduce the PUNCTURE wrapper action. v2
Comment 37 Bobby Holley (busy) 2012-03-12 17:06:54 PDT
Created attachment 605232 [details] [diff] [review]
part 3 - Handle wrappers during structured clone. v2
Comment 38 Bobby Holley (busy) 2012-03-12 17:07:10 PDT
Created attachment 605233 [details] [diff] [review]
part 4 - Tests. v3
Comment 39 Bobby Holley (busy) 2012-03-20 23:04:34 PDT
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.
Comment 40 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-03-22 09:42:42 PDT
Comment on attachment 607865 [details] [diff] [review]
part 3 - Handle wrappers during structured clone. v3

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

Beautiful.
Comment 41 Bobby Holley (busy) 2012-03-23 11:44:32 PDT
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
Comment 42 Bobby Holley (busy) 2012-03-23 15:02:36 PDT
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
Comment 44 Masatoshi Kimura [:emk] 2012-03-25 09:02:47 PDT
> +    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'});

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