601803 - Support adopting a node cross-compartment

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Peter Van der Beken [:peterv]]

We've added code to bail out (see nsNodeUtils::CloneAndAdopt) but we should instead brain-transplant the existing XPCWrappedNative's JSObject into the new compartment.

Comment 1

[Jason Orendorff [:jorendorff]]

There are some interesting problems to work through here.

It's helpful to think of even XPCOM objects as living in a particular compartment. Like JS objects, DOM nodes collectively belong to a domain and have the same principals. I think eventually we want them to be allocated in compartments in fact. If we really do want to go in that direction, then I think we have to implement this using a lot of C++ Swap() methods. Adopting would be O(total size of the tree being adopted).

JS_TransplantWrapper does not work on ordinary JSObjects yet, only non-script proxies. A brain transplant would be a bit more complex for ordinary JSObjects. When you move an object from one compartment to another, any pointers it holds to other values in its compartment of origin must be rewrapped for the new compartment. That's easy enough to do for pointers directly held by the JSObject itself-- simply loop over all the fslots and dslots (proto and parent, too) and use compartment->wrap on each one. For JSObjects held by the underlying C++ object (event handlers? nsIDOM3Node::setUserData? WrappedJS-es?) I'm not sure how to do it.

Comment 3

[Wladimir Palant]

This should block bug 584237, otherwise it is too difficult to find.

Comment 4

[Johnny Stenback (:jst)]

Over to mrbkap who's likely going to work on XPCWrappedNative brain transplanting, and blocking.

Comment 6

[Guillermo López :willyaranda (probably SLOW response)]

I would like to see this bug blocking beta7, not beta8 per bug 604612

Comment 7

[Rubén Martín [:Nukeador] ❌ [away till Aug 31st]]

In my opinion this bug should block beta7, but beta7 was released yesterday and we still have the problem and a big marketing problem in Spain, Tuenti.com does not load (this site is more important than Facebook in Spain).

Just imagine a Firefox 4 beta which is unable to load Facebook...

Comment 8

[Brendan Eich [:brendan]]

Rubén, knowing this before b7 cutoff might have helped, but it's water under the bridge. The best we can do is get a fix in a nightly and promote a stable nightly with the fix instead of b7.

Blake, is this fixable soon?

/be

Comment 9

[Andreas Gal :gal]

This is the most complex bug to fix for b8, so we are dealing with it first. It will probably take 2 or 3 days to fix.

Comment 10

[Rubén Martín [:Nukeador] ❌ [away till Aug 31st]]

Sorry guys, I though you were aware about this site importance due bug #604612 comments.

Thanks for your hard work!

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Draft of XPConnect changes

This is missing the JS bits (it doesn't even compile), but it's a first stab at what I think need to be done in XPConnect.

Comment 12

[Andreas Gal :gal]

Attachment: patch

More potent swap that should work across compartments and can also swap objects with different builtin slot count as well as native and non-native objects.

Comment 13

[Aakash Desai [:aakashd]]

Sorry for bug spam, but this tuenti.com is the second highest submitted site with issues on beta 7 via Input - http://input.mozilla.com/en-US/site/http/tuenti.com?sentiment=sad&search_type=latest+beta . I think they're all related to this issue.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Any way we can test that patch to ensure it's having the desired effect?

Comment 15

[Mark Smith [:mcs]]

Attachment: stack trace (failed assertion)

For what its worth, I applied attachment 490287 [details] [diff] [review] to my working mozilla-central tree on Mac OS and an assertion failed during startup/initial page load (maybe this stack trace will help someone).

I am not trying to work on this bug, but I was trying to determine if it explains a problem I am experiencing with an extension.  My extension stores a JS object as a JS property on a content node and then tries to retrieve the object later... but this does not work reliably with recent builds (it did work OK in Firefox 4.0b6 and in 3.x).

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Mark, I think your problem is another bug, please file it.

The patches in this bug are buggy for sure. We're still debugging them.

Comment 17

[Mark Smith [:mcs]]

(In reply to comment #16)
> Mark, I think your problem is another bug, please file it.

Done.  See bug 613515.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Mostly untested patch

This doesn't crash immediately upon brain transplants or even after the first GC, but I haven't made sure that the result actually works or that expando properties are preserved (I doubt they do, Andreas, I think we need to expose a version JSObject::swap that only swaps properties (i.e. not the proto, parent and class) as well).

Comment 19

[Johnny Stenback (:jst)]

Per discussion with mrbkap and gal we don't need to finalize this for beta8, pushing to beta9.

Comment 20

[Rubén Martín [:Nukeador] ❌ [away till Aug 31st]]

(In reply to comment #19)
> Per discussion with mrbkap and gal we don't need to finalize this for beta8,
> pushing to beta9.

Please, could you reconsider this? This bug is breaking the most important social network in Spain (more that 8 million users) and is the 3rd site reported as "not working" at input.mozilla.org (comment #13)

We are having a big marketing problem in Spain with Firefox 4 betas and this bug.

Comment 21

[Mike Shaver (:shaver emeritus)]

I'd thought this was one of the critical issues for beta 8, too. Can someone elaborate on why it's being bumped?

Comment 22

[Damon Sicore (:damons)]

Yeah, would like to know why here, and how much work is remaining here?  Days?  Weeks?  Original estimate was a couple/few days.  

Moving back to beta 8 until we have an answer here.

Comment 23

[Andreas Gal :gal]

We have a patch but it needs work. I agree doing this for b8 would be good. Need Blake for this but I can help.

Comment 24

[Johnny Stenback (:jst)]

I fully agree that this would be good to fix for beta8, but given that we're not confident in the fix yet and the planned freeze *was* today I think we'd be better off releasing what we have than delaying only for this. But seems we're delaying for other reasons now, so we'll obviously try to get this in if at all possible. If it doesn't get fixed for beta8, it'll be fixed in the beta after that.

Comment 25

[Damon Sicore (:damons)]

Need progress here soon.  If we think this is going to take a week, we should ship beta 8 without it, and get it in as soon as possible.  To be clear:  Yes, we're going to fix it; however, we're tight on resources in this area so please be patient (typing to those users of the Spanish social networking site).

Comment 26

[jvbferrer]

(In reply to comment #25)
> Need progress here soon.  If we think this is going to take a week, we should
> ship beta 8 without it, and get it in as soon as possible.  To be clear:  Yes,
> we're going to fix it; however, we're tight on resources in this area so please
> be patient (typing to those users of the Spanish social networking site).

I agree with you, but you also have to think that we couldn't enter the site since two months ago.

Comment 27

[LegNeato]

Yes, it'll be a tough call. We are also planning on doing more non-tech outreach for beta8, so it would be unfortunate not to get this in.

On the other hand, we need feedback on the other fixes, this issue has existed for multiple betas, and we plan to have beta9 come fairly quickly after.

Comment 28

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

I just pushed this to try, though on first glance it appears to work.

Comment 29

[Johnny Stenback (:jst)]

Attachment: Additional change to make the above compile

Comment 30

[Andreas Gal :gal]

Comment on attachment 494598 [details] [diff] [review]
patch v1

>      * This function is called when a window is navigating. In that case, we
>-     * need to "move" the window from wrapper's compartment to target's
>+     * need to "move" the window from origobj's compartment to target's
>      * compartment.

This function is called when an object moves from one origin to another, i.e. a window is navigating or a node is being adopted.

Comment 31

[Johnny Stenback (:jst)]

For the record, it's looking very likely that this will land today, and thus be fixed in beta8.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/08c80bfca302

Comment 34

[Johnny Stenback (:jst)]

This landed on mozilla-central last night, marking FIXED!

http://hg.mozilla.org/mozilla-central/rev/a4813c8be814

Comment 35

[jvbferrer]

Yeah! Now it works perfectly. Thanks.

Comment 36

[chris hofmann]

we need to figure out if this could be connected to the crash regression on dec 4 in bug 617048

Comment 37

[Henri Sivonen (:hsivonen)]

The mochitest fails with my current iteration of parserless about:blank (attachment 541670 [details] [diff] [review]) applied. How do I need to prepare the generated about:blank to make sure the expando in the mochitest added here gets preserved?

Comment 38

[Henri Sivonen (:hsivonen)]

The solution from bug 428653 comment 25 applies here, too.

Comment 39

[Peter Van der Beken [:peterv]]

Comment on attachment 494598 [details] [diff] [review]
patch v1

gal and jst r+'ed, don't think I can add anything (well, clownshoes+).