Implement brain transplants

RESOLVED FIXED in mozilla2.0b7

Status

()

RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

({dev-doc-needed})

Trunk
mozilla2.0b7
x86_64
Linux
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

(Whiteboard: [compartments] fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

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.

Updated

9 years ago
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.

Comment 3

9 years ago
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).

Comment 4

9 years ago
Posted patch patch (obsolete) — Splinter Review

Comment 5

9 years ago
v1, implementing the outer window proxy

Comment 6

9 years ago
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.

Updated

9 years ago
Depends on: 574974

Updated

9 years ago
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

Updated

9 years ago
blocking2.0: --- → beta5+

Updated

9 years ago
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
Depends on: 591136
This isn't necessary for beta5 with the feature freeze moved out, pushing to beta6.
blocking2.0: beta5+ → beta6+

Updated

9 years ago
Blocks: 592992

Updated

9 years ago
Blocks: 568629

Updated

9 years ago
Blocks: 593957

Comment 11

9 years ago
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).

Comment 12

9 years ago
In Blake's patch queue. Implemented and working except location. document.domain probably working right but needs tests.
Posted 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)
Depends on: 597104
Depends on: 597109
Depends on: 597114
Depends on: 597116
Depends on: 597118
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

Updated

9 years ago
Blocks: 601006

Updated

9 years ago
Whiteboard: [ETA: ??] → [compartments]

Updated

9 years ago
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
Depends on: 603152
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

Updated

9 years ago
Depends on: 604564

Comment 23

9 years ago
http://hg.mozilla.org/mozilla-central/rev/5378adc0eb9f
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
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

Updated

9 years ago
Depends on: 606077
Depends on: 606138
Depends on: 605001
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7

Updated

8 years ago
Depends on: 622259
Depends on: 623614

Updated

8 years ago
Blocks: 634752
No longer blocks: 634752
Depends on: 634752

Updated

8 years ago
Depends on: 643233

Comment 25

8 years ago
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.