Closed Bug 580128 Opened 10 years ago Closed 10 years ago

Implement brain transplants

Categories

(Core :: XPConnect, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: mrbkap, Assigned: mrbkap)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [compartments] fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

In the patch for bug 563106, I added the line:

  // XXX Brain transplant outer window JSObject and create new one!

this needs to turn into real code.
Assignee: nobody → gal
Burns:    [saws off the top of Homer's head.  No blood, very clean.
           The top of Homer's head rolls away.]
          Smithers, hand me that ice-cream scoop.
Smithers: Ice-cream scoop?!
Burns:    Dammit, Smithers, this isn't rocket science, it's brain surgery!

("If I Only Had a Brain", from "Treehouse of Horror II")

/be
Andreas, what's the plan here?

We want to transplant the outer object's brain from one JSObject to another as we navigate from one compartment to the next (in ConnectToInner maybe). I worry about transplanting the brain of an object that's in internal XPConnect tables though. I think it will be easier if we make a new implementation of the outer window using proxies instead of XPConnect and scriptable helpers. The outer window could be a straightforward (non-cross-compartment) wrapper of the inner.
Our plan is to rewrite the outer object as a proxy and then we are doing a very limited brain transplant (switch handlers) between the outer window (proxy) and the wrapper (proxy).
Attached patch patch (obsolete) — Splinter Review
v1, implementing the outer window proxy
Next step: go through the compartment lists and update/swap the wrappers.

I can't test this well until we have more xpconnect changes from blake in place.
Depends on: 574974
Blocks: 584198
Reminder - we need to make sure that XrayWrappers see through the new outer window proxy to the underlying C++ object (as XPCNativeWrappers do for the old-style outer window XPCWNs now).
Duplicate of this bug: 569000
blocking2.0: --- → beta5+
Depends on: 586083
I'm working on this. <http://hg.mozilla.org/users/mrbkap_mozilla.com/brain-transplants> is my current patch queue. With the patches in the queue, the browser starts, but there are a couple of known bugs to fix.
Assignee: gal → mrbkap
This isn't necessary for beta5 with the feature freeze moved out, pushing to beta6.
blocking2.0: beta5+ → beta6+
Blocks: 592992
Blocks: lazy-console
Blocks: devtools4b7
Is this going to land in beta 6? This bug is blocking the Web Console feature (or, at least, the lazily created "console" object that is the new basis for the Web Console feature).
In Blake's patch queue. Implemented and working except location. document.domain probably working right but needs tests.
Attached file Patches for peterv
The patches are still in the patch queue. These are the patches that I need review from peterv on.
Attachment #462230 - Attachment is obsolete: true
Attachment #475683 - Flags: review?(peterv)
Whiteboard: [ETA: ??]
Comment on attachment 475683 [details]
Patches for peterv

>optionelement-hiding-getsettext             -- I got tired of seeing these warnings

r=peterv

=========

>special-case-xpconnect                      -- Adds a way to special case an object to
>					       not have a wrapped native in XPConnect

>+    mWrapperPtrBits = reinterpret_cast<PtrBits>(aWrapper) | (mWrapperPtrBits & WRAPPER_IS_PROXY);

Long line.

r=peterv

=========

>system-principal-consistant-origin          -- Makes the system principal consistent
>					       with the JSPrincipal for it.

r=peterv

=========

>create-proxy-for-outer			    -- Makes us create a proxy for the outer
>					       window instead of a wrapped native.

>diff --git a/js/src/xpconnect/src/XPCWrapper.cpp b/js/src/xpconnect/src/XPCWrapper.cpp

> JSObject *
> Unwrap(JSContext *cx, JSObject *wrapper)
> {
>+  if (wrapper->isProxy()) {
>+    if (wrapper->getProxyHandler() != &JSCrossCompartmentWrapper::singleton) {
>+      // XXX Security check!

What's this about?

>diff --git a/js/src/xpconnect/src/xpcconvert.cpp b/js/src/xpconnect/src/xpcconvert.cpp

>@@ -1143,386 +1143,256 @@ XPCConvert::NativeInterface2JSObject(XPC

>+    NS_ASSERTION(!cache || !cache->IsProxy(),
>+                 "Can't have a proxy cache past this point");

Don't really need this anymore I think.

>+    if(!XPCPerThreadData::IsMainThread(lccx.GetJSContext()) || !allowNativeWrapper)

Long line.

>+    if(!accc.enter(ccx, callee) || !JS_WrapObject(ccx, &flat))
>+        return JS_FALSE;
>+
>+    *d = OBJECT_TO_JSVAL(flat);
>+    if(dest)
>+    {
>+        // The strongWrapper still holds the original flat object.
>+        if(OBJECT_TO_JSVAL(flat) == *d)

How can this not be true, shouldn't you compare to the original flat from before calling JS_WrapObject? Maybe you wanted to set |*d = OBJECT_TO_JSVAL(flat)| before the call to JS_WrapObject.

=========

>nuke-outerwindow-sh                         -- Gets rid of the (now unused)
>					       nsOuterWindowSH

r=peterv.

=========

>no-wn-for-outer                             -- Fixes places that assumed that XPConnect
>					       hands out outer windows and makes them
>					       deal with inner windows.

r=peterv

=========

>leak-fix				    -- Fixes the leak introduced by
>					       create-proxy-for-outer

> nsresult
> nsJSContext::CreateOuterObject(nsIScriptGlobalObject *aGlobalObject,
>                                nsIScriptGlobalObject *aCurrentInner)
> {
>   nsCOMPtr<nsIDOMChromeWindow> chromeWindow(do_QueryInterface(aGlobalObject));
>   PRUint32 flags = 0;
> 
>+  mGlobalObjectRef = aGlobalObject;
>+

I'd do this first in CreateOuterObject.

r=peterv

=========

>window-wrappedJSObject-notwrapped           -- Because we don't wrap chrome:// windows
>					       in content docshells when passed into
>					       chrome code, we need to deal with
>					       extraneous .wrappedJSObject calls.

r=peterv

=========

>define-Window-constructor		    -- Makes sure Window is defined

This needs to work for all pages, not just the first.

=========

>quickstubs-globalwindow-wrappercache-warning

r=peterv
Attachment #475683 - Flags: review?(peterv) → review-
Some updates pushed to the queue. Some numbers:

$ hg diff -r qparent:. | diffstat
55 files changed, 1499 insertions(+), 1414 deletions(-)
$ hg qser | wc -l
40

Note that the second number is inflated somewhat by some small patches that fix compartment warnings. Still chugging along. We run the first group of mochitests but are currently getting shot down by compartment assertions under XBL-land. Still debugging...
$ hg qser | wc -l
72
Blocks: 601006
Whiteboard: [ETA: ??] → [compartments]
Depends on: 601768
$ hg qser | wc -l
     102
Please note that we have now created a branch for beta 7 work. In addition to landing your fix on mozilla-central default, please also land it on mozilla-central GECKO20b7pre_20101006_RELBRANCH

(note: when landing on mozilla-central default, you will be given priority in the landing queue at https://wiki.mozilla.org/LandingQueue )
$ hg qser | wc -l
     128
On tracemonkey:

remote: added 136 changesets with 420 changes to 149 files
Whiteboard: [compartments] → [compartments] fixed-in-tracemonkey
(and for the record: diffstat says:

157 files changed, 4229 insertions(+), 9938 deletions(-))
Depends on: 604087
Depends on: 604564
http://hg.mozilla.org/mozilla-central/rev/5378adc0eb9f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 604612
Depends on: 604362
Depends on: 603531
Depends on: 603677
Depends on: 604957
I just ended up landing a patch on the b7 branch that changes the window utils iid.  I generated a new IID, because what's there matches nothing else on m-c, but once this is merge to that branch the IIDs on branch and m-c should match.
Depends on: 603535
Depends on: 606058
Depends on: 606077
Depends on: 605001
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Depends on: 622259
Depends on: 623614
Blocks: 634752
No longer blocks: 634752
Depends on: 634752
Depends on: 643233
Adding dev-doc-needed because this removed XPCSafeJSObjectWrapper (which devmo just says "New in Firefox 3"), and a few places such as nsProxyAutoConfig.js and venkman-static.js still use it. Sorry for the bugspam.
Keywords: dev-doc-needed
Depends on: 759082
You need to log in before you can comment on or make changes to this bug.