If reparenting crosses compartments, we need to transplant the object, which messes up preserved wrappers. Currently we handle preserved wrappers in CloneAndAdopt, but this doesn't account for the other ways we can end up in ReparentWrapperIfFound. My best guess as to why we handle it outside of ReparentWrapperIfFound is that the myriad of early-return statements make it tricky to do manually. But that's why we have RAII. Patch coming right up.

Attaching patch. Flagging mrbkap for review.

Comment on attachment 601807 [details] [diff] [review] XPCWrappedNative::ReparentWrapperIfFound should handle preserved wrappers. v1 I think the original reason I pushed this into nsNodeUtils.cpp was that it still feels somewhat icky to touch the DOM directly in XPConnect. However, this is clearly the way to go.

Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=50cfbd69784d

This looks green so far. Pushing to m-i: http://hg.mozilla.org/integration/mozilla-inbound/rev/2fbc7d9ac670

Was this expected? Regression :( Dromaeo (DOM) decrease 1.85% on MacOSX 10.6 (rev4) Mozilla-Inbound -------------------------------------------------------------------------------- Previous: avg 289.913 stddev 2.621 of 30 runs up to revision 54a7b61b9535 New : avg 284.552 stddev 0.234 of 5 runs since revision 2fbc7d9ac670 Change : -5.361 (1.85% / z=2.045) Graph : http://mzl.la/AeLeWD Changeset range: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=54a7b61b9535&tochange=2fbc7d9ac670 https://groups.google.com/forum/#!topic/mozilla.dev.tree-management/9R3MgDfZyJo

I'm spinning up a profiling build to investigate this now. I'll check if any of the dromaeo tests are disproportionately affected, and if anything jumps out at me from the profiler. If I don't find anything though, I think we should probably ignore this: * It's not clear from the graph that this is actually the culprit changeset. I'm somewhat skeptical that it is, given that this only changes codepaths that reparent wrappers, which are already, in general, very slow. * It's a small drop on one-platform, making it very difficult to say anything definitive about * It blocks compartment-per-global (a Q1 DOM goal). FWIW, khuey agrees with the above analysis.

Sounds good to me; thank you for checking just in case anyway :-)

Also, test_native_mouse_mac.xul has been timing out about 41% of the time on OS X 10.5 and 10.6 debug builds since this patch landed (bug 675484). Previously it timed out only about 3% of the time in OS X 10.5 and 10.6 debug builds. I retriggered to get a total of 15 test runs on the two pushes immediately preceding this patch. Those runs had 0 timeouts out of 15 runs. (For comparison, the chance getting 15 green runs in a row *after* this patch landed is somewhere around 0.04%.) I think we should back this patch out on inbound and see if it resolves the failures.

(In reply to Matt Brubeck (:mbrubeck) from comment #8) > I think we should back this patch out on inbound and see if it resolves the > failures. Sounds reasonable. Let's do it.

Backed out on inbound. Sorry! https://hg.mozilla.org/integration/mozilla-inbound/rev/2fbc7d9ac670

The backout did not reduce the test failures, so I re-landed the original patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/4091b2b24293

Also, with regards to dromaeo: I ran it under a profiler, and there aren't even samples for ReparentWrapperIfFound, so it's likely that this code is never even hit from dromaeo. So I'm guessing this wasn't the culprit.