Closed Bug 601803 Opened 9 years ago Closed 9 years ago

Support adopting a node cross-compartment

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta8+

People

(Reporter: peterv, Assigned: mrbkap)

References

Details

(Keywords: regression, Whiteboard: [compartments], [Input] fixed-in-tracemonkey)

Attachments

(2 files, 4 obsolete files)

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.
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.
Duplicate of this bug: 604736
This should block bug 584237, otherwise it is too difficult to find.
Blocks: compartments
Over to mrbkap who's likely going to work on XPCWrappedNative brain transplanting, and blocking.
Assignee: nobody → mrbkap
blocking2.0: --- → beta8+
Keywords: regression
Duplicate of this bug: 604612
I would like to see this bug blocking beta7, not beta8 per bug 604612
Whiteboard: [compartments]
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...
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
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.
Sorry guys, I though you were aware about this site importance due bug #604612 comments.

Thanks for your hard work!
Attached patch Draft of XPConnect changes (obsolete) — Splinter Review
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.
Attached patch patch (obsolete) — Splinter Review
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.
Blocks: 611982
No longer blocks: 611982
Depends on: 611982
Whiteboard: [compartments] → [compartments], [Input]
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.
Any way we can test that patch to ensure it's having the desired effect?
Attached file stack trace (failed assertion) (obsolete) —
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).
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.
(In reply to comment #16)
> Mark, I think your problem is another bug, please file it.

Done.  See bug 613515.
Attached patch Mostly untested patch (obsolete) — Splinter Review
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).
Attachment #490013 - Attachment is obsolete: true
Attachment #490287 - Attachment is obsolete: true
Attachment #491682 - Attachment is obsolete: true
Per discussion with mrbkap and gal we don't need to finalize this for beta8, pushing to beta9.
blocking2.0: beta8+ → beta9+
(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.
I'd thought this was one of the critical issues for beta 8, too. Can someone elaborate on why it's being bumped?
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.
blocking2.0: beta9+ → beta8+
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.
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.
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).
(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.
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.
Attached patch patch v1Splinter Review
I just pushed this to try, though on first glance it appears to work.
Attachment #492466 - Attachment is obsolete: true
Attachment #494598 - Flags: review?(peterv)
Attachment #494598 - Flags: review?(jst)
Attachment #494598 - Flags: review?(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.
Attachment #494598 - Flags: review?(gal) → review+
For the record, it's looking very likely that this will land today, and thus be fixed in beta8.
Duplicate of this bug: 611982
http://hg.mozilla.org/tracemonkey/rev/08c80bfca302
Whiteboard: [compartments], [Input] → [compartments], [Input] fixed-in-tracemonkey
This landed on mozilla-central last night, marking FIXED!

http://hg.mozilla.org/mozilla-central/rev/a4813c8be814
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Yeah! Now it works perfectly. Thanks.
Attachment #494598 - Flags: review?(jst) → review+
we need to figure out if this could be connected to the crash regression on dec 4 in bug 617048
Blocks: 611175
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?
The solution from bug 428653 comment 25 applies here, too.
Comment on attachment 494598 [details] [diff] [review]
patch v1

gal and jst r+'ed, don't think I can add anything (well, clownshoes+).
Attachment #494598 - Flags: review?(peterv)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.