Right now we have two cloning implementations in XPConnect. One of them is for Cu.cloneInto, which optionally handles functions. The other is for Cu.exportFunction, which handles reflectors by wrapping them. We should unify these implementations, which will allow us to make the reflector-wrapping behavior available to Cu.cloneInto, solving the problem in bug 1026786.

This ended up being more involved than I was hoping, but it was worthwhile cleanup. https://tbpl.mozilla.org/?tree=Try&rev=34817737c871

A lot of this stuff is usable from both Sandbox.cpp and XPCComponents.cpp, and those files are both pretty big these days.

If a clone happens between two same-origin scopes, we'll end up with CCWs here that are neither Xrays nor reflectors.

Comment on attachment 8442286 [details] [diff] [review] Part 2 - Remove invalid assertion. v1 Review of attachment 8442286 [details] [diff] [review]: ----------------------------------------------------------------- > If a clone happens between two same-origin scopes, we'll end up with CCWs here > that are neither Xrays nor reflectors. Modulo wantXray flag on the sandbox... but yeah if not jetpack content-script is the only consumer this should go.

(In reply to Bobby Holley (:bholley) from comment #4) > Created attachment 8442287 [details] [diff] [review] > Part 3 - Rename CloneNonReflectors into StackScopedClone, and give it an > options struct. v1 StackScopedClone? That does not mean a thing for me... could you elaborate on this name change? It sounds very-very confusing to me.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11) > (In reply to Bobby Holley (:bholley) from comment #4) > > Created attachment 8442287 [details] [diff] [review] > > Part 3 - Rename CloneNonReflectors into StackScopedClone, and give it an > > options struct. v1 > > StackScopedClone? That does not mean a thing for me... It means that the clone may only be used in stack scope - this is distinct from the way Structured Clone often happens in the DOM, when it's used asynchronously for things like postMessage, and the lifetime is more complicated. I could add comments to this effect if you want. > could you elaborate > on this name change? It sounds very-very confusing to me. Given all the things we're using it for, CloneNonReflectors isn't an appropriate name anymore. The alternative would be something like XPCClone, but I think we should move away from identifying utilities by their module and instead focus on identifying them by their function. The DOM may well have uses for StackScopedClone as well.

(In reply to Bobby Holley (:bholley) from comment #12) > complicated. I could add comments to this effect if you want. Yeah with a comment it sounds fine. > and instead focus on identifying them by their function. The DOM may well > have uses for StackScopedClone as well. It is much better than XPCClone for sure. Stack scope isn't really its functionality, but I'm OK with the name, it's a sane choice. Just let's try to explain what it does in a comment, since it's far from trivial from the name (or from the code. And I don't think any name would be descriptive enough...

Comment on attachment 8442287 [details] [diff] [review] Part 3 - Rename CloneNonReflectors into StackScopedClone, and give it an options struct. v1 Review of attachment 8442287 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/ExportHelpers.cpp @@ +55,3 @@ > } > > + MOZ_ASSERT_UNREACHABLE("Encountered garbage in the clone stream!"); Didn't the compiler say something about no return value here? Or does this macro solve that problem? @@ -90,5 @@ > nullptr > }; > > /* > - * This is a special structured cloning, that clones only non-reflectors. Removing this part of the comment is not OK imo. Please explain what this function does I know it's more difficult with the different options after these patches, but that just makes it more important to have this comment.

Comment on attachment 8442288 [details] [diff] [review] Part 4 - Switch to a struct for the StackScopedClone closure. v1 Review of attachment 8442288 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/ExportHelpers.cpp @@ +130,2 @@ > > return true; return buffer.read(...) could work here too

Comment on attachment 8442291 [details] [diff] [review] Part 6 - Innerize before reflector detection. v1 Review of attachment 8442291 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/ExportHelpers.cpp @@ +102,5 @@ > MOZ_ASSERT(closure, "Null pointer!"); > StackScopedCloneData *cloneData = static_cast<StackScopedCloneData *>(closure); > + > + // The SpiderMonkey structured clone machinery does a CheckedUnwrap, but > + // but doesn't strip off outer windows. Do that to avoid confusing the double but

Comment on attachment 8443012 [details] [diff] [review] Part 8 - Use StackScopedClone for cloneInto. v2 Review of attachment 8443012 [details] [diff] [review]: ----------------------------------------------------------------- Actually this checkThrows would be nice to add to the standard testing functions in some form, what do you think? I find it very handy, and tests often need it. This is completely unrelated to this bug/patch ofc.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #16) > > + MOZ_ASSERT_UNREACHABLE("Encountered garbage in the clone stream!"); > > Didn't the compiler say something about no return value here? Or does this > macro solve that problem? Hm - it didn't for me, but I don't see anything in the implementation of that macro that would give the compiler the necessary hint. I'll just add a |return nullptr;| to be safe. (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19) > Actually this checkThrows would be nice to add to the standard testing > functions in some form, what do you think? I find it very handy, and tests > often need it. This is completely unrelated to this bug/patch ofc. Agreed. I just went to file a bug though, and realized that I had no idea which component to file it in (Mochitest? XPCShell? Assert.jsm?), because we're still waiting for the test framework holy wars to sort themselves out. :-( remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b576ed8a17c2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c8faa8089e9 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f3fc9863ef31 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ffed77f6ec4e remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d1e64043288 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5a0b0eb5bc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ee620c5b0743 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/63c6584a60d3 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a5e11ed9245

https://hg.mozilla.org/mozilla-central/rev/b576ed8a17c2 https://hg.mozilla.org/mozilla-central/rev/4c8faa8089e9 https://hg.mozilla.org/mozilla-central/rev/f3fc9863ef31 https://hg.mozilla.org/mozilla-central/rev/ffed77f6ec4e https://hg.mozilla.org/mozilla-central/rev/3d1e64043288 https://hg.mozilla.org/mozilla-central/rev/0c5a0b0eb5bc https://hg.mozilla.org/mozilla-central/rev/ee620c5b0743 https://hg.mozilla.org/mozilla-central/rev/63c6584a60d3 https://hg.mozilla.org/mozilla-central/rev/9a5e11ed9245

(In reply to Bobby Holley (:bholley) from comment #20) > (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #19) > > Actually this checkThrows would be nice to add to the standard testing > > functions in some form, what do you think? I find it very handy, and tests > > often need it. This is completely unrelated to this bug/patch ofc. > > Agreed. I just went to file a bug though, and realized that I had no idea > which component to file it in (Mochitest? XPCShell? Assert.jsm?), because > we're still waiting for the test framework holy wars to sort themselves out. > :-( SimpleTest.doesThrow does half of what you want. It doesn't check the exception details with a regex. I'm sure Ted or :robcee could easily be convinced to add that. Bugs can be filed in Testing::Mochitest. ;-)

Cool! Next time I find myself writing checkThrows, I'll use that and write a patch to SimpleTest.js to add an optional regexp argument.