296639 - (splitwindows) Split windows into an inner and outer object

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

We should split windows into two objects -- inner and outer.  The inner object
is the scope for the document, while the outer is what we hand out for
"external" access (things like window.open, window.frames, etc,
iframe.contentWindow, etc).  The outer object passes all access through to the
inner object; when we load a new URL we replace the inner object that the outer
object passes through to.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note that we have an existing bug about event listeners getting JS errors
because they're firing on a cleared JS scope that would be fixed by this.  I
can't find the bug#, though.... :(

Comment 2

[Brendan Eich [:brendan]]

We can use JSExtendedClass to make outer and inner window objects == one another
(but not ===, a slight incompatibility that we should push).

Fixing this bug will help us in a number of ways:
- Faster object principal checks.
- Even faster fast-back navigation: no property copying, just object reference
swapping.
- Better usability when loading a new page over an old one: while the old page's
presentation is visible, you should be able to interact with its DOM.  IE does
this and we have bugs about not doing it.
- This architecture fix should help fix bug 114461, which is frequently dup'ed.

Let's come up with a plan on wiki.mozilla.org to fix this for 1.9.

/be

Comment 3

[Brendan Eich [:brendan]]

If we do this right, we can even avoid all security checks in the inner window
object's GetProperty and SetProperty hooks.  We need to prove that references to
the inner window can't leak out of that window's sandbox.

/be

Comment 4

[Benjamin Smedberg]

yay!

Comment 5

[Boris Zbarsky [:bzbarsky]]

Rambly WikiThoughts at http://wiki.mozilla.org/Gecko:SplitWindow (which I linked
from the content team minutes).

Comment 6

[Johnny Stenback (:jst)]

Attachment: WIP, take one.

This is sortof limping along, with window objects split into inner/outer.
Nowhere close to done, but it builds and mostly runs, and leaks (a lot). If
you're planing to give this a spin, find me on IRC before you spend too much
time on investigating odd behaviour...

Comment 7

[Aaron Leventhal]

This might not be compatible with the current accessibility module. It could
break screen reader access. I haven't tested that yet, or looked at how much
work it might take to make accessibility compatible with this change.

Comment 8

[Johnny Stenback (:jst)]

Attachment: diff -w of the branch, no new leaks (well, very few at the most)!

This is ready for reviewers to start looking at, there's still some issues to
figure out (you'll see some XXXjst comments in there), I'll be working on
those, but in the mean time, please do review and give feedback.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 190824 [details] [diff] [review]
diff -w of the branch, no new leaks (well, very few at the most)!

I still haven't gotten to a point where I've fine-combed anything useful...
General comments follow.  First off, this looks much better than we had any
right to hope.	I don't see any obvious blockers, though there's a good deal of
tedious checking to do...

One thing, though.  Do we need to worry about people doing pointer equality
comparisons on nsIDOMWindow or nsIScriptGlobalObject or whatever pointers and
getting confused in the new world?  I suspect we do, but I don't know a good
way to search for such code.. :(  For nsPIDOMWindow, at least, might be able to
define an operator== that we can either do something sane in or at least warn
(assert?) in...

Note to self: need to check callers of GetScriptGlobalObject() and callers of
JS_GetGlobalObject().

>Index: content/base/src/nsDocument.cpp

Document on nsIDocument that the return value of GetScriptGlobalObject() is the
inner window?

Would it make sense to have a separate getter for the outer window and move all
callers that don't absolutely need the inner one to that?

>@@ -2904,23 +2905,29 @@ nsDocument::CreateTreeWalker(nsIDOMNode 

>+  if (win) {
>+    nsPIDOMWindow *view =
>+      win->IsInnerWindow() ? win->GetOuterWindow() : win.get();
>+
>+    return CallQueryInterface(view, aDefaultView);

I think GetOuterWindow() should just return |this| for an outer window.  Then
this code could just be:

if (win) {
  return CallQueryInterface(win->GetOuterWindow(), aDefaultView);
}

or something along those lines.  Similar for GetInnerWindow.  That should make
code in a few places simpler...

>Index: content/events/src/nsEventListenerManager.cpp

>@@ -1217,21 +1217,24 @@ nsEventListenerManager::AddScriptEventLi
> 
>+    if (!scope) {
>+      scope = ::JS_GetGlobalObject(cx);
>+    }

When will this happen?	Should we just throw when it does?

>Index: content/xbl/src/nsXBLBinding.cpp

>@@ -1108,36 +1108,44 @@ nsXBLBinding::InitClass(const nsCString&
>+  nsIDocument *ownerDoc = mBoundElement->GetOwnerDoc();
>+  nsIScriptGlobalObject *sgo;
>+
>+  if (!ownerDoc || !(sgo = ownerDoc->GetScriptGlobalObject())) {
>+    NS_ERROR("Can't find global object for bound content!");

I think that we should just pass around the scope JSObject through XBL binding
init.  Having to do this in InitClass, various InstallMember impls, etc, seems
silly.	Can do a followup bug on this if you want.

>Index: content/xbl/src/nsXBLPrototypeBinding.cpp
>@@ -737,21 +737,26 @@ nsXBLPrototypeBinding::InitClass(const n
>+  while ((tmp = ::JS_GetParent(cx, global))) {
>+    global = tmp;

This code uses its own JSContext thing (not off an nsGlobalWindow), iirc, so
this might not matter... Worth checking, possibly again in a separate bg.

>Index: dom/public/base/nsPIDOMWindow.h

It'd be good to document here, and in other window classes, which
members/methods are handled by inner and which are handled by outer windows. 
Document possibly in a single block, not at each member (or in addition to each
member?) so that there's one spot where the whole setup is explained.

>+  PRBool IsInnerWindow() const
>+  {
>+    return mOuterWindow != nsnull;
>+  }
>+
>+  PRBool IsOuterWindow() const
>+  {
>+    return !IsInnerWindow();
>+  }

>+  nsPIDOMWindow         *mInnerWindow;
>+  nsPIDOMWindow         *mOuterWindow;

These are non-owning pointers...  Who's responsible for keeping them up to
date?  If an object dies, does that effectively mean that an inner window can
start thinking it's an outer window?  Note also that as the code is written an
outer window will never become inner...  Purposeful asymmetry, or just the way
it came out?  Perhaps the inner/outer identity should be a boolean in addition
to the ptrs?  And deal with cases when an inner has no outer or vice versa?

That said, what is the ownership model of the two windows and their global
objects anyway?  Per our discussion on irc, I believe the document holds a ref
to the inner, the script context a ref to the outer.  The docshell holds the
outer.	What dose the securityUI hold?	I'm betting the outer... What about
editor?  What about command manager?

In the new world, does a document's mScriptGlobal ever go null?  If not, that's
great!

>Index: dom/src/base/nsDOMClassInfo.cpp

> nsWindowSH::DelProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>+      printf("Property '%s' del on outer window %p\n",
>+             NS_ConvertUTF16toUTF8(str).get(), (void *)win);

This isn't wrapped in a relevant debug ifdef that I can see.

>Index: js/src/jsapi.h

I'd really like shaver or brendan to look over the jseng changes, of course...
(look over like r=, not sr=).

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)

> I'd really like shaver or brendan to look over the jseng changes, of course...
> (look over like r=, not sr=).

Just between you and me, I wrote the js*.[ch] patch, so can't self-review.  Back
to paternity leave....

/be

Comment 11

[Simon Fraser [no longer active]]

What's happening about the test failure on tinderbox?

Comment 12

[Johnny Stenback (:jst)]

Spent a bunch of time with people on #developers, but no luck in identifying
exactly what's not working yet. Any help would be greatly appreciated.

Comment 13

[Johnny Stenback (:jst)]

Just to set the record straight here, this change got checked in on Saturday
7/30, at ~2pm. Most tinderboxes are not happy, but the builds seem to work fine.
So far no good clues as to what the problem could be. I'll be posting the diffs
from the last diffs here to the state of the branch at the point where I merged
it to the trunk and landed it. I'll post full trunk diffs here too.

Comment 14

[Johnny Stenback (:jst)]

Attachment: branch changes since the previous diff.

Comment 15

[Johnny Stenback (:jst)]

Attachment: Patch that was checked in on the trunk.

Comment 16

[Johnny Stenback (:jst)]

Attachment: Back out the removal of mDocumentPrincipal.

We didn't need this code initially when I made these changes since we weren't
forcing a release of mDocument in the window code, but that caused a reference
cycle that caused us to leak the world (thanks for dbaron for leading me to
that).

Comment 17

[Johnny Stenback (:jst)]

(hit submit too early there)...

But now that we reset mDocument again when leaving a page, we need to remember
the principals from that document so that security checks that happen after a
window is torn down don't always fail.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 191123 [details] [diff] [review]
Back out the removal of mDocumentPrincipal.

This seems reasonable to me, although bz and shaver should probably review it
as well.  The comment in the header file should probably say that it's only set
once the window is torn down (for both), assuming that's actually the case.

Comment 19

[jag (Peter Annema)]

This has increased Tp a bit, roughly 9ms on btek and 8ms on monkey. Not
complaining, just noting.

Comment 20

[Boris Zbarsky [:bzbarsky]]

This also regressed Tdhtml a little tiny bit, as far as I can see.  Not exactly
surprising, and in 1.9 I expect us to use this split to get even bigger wins there.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 190824 [details] [diff] [review]
diff -w of the branch, no new leaks (well, very few at the most)!

>Index: dom/src/base/nsDOMClassInfo.cpp

>   NS_DEFINE_CLASSINFO_DATA(Window, nsWindowSH,
>                            DEFAULT_SCRIPTABLE_FLAGS |
>                            nsIXPCScriptable::WANT_GETPROPERTY |
>                            nsIXPCScriptable::WANT_SETPROPERTY |
>                            nsIXPCScriptable::WANT_PRECREATE |
>                            nsIXPCScriptable::WANT_FINALIZE |
>                            nsIXPCScriptable::WANT_ADDPROPERTY |
>                            nsIXPCScriptable::WANT_DELPROPERTY |
>                            nsIXPCScriptable::WANT_ENUMERATE |
>+                           nsIXPCScriptable::WANT_EQUALITY |
>+                           nsIXPCScriptable::WANT_OUTER_OBJECT |
>                            nsIXPCScriptable::DONT_ENUM_QUERY_INTERFACE)

Any reason you didn't also modify the ChromeWindow classinfo data's flags?

Comment 22

[Johnny Stenback (:jst)]

Attachment: Add new flags to ChromeWindow too

Um, no. Simply just didn't think of that, even though I should've...

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 191397 [details] [diff] [review]
Add new flags to ChromeWindow too

r+sr=dbaron, but consolidating these in a macro might be nice

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: clean up from btek bustage fix; fix window Finalize hook

This cleans up from the btek bustage fix where I reordered the base classes to
work around what could probably be fixed a better way (but reordering has
significant codesize benefits), and also fixes nsWindowSH::Finalize to call the
base class Finalize (and older regression, but probably worse with this patch).

Comment 25

[Boris Zbarsky [:bzbarsky]]

>Index: content/base/public/nsIDocument.h

>+  Note that
>+   * this is the *inner* window object.

That's not the case when mIsGoingAway in the document.  In that case we get the
script global object from the docshell, so it'd be the outer window.

Though I've been thinking about this, and I think we want to be using the outer
window in all cases except when calling GetGlobalJSObject(); that will make sure
that people's pointer compares work.  So either this method should return an
outer window (and we should have a separate method for the few callers that want
an inner one), or we should add a new method for getting the outer window and
move most of the GetScriptGlobalObject() (and some mScriptGlobalObject)
consumers to that.  I'm assuming that we'll do something like this, so I'm not
checking all GetScriptGlobalObject() callers.

This is a problem in particular in Destroy(), where by the time we unbind our
kids we'll have the wrong script global.  That will likely cause problems with
the code in nsXBLBinding::ChangeDocument, since that uses the
GetGlobalJSObject() off the script global it gets from the document as the scope
arg to WrapNative, and this will be the outer window JSObject.  I think that
when SetScriptGlobalObject(nsnull) is called, we should stash
mScriptGlobalObject in a different variable and continue to return it for
GetScriptGlobalObject (so remove the mIsGoingAway block in that method, basically).

>Index: content/base/src/nsDocument.cpp

> nsDocument::GetDefaultView(nsIDOMAbstractView** aDefaultView)

>+    // The default view is our outer window.
>+    if (!win->IsInnerWindow()) {

If we make it so the "inner or outer" identity of a window is immutable, we
don't need this check, right?

Please add an assert to SetScriptGlobalObject that the object passed in is an
inner window.

>Index: content/events/src/nsEventListenerManager.cpp

>@@ -1166,25 +1162,28 @@ nsEventListenerManager::AddScriptEventLi

>     nsCOMPtr<nsIDOMWindow> win(do_QueryInterface(aObject));
>     nsCOMPtr<nsIScriptGlobalObject> global;
>     if (win) {

Will |win| always be the inner window here?  If so, that should probably be
asserted.  If not, do we really want to be adding event listeners on the outer
window?

>Index: content/events/src/nsEventStateManager.cpp

>+GetDocumentOuterWindow(nsIDocument *aDocument)

This is what I thikn should live on nsIDocument...

>+GetInnerDocument(nsISupports *aWindow)

Why is this needed if mDocument is kept in sync between the inner and outer
window?  Especially between the current inner and outer?  Why not just get the
extant document off the outer?

>Index: content/html/content/src/nsGenericHTMLElement.cpp

>@@ -3431,21 +3431,27 @@ nsGenericHTMLFrameElement::GetContentWin

>   nsCOMPtr<nsIDOMWindow> win(do_GetInterface(doc_shell));
>-  win.swap(*aContentWindow);
>+  nsCOMPtr<nsPIDOMWindow> piwin(do_QueryInterface(win));
> 
>-  return NS_OK;
>+  if (piwin && piwin->IsInnerWindow()) {
>+    // We got an inner window here somehow, this just should not happen.

Indeed.  I'd almost say replace this with an assert (or do both if we want to
lay it safe for now; I definitely want an assert here, though).

>+  return CallQueryInterface(piwin->GetOuterWindow(), aContentWindow);

No need for the GetOuterWindow() call; we've already bailed if piwin is an inner....

>Index: content/html/document/src/nsHTMLDocument.cpp

Should this really be passing the inner window to MakeWindowEditable()?  All
other callers pass the outer window.  Maybe it doesn't matter, but it'd be
better to be consistent, I think.  Same thing for the command manager DoCommand
calls in nsHTMLDocument::ExecCommand, for the code in
nsHTMLDocument::QueryCommandEnabled, nsHTMLDocument::QueryCommandIndeterm,
nsHTMLDocument::QueryCommandState, nsHTMLDocument::QueryCommandValue, etc.

Actually, this seems to be causing some midas bustage, so we really should
change to passing the outer everywhere here.

>Index: content/xbl/src/nsXBLPrototypeBinding.cpp

>@@ -737,21 +737,26 @@ nsXBLPrototypeBinding::InitClass(const n
>+  while ((tmp = ::JS_GetParent(cx, global))) {
>+    global = tmp;

We should just pass the global to InitClass().  Both callers have it readily
available.

>Index: content/xbl/src/nsXBLPrototypeHandler.cpp

>   // if the focused window was found get our script global object from
>   // that.

I know you didn't really change this code, but this should only be happening for
XUL <key>, right?  Might be worth asserting that; I'd really hate it if we
compiled random XBL handlers against whatever the focused window is.

>+    if (piWin && piWin->GetCurrentInnerWindow()) {
>+      piWin = piWin->GetCurrentInnerWindow();

Do we really want to use the outer if there is no inner?  I'd think we want to
bail out.

>Index: dom/public/nsIScriptContext.h
>+  /**
>+   * Initialize DOM classes on aGlobalObj
>+   */
>+  virtual nsresult InitClasses(JSObject *aGlobalObj) = 0;

Does this have to be bracketed by WillInitializeContext and DidInitializeContext
calls?  If so, please document that?  And perhaps assert if InitClases() is
called and WillInitializeContext wasn't called before that (assuming we can
detect this; though if we can there seems to be no need for WillInitializeContext).

>Index: dom/public/base/nsPIDOMWindow.h

>+  void SetMutationListeners(PRUint32 aType)
>+    if (!win) {
>+      win = this;

Do we really want to set mMutationBits on the outer window in this case?  I'd
really rather bail out instead...

>   void SetFrameElementInternal(nsIDOMElement *aFrameElement)

And this can actually set mFrameElement if an inner loses its outer... Again, I
think it would be better to have a boolean for inner vs outer that doesn't
depend on whether the other object in the pair has died.

>   PRBool IsLoadingOrRunningTimeout() const
>+    const nsPIDOMWindow *win = GetCurrentInnerWindow();
>+
>+    if (!win) {

So is there a reason not to just return |this| for GetCurrentInnerWindow() for
an inner window?  Or is that not why you have the null-checks scattered about?

Again, I'm not sure we want to be looking at these members on an outer window...
> protected:
>+  nsPIDOMWindow(nsPIDOMWindow *aOuterWindow)

aOuterWindow should be null if and only if an outer window is being created? 
Please document?

>Index: dom/src/base/nsDOMClassInfo.cpp

> @@ -480,27 +481,30 @@ static nsDOMClassInfoData sClassInfoData
>   NS_DEFINE_CLASSINFO_DATA(Window, nsWindowSH,

Need to change the flags for ChromeWindow too?

> nsWindowSH::GetProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>+  if (win->IsOuterWindow() && !ObjectIsNativeWrapper(cx, obj)) {
...
>+      return NS_OK;

Should that be NS_SUCCESS_I_DID_SOMETHING?  Probably doesn't matter as things
stand, but still...

>   if (JSVAL_IS_INT(id)) {
>+      rv = WrapNative(cx, obj, frame, NS_GET_IID(nsIDOMWindow), vp);

Using |obj| here is wrong, no?  Consider the case when |obj| is an
XPCNativeWrapper, for example.  The right scope is probably the JSObject of the
inner window of |win|.  If there is no inner, then there really shouldn't be any
child frames either....  Perhaps assert that.  Or does XPConnect do the right
thing with random |scope| args here?

> nsWindowSH::AddProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,

I'd really like brendan or shaver to check the OBJ_DEFINE_PROPERTY fu here.

> nsWindowSH::DelProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,

>+#ifdef DEBUG_SH_FORWARDING
>+      printf(" --- Forwarding add to inner window %p\n", (void *)innerWin);
>+#endif

Forwarding del, not add.

>@@ -5046,20 +5233,19 @@ nsWindowSH::GlobalResolve(nsIScriptGloba

>+      rv = WrapNative(cx, obj, native, NS_GET_IID(nsISupports), &prop_val);

Again, using |obj| as the scope is wrong -- it could be an XPCNativeWrapper. 
And in that case |global| would be the outer window.  I suspect what we want
here is again to get the inner window, etc.

There's a WrapNative call in nsWindowSH::GlobalScopePolluterNewResolve that gets
the scope off |obj|.  I'm guessing that's ok?

What about the WrapNative in BaseStubConstructor?

> nsWindowSH::NewResolve(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>+      // be reused by the actual document being loaded into this outer
>+      // window. This way properties defined on the window while the
>+      // document before the document load stated

s/while the document// and s/stated/started/

>+        // Grab the new inner window.
>+        innerWin = win->GetCurrentInnerWindowInternal();

And if !innerWin after this, throw NS_ERROR_OUT_OF_MEMORY, please?

>@@ -5208,20 +5487,23 @@ nsWindowSH::NewResolve(nsIXPConnectWrapp
>     if (child_win) {
...
>+      JSObject *wrapperObj;
>+      wrapper->GetJSObject(&wrapperObj);
>+
>       jsval v;
>-      rv = WrapNative(cx, ::JS_GetGlobalObject(cx), child_win,
>+      rv = WrapNative(cx, wrapperObj, child_win,

If |obj| is an XPCNativeWrapper and |wrapper| is the outer window this seems
to do the wrong thing, no?


>@@ -5298,27 +5580,40 @@ nsWindowSH::NewResolve(nsIXPConnectWrapp
>+    // Make sure we wrap the location object in the inner window's
>+    // scope if we've got an inner window.

Right.  I think we should do this in general, not just for location.

>+    if (!scope) {
>+      scope = obj;

And if no inner window, maybe fall back to wrapper's JSObject, not |obj|?  Then
I think you can skip the GetGlobalJSObject() call for WrapNative() here and just
use the scope object we end up with.  At least I think so.  I could be wrong....

>@@ -5348,60 +5643,65 @@ nsWindowSH::NewResolve(nsIXPConnectWrapp
>     if (id == sNavigator_id) {
...
>       rv = WrapNative(cx, obj, navigator, NS_GET_IID(nsIDOMNavigator), &v);

Again, probably want to pass something else as the scope here.  Should this
always be the outer window scope?

>     if (id == sDocument_id) {
>+      rv = WrapNative(cx, obj, document, NS_GET_IID(nsIDOMDocument), &v);

Again, is that the right scope?

>+      if (!::JS_DefineUCProperty(cx, obj, ::JS_GetStringChars(str),
>+                                 ::JS_GetStringLength(str), v, nsnull,
>+                                 nsnull, JSPROP_READONLY | JSPROP_ENUMERATE)) {
>+        return NS_ERROR_FAILURE;

This code shouldn't be here.  It was moved to nsDocumentSH::PostCreate; please
just remove it from here.  Note the comment in the OnDocumentChanged method that
you removed....

>+NS_IMETHODIMP
>+nsWindowSH::Equality(nsIXPConnectWrappedNative *wrapper, JSContext * cx,
>+                     JSObject * obj, jsval val, PRBool *bp)

As you mentioned on IRC, this should probably be debug-only and assert that
we're not comparing an inner window to an outer window, since we should never
have such comparisons.

>+nsWindowSH::OuterObject(nsIXPConnectWrappedNative *wrapper, JSContext * cx,
>+  *_retval = win->GetOuterWindowInternal()->GetGlobalJSObject();

What if there is no outer window for |win| at this point (eg it's dead)?

>+  // XXXjst: Add code that asserts the the scope is an inner window

Please.

>@@ -6206,25 +6587,18 @@ documentNeedsSecurityCheck(JSContext *cx

>-  if (wrapper_global != ::JS_GetGlobalObject(cx)) {
>-    // cx is not the context in the global object in wrapper, force
>-    // a security check.
>-
>-    return PR_TRUE;

So... this was just an optimization for this optimization, right?

>@@ -7183,20 +7560,19 @@ nsHTMLDocumentSH::GetProperty(nsIXPConne
>+    rv = WrapNative(cx, obj, result, NS_GET_IID(nsISupports), vp);

|GetGlobalJSObject(cx, obj)| instead of |obj|, right?

>Index: dom/src/base/nsFocusController.cpp

> nsFocusController::SetFocusedWindow(nsIDOMWindowInternal* aWindow)
>+  nsCOMPtr<nsPIDOMWindow> pwin = do_QueryInterface(aWindow);
>+
>+  if (pwin) {
>+    pwin = pwin->GetOuterWindow();
>+  }

If we fix things so people have to try to get an inner window, could we make
this an assert instead?  I guess what we do is safer...

>@@ -300,30 +315,28 @@ nsFocusController::Focus(nsIDOMEvent* aE

>+nsFocusController::GetWindowFromDocument(nsIDOMDocument* aDocument)
>+  nsCOMPtr<nsPIDOMWindow> win =
>+    do_QueryInterface(doc->GetScriptGlobalObject());
>+
>+  if (win->IsInnerWindow()) {

It either always will be or never will be depending on what we decide to do with
nsIDocument.  In either case, that check can go away at that point.

>@@ -406,46 +419,44 @@ nsFocusController::GetControllerForComma
>+      if(controller) {
>+        *_retval = controller;
>+        NS_ADDREF(*_retval);
>+        return NS_OK;

How about controller.swap(*_retval) here?

>Index: dom/src/base/nsGlobalWindow.cpp

Should we check for a non-null mArguments in CleanUp() and clean it up if it's
there?  Or should that be an assert (in CleanUp or in ~nsGlobalWindow)?

>+nsGlobalWindow::nsGlobalWindow(nsGlobalWindow *aOuterWindow)
>+  // Window object's are also PRCList's, the list is for keeping all

s/object's/objects/

>+  // innter windows reachable from the outer so that the proper

s/innter/inner/

>+  // cleanup can be done on shutdown.

So what does the PRCList actually contain?  The outer and all its inners? 
Please document here or better yet in the header?

>+  if (aOuterWindow) {
>+    PR_INSERT_AFTER(aOuterWindow, this);
>+  }

I think you want PR_INSERT_AFTER(this, aOuterWindow).

>@@ -243,34 +285,56 @@ nsGlobalWindow::nsGlobalWindow()
>+  if (IsOuterWindow() && !PR_CLIST_IS_EMPTY(this)) {
...
>+    nsGlobalWindow *w = (nsGlobalWindow *)PR_LIST_HEAD(this);
...
>+    while ((w = (nsGlobalWindow *)PR_LIST_HEAD(this)) != this) {

So wouldn't this be clearer as:

if (IsOuterWindow()) {
  nsGlobalWindow *w;
  while ((w = (nsGlobalWindow*)PR_LIST_HEAD(this)) != this) {

(as in, the first PR_LIST_HEAD call you do here is redundant, as is the
PR_CLIST_IS_EMPTY check).

>+  } else {

And this else was running when IsOuterWindow() and PR_CLIST_IS_EMPTY(this), when
it doesn't need to...  If the code is changed as above, it'll only run for inner
windows.

> nsGlobalWindow::SetContext(nsIScriptContext* aContext)
> {
>+  NS_ASSERTION(IsOuterWindow(), "Uh, SetContext() called on inner window!");

Can we also assert that !aContext || !mContext?  I don't see a good reason both
would be non-null.

> nsGlobalWindow::SetNewDocument

>+  /* No mDocShell means we've either an inner window or we're already

"we're either"?

>+  /* We only want to do this when we're setting a new document rather
>+     than going away. See bug 61840.  */

Does this method even get called when going away?  If so, with what document? 
(I'm guessing it does not anymore.)

>+  // If we're in the middle of shutdown, nsContentUtils may have
>+  // already been notified of shutdown and may return null here.

How would we get into this code in the middle of shutdown?

> nsGlobalWindow::SetDocShell(nsIDocShell* aDocShell)

>   if (!aDocShell && mContext) {

This seems to share a lot of code with CleanUp().  Perhaps factor some of that out?

>     ClearAllTimeouts();
>+    if (currentInner) {
>+      currentInner->ClearAllTimeouts();

Hmm... So timeouts can live on either the outer or the inner? 
SetTimeoutOrInterval forwards to the inner, so do we really need the
ClearAllTimeouts() call on the outer there?

>+      // XXXjst: We shouldn't need to do this, but if we don't we leak
>+      // the world... actually, even with this we leak the
>+      // world... need to figure this out.

Is this comment still correct?

>+      if (currentInner->mJSObject) {
....
>+        // Release the current inner window's document references.
>+        currentInner->mDocument = nsnull;

Why only when currentInner->mJSObject is non-null?

> nsIPrincipal*
> nsGlobalWindow::GetPrincipal()
> {
>+  // XXXjst: figure out inner window stuff...

At a guess, we want to forward this to inner, and return null if there is no
inner?  Or something.

> nsGlobalWindow::GetDocument(nsIDOMDocument** aDocument)

Shouldn't this forward to outer if it's messing with the docshell?  At least for
the "messing with docshell" part?

Or would that do the wrong thing if an inner has an outer but no document and is
not the current inner?  In any case, we need to sort this out.

>@@ -2574,18 +2944,20 @@ nsGlobalWindow::Prompt(const nsAString& 

Is there a reason this version of Prompt() doesn't forward to outer?  It
probably should....

> nsGlobalWindow::Print()

This should probably forward to outer.

Same for MoveTo/MoveBy/ResizeTo/ResizeBy (I suspect they may not _have_ to, but
it would make things clearer).

> nsGlobalWindow::RestoreWindowState(nsISupports *aState)
> {
>+  // SetNewDocument() has already been called so we should have a
>+  // new clean inner window to restore stat into here.

"stat"?  You mean "state"?

> nsGlobalWindow::SuspendTimeouts()
> {
>+  FORWARD_TO_INNER_VOID(SuspendTimeouts, ());

This method uses mDocShell, so as things stand we'll fail to suspend timeouts in
child frames.

> nsGlobalWindow::ResumeTimeouts()
> {
>+  FORWARD_TO_INNER(ResumeTimeouts, ());

Again, this method uses mDocShell.

>Index: embedding/components/windowwatcher/src/nsWindowWatcher.cpp

nsWindowWatcher::AddInterfaceTojsvals passes JS_GetGlobalObject as the global
it wraps against.  That looks wrong.  Same for
nsWindowWatcher::AddSupportsTojsvals.  

>Index: layout/base/nsPresShell.cpp

>+    nsCOMPtr<nsPIDOMWindow> curWin =
>+      do_QueryInterface(curDoc->GetScriptGlobalObject());

This would just want to get the outer win...

>@@ -4353,24 +4358,25 @@ PresShell::GoToAnchor(const nsAString& a
>       nsCOMPtr<nsPIDOMWindow> win =
>         do_QueryInterface(mDocument->GetScriptGlobalObject());

Same here.

General questions/comments:

Should typeahead find be working with a mix of inner and outer windows like it
seems to be?  Probably ok, but worth watching out for...

nsSecurityNameSet::InitializeNameSet seems to end up working with the outer
window's JSObject (it uses JS_GetGlobalObject).  Is this what we want?

MozAxAutoPushJSContext::MozAxAutoPushJSContext seems to compile scripts against
the JS_GetGlobalObject of a cx.  Not sure whether windows are involved.

Liveconnect seems to compile scripts agains JS_GetGlobalObject.

There also seem to be some calls to JS_GetGlobalObject in XPConnect.  The
subscript loader ones are probably ok, but what about XPCDispConvert?  And
XPC_JSArgumentFormatter in xpcconvert.cpp?

ns4xPlugin.cpp also seesm to use JS_GetGlobalObject for WrapNative calls. Should
probably use the relevant window....

Same for nsCrypto.cpp.

Same for wrapper reparenting in nsContentUtils.cpp.

Same for nsDOMClassInfo::ThrowJSException.

Same for nsObjectFrame::NotifyContentObjectWrapper

nsDirectoryViewer.cpp seems to do some stuff with the outer window; this may be
ok, but worth checking.

Do we need to worry about inner windows being passed to things like the window
watcher as parents for prompts?  I think this should always pass outer windows.

map_java_object_to_js_object_impl (in modules/oji/src/lcglue.cpp) returns the
inner window JSObject.  Is that what we want?  I'm not really sure what this
code is doing.

IsChildOfDomWindow (in nsSecureBrowserUIImpl.cpp) does pointer compares on
nsIDOMWindows, where one nsIDOMWindow is always inner and one is at least
sometimes outer.  Probably needs to be fixed (by using outer windows throughout).

javascript: URIs are evaluated against the outer window JS object (the one they
get off the docshell).  They should probably use the inner window.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Wishlist (for 1.9?):

1) Clarify the ownership model.  I think it would make sense for the inner
   window to be owned by the document and for the outer window to own the
   document.  This eliminates our document->window->document cycle, I hope.

   If we do this right, we can maybe avoid having to null out
   mScriptGlobalObject on documents.  Have to watch out for the fact that
   documents clean up cycles through content on such sets.  And that some
   callers check for a null script global to see whether a document is "in a
   window".
2) Eliminate SaveWindowState -- just save the inner window in the bfcache, and
   restore it on restore.  That is, move further toward having the inner window
   attached to the document, not to the docshell/outer.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 191408 [details] [diff] [review]
clean up from btek bustage fix; fix window Finalize hook

Actually, ignore the nsDOMClassInfo.cpp change in this patch, there is no
finalize hook to call (although that's probably it's own bug).

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 191408 [details] [diff] [review]
clean up from btek bustage fix; fix window Finalize hook

r+sr=jst (for some reason I thought this was already checked in already, but
since it's not...)

Comment 29

[Johnny Stenback (:jst)]

Attachment: Cleanup based on bz's review feedback.

Comment 30

[Johnny Stenback (:jst)]

(In reply to comment #25)
> >Index: content/base/public/nsIDocument.h
> 
> >+  Note that
> >+   * this is the *inner* window object.
> 
> That's not the case when mIsGoingAway in the document.  In that case we get the
> script global object from the docshell, so it'd be the outer window.
> 
> Though I've been thinking about this, and I think we want to be using the outer
> window in all cases except when calling GetGlobalJSObject(); that will make sure
> that people's pointer compares work.  So either this method should return an
> outer window (and we should have a separate method for the few callers that want
> an inner one), or we should add a new method for getting the outer window and
> move most of the GetScriptGlobalObject() (and some mScriptGlobalObject)
> consumers to that.  I'm assuming that we'll do something like this, so I'm not
> checking all GetScriptGlobalObject() callers.
> 
> This is a problem in particular in Destroy(), where by the time we unbind our
> kids we'll have the wrong script global.  That will likely cause problems with
> the code in nsXBLBinding::ChangeDocument, since that uses the
> GetGlobalJSObject() off the script global it gets from the document as the scope
> arg to WrapNative, and this will be the outer window JSObject.  I think that
> when SetScriptGlobalObject(nsnull) is called, we should stash
> mScriptGlobalObject in a different variable and continue to return it for
> GetScriptGlobalObject (so remove the mIsGoingAway block in that method,
basically).

I fully agree, but I didn't change all of this in the last patch I attached,
mostly because I want to keep that in a separate diff to keep what's being
changed clearer.

[...]
> >Index: content/events/src/nsEventStateManager.cpp
> 
> >+GetDocumentOuterWindow(nsIDocument *aDocument)
> 
> This is what I thikn should live on nsIDocument...

Agreed. I added a GetWindow() method to nsIDocument, and that method returns the
outer window. I didn't change *all* users of GetScriptGlobalObject() in this
cleanup patch, but I've got one on the stove for the trunk (rather large change)...

> >Index: content/xbl/src/nsXBLPrototypeBinding.cpp
> 
> >@@ -737,21 +737,26 @@ nsXBLPrototypeBinding::InitClass(const n
> >+  while ((tmp = ::JS_GetParent(cx, global))) {
> >+    global = tmp;
> 
> We should just pass the global to InitClass().  Both callers have it readily
> available.

Fixed, but there's still some more type cleanup that could be done there. Lots
of void*'s being passed around there still for no good reason...

> >Index: dom/public/nsIScriptContext.h
> >+  /**
> >+   * Initialize DOM classes on aGlobalObj
> >+   */
> >+  virtual nsresult InitClasses(JSObject *aGlobalObj) = 0;
> 
> Does this have to be bracketed by WillInitializeContext and DidInitializeContext
> calls?  If so, please document that?  And perhaps assert if InitClases() is
> called and WillInitializeContext wasn't called before that (assuming we can
> detect this; though if we can there seems to be no need for
WillInitializeContext).

This was all cleaned up as part of bug 303420, except the documentation that I
added in the patch I just attached.

[...]
> >@@ -5046,20 +5233,19 @@ nsWindowSH::GlobalResolve(nsIScriptGloba
> 
> >+      rv = WrapNative(cx, obj, native, NS_GET_IID(nsISupports), &prop_val);
> 
> Again, using |obj| as the scope is wrong -- it could be an XPCNativeWrapper. 
> And in that case |global| would be the outer window.  I suspect what we want
> here is again to get the inner window, etc.

I changed WrapNative() to get the global from the given scope. It can still end
up giving us the outer when we want the inner, but that's ok since we'll get it
right in our PreCreate() hooks anyways. We're looking at finding the right
balance over letting that save us over adding code in a bunch of places to find
the right inner/outer, for now I think what my patch does is good enough.

> >+NS_IMETHODIMP
> >+nsWindowSH::Equality(nsIXPConnectWrappedNative *wrapper, JSContext * cx,
> >+                     JSObject * obj, jsval val, PRBool *bp)
> 
> As you mentioned on IRC, this should probably be debug-only and assert that
> we're not comparing an inner window to an outer window, since we should never
> have such comparisons.

Yes, I'll add that. I just realized that this didn't make it into the diff I
just attached.

> >+nsWindowSH::OuterObject(nsIXPConnectWrappedNative *wrapper, JSContext * cx,
> >+  *_retval = win->GetOuterWindowInternal()->GetGlobalJSObject();
> 
> What if there is no outer window for |win| at this point (eg it's dead)?

I made us return null in such a case.

> >+  // XXXjst: Add code that asserts the the scope is an inner window
> 
> Please.

I can't actually do that since when properties on a window are accessed from
another window we'll end up wrapping them in the outer window's scope, and we
should IMO do that since there's no clean way to get to the inner etc. We'll do
the right thing regardless AFAICT.

> >@@ -6206,25 +6587,18 @@ documentNeedsSecurityCheck(JSContext *cx
> 
> >-  if (wrapper_global != ::JS_GetGlobalObject(cx)) {
> >-    // cx is not the context in the global object in wrapper, force
> >-    // a security check.
> >-
> >-    return PR_TRUE;
> 
> So... this was just an optimization for this optimization, right?

Yes, one that no longer makes sense.

> > nsGlobalWindow::SetDocShell(nsIDocShell* aDocShell)
> 
> >   if (!aDocShell && mContext) {
> 
> This seems to share a lot of code with CleanUp().  Perhaps factor some of that
out?

I had a quick look but I didn't see enough shared code to factor out. I'm happy
to do that later tho if there's more than I realized...

> >     ClearAllTimeouts();
> >+    if (currentInner) {
> >+      currentInner->ClearAllTimeouts();
> 
> Hmm... So timeouts can live on either the outer or the inner? 
> SetTimeoutOrInterval forwards to the inner, so do we really need the
> ClearAllTimeouts() call on the outer there?

Only inner windows have timeouts, I removed the extra call and added asserts...

> >+      // XXXjst: We shouldn't need to do this, but if we don't we leak
> >+      // the world... actually, even with this we leak the
> >+      // world... need to figure this out.
> 
> Is this comment still correct?

I *think* it is, the good window->document->eventhandlers->context->window cycle
is still making this necessary...

> > nsIPrincipal*
> > nsGlobalWindow::GetPrincipal()
> > {
> >+  // XXXjst: figure out inner window stuff...
> 
> At a guess, we want to forward this to inner, and return null if there is no
> inner?  Or something.

All cleared up in bug 296639.

> 
> > nsGlobalWindow::GetDocument(nsIDOMDocument** aDocument)
> 
> Shouldn't this forward to outer if it's messing with the docshell?  At least for
> the "messing with docshell" part?
> 
> Or would that do the wrong thing if an inner has an outer but no document and is
> not the current inner?  In any case, we need to sort this out.

Decided not to forward for now, and added comment explaining why.

> >@@ -2574,18 +2944,20 @@ nsGlobalWindow::Prompt(const nsAString& 
> 
> Is there a reason this version of Prompt() doesn't forward to outer?  It
> probably should....

No other reason than "it didn't need to", but for clarity I made it forward...

> General questions/comments:
> 
> Should typeahead find be working with a mix of inner and outer windows like it
> seems to be?  Probably ok, but worth watching out for...

That'll be fixed up eventually (in some other cleanup patches I've got for 1.9)...

> nsSecurityNameSet::InitializeNameSet seems to end up working with the outer
> window's JSObject (it uses JS_GetGlobalObject).  Is this what we want?

Hard to say. I started changing that, but since it doesn't really matter (as all
property get/set etc are forwarded to the inner) I decided to not chnage it.
Changing it means changing the API...

> MozAxAutoPushJSContext::MozAxAutoPushJSContext seems to compile scripts against
> the JS_GetGlobalObject of a cx.  Not sure whether windows are involved.

Seems like that's fine as is.

> Liveconnect seems to compile scripts agains JS_GetGlobalObject.

I *believe* that's ok, and it looks like it'd be hard to change too.

> There also seem to be some calls to JS_GetGlobalObject in XPConnect.  The
> subscript loader ones are probably ok, but what about XPCDispConvert?  And
> XPC_JSArgumentFormatter in xpcconvert.cpp?

AFAICT those are fine too, and also hard to fix...

> ns4xPlugin.cpp also seesm to use JS_GetGlobalObject for WrapNative calls. Should
> probably use the relevant window....

I actually want the plugin code to use the outer since we don't know what it'll
do with it. Using the outer should work there...

> Same for nsCrypto.cpp.
> 
> Same for wrapper reparenting in nsContentUtils.cpp.
> 
> Same for nsDOMClassInfo::ThrowJSException.

AFAICT those are all ok.

> Same for nsObjectFrame::NotifyContentObjectWrapper

Fixed.

> nsDirectoryViewer.cpp seems to do some stuff with the outer window; this may be
> ok, but worth checking.

Yeah, looks ok.

> Do we need to worry about inner windows being passed to things like the window
> watcher as parents for prompts?  I think this should always pass outer windows.

Yup, added asserts.

> map_java_object_to_js_object_impl (in modules/oji/src/lcglue.cpp) returns the
> inner window JSObject.  Is that what we want?  I'm not really sure what this
> code is doing.

Fixed, I made the plugin code return the outer window so that this will work
with the outer window. I think that's safer since we don't know what plugins may
do with the window object...

> IsChildOfDomWindow (in nsSecureBrowserUIImpl.cpp) does pointer compares on
> nsIDOMWindows, where one nsIDOMWindow is always inner and one is at least
> sometimes outer.  Probably needs to be fixed (by using outer windows throughout).

Fixed.

> javascript: URIs are evaluated against the outer window JS object (the one they
> get off the docshell).  They should probably use the inner window.

Fixed.

Thanks for the review! Things are starting to look pretty good here, even if
there's still work to be done here.

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 192421 [details] [diff] [review]
Cleanup based on bz's review feedback.

See previous comments for discussion about what's being changed here.

Comment 32

[Johnny Stenback (:jst)]

Attachment: Followup patch that was checked in (reviews pending).

Comment 33

[amano]

Does that fix bug #302885 as well?

Comment 34

[Aaron Leventhal]

This appears to have broken tabbing out of the location bar. Eek!

I suspect the changes to nsEventStateManager.cpp, but it could be something else
from yesterday I suppose.

Comment 35

[Tracy Walker [:tracy]]

(In reply to comment #34)
> This appears to have broken tabbing out of the location bar. Eek!
> 
> I suspect the changes to nsEventStateManager.cpp, but it could be something else
> from yesterday I suppose.

confirmed...however, shift+tab moves focus out of the location bar.

Comment 36

[Peter van der Woude [:Peter6]]

(In reply to comment #34)
> This appears to have broken tabbing out of the location bar. Eek!
> 
> I suspect the changes to nsEventStateManager.cpp, but it could be something else
> from yesterday I suppose.

Nope,this

works in the 20050811 1727pdt build
fails in the 20050811 1856pdt build

http://tinderbox.mozilla.org/bonsai/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1123803540&maxdate=1123809479

Comment 37

[Aaron Leventhal]

Okay, this is not the bug that caused the tab navigation regresiion. Sorry for
the false alarm.

Comment 38

[Gérard Talbot]

Can someone try attachment 112654 [details] from bug 161061? I keep crashing. But I don't
crash with Seamonkey 1.0a rv 1.8b4 build 2005081106.
I also crashed with a local testcase, same testcase.
3 crashes reported so far and data transferred.

Deer Park alpha 2 rv:1.8b4 build 2005081208 under XP Pro SP 2 here.

Comment 39

[Johnny Stenback (:jst)]

Attachment: Fix framepage crashes.

This removes the crashing (and wrong) code that tried to wrap an outer frame
window in the scope of the inner window in the frame, which ni some cases
didn't exist yet when we got here.

Comment 40

[Gérard Talbot]

I've crashed twice so far in less than 5 min apart when trying to load

http://www.w3.org/MarkUp/Test/HTML401/current/tests/sec16_2_2-BF-03.html

in Deer Park alpha 2 rv 1.8b4 build 2005081208 under XP Pro SP2. Both crashes
talkback incident data were sent and received.
I have a 3rd testcase which also crashes...

Comment 41

[Gérard Talbot]

http://www.w3.org/MarkUp/Test/HTML401/current/tests/16_2_2-BF-03.html
also crashes in Deer Park alpha 2 1.8b4 build 2005081208 when taht same testcase
does not crash in Seamonkey 1.0a rv 1.8b4 build 2005081106

Talkback incident crash data was reported and sent+received.

I have a local testcase which also crash. I am sure I've used it in a bugfile
here... but can't find it...

Comment 42

[Gérard Talbot]

I've crashed 3 times when Shift+Reload-ing (or Ctrl+F5) attachment 151503 [details]. Crash
talkback incident data was sent; only the 3rd crash was clearly indicated.

I'm not 100% sure this is related to this bug... 

Comment 43

[Hermann Schwab]

(In reply to comment #42)
> I've crashed 3 times when Shift+Reload-ing (or Ctrl+F5) attachment 151503 [details]
[edit]. Crash
> talkback incident data was sent; only the 3rd crash was clearly indicated.

wfm Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20050812 Firefox/1.0+

Downloaded from pacifica tinderbox should be BuildID: 2005081222, as the files
are dated 22:09, but Windows Explorer tells Properties: Version 1.8b4: 2005081207

I assume your Talkbacks are here: 
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=comments&match=contains&searchfor=296639
TB8325763 	 nsWindowSH::GetProperty

275 crashes found where the Stack Signature contains 'nswindowsh::getproperty' 
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=+nsWindowSH%3A%3AGetProperty

I would prefer that you copy&paste the TalkbackIDs to the bug report, so I don't
have to search myself.

Comment 44

[Gérard Talbot]

> I would prefer that you copy&paste the TalkbackIDs to the bug report, so I don't
> have to search myself.

I understand and agree. I had to run last night. I couldn't do it right away
because I couldn't find the right talkback.exe. IMO, only 5 pages are
responsible for all these crashes.

In the upcoming attachment are all the talkback incident id crashes (21 crashes
in 3 hours) and all comments (first 30 characters or first line only) related to
each of them.

Comment 45

[Gérard Talbot]

Attachment: 21 talkback id crashes with comments

21 talkback id crashes with dates, comments

Comment 46

[Gérard Talbot]

> Downloaded from pacifica tinderbox should be BuildID: 2005081222, as the files
> are dated 22:09, but Windows Explorer tells Properties: Version 1.8b4: 2005081207

I don't understand this: planet.mozilla.org still has a link to download from

ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2005-08-12-08-trunk

and a call "to do some thorough testing of both Firefox (...) working with
framesets, pop up windows (...)" for this specific bug.

In any case, like I mentioned, Seamonkey 1.0a rv 1.8b4 build 2005081106 did not
crash on any of these pages or testcases.

Comment 47

[Mike Shaver (:shaver emeritus)]

Comment on attachment 192552 [details] [diff] [review]
Fix framepage crashes.

sr=shaver

Comment 48

[Johnny Stenback (:jst)]

Framepage crash fix checked in on trunk and branch.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 192477 [details] [diff] [review]
Followup patch that was checked in (reviews pending).

>Index: content/base/public/nsIDocument.h

>    * document creation and during event handling) will run. Note that
>    * this is the *inner* window object.
>    */
>   virtual nsIScriptGlobalObject* GetScriptGlobalObject() const = 0;

Again, if mIsGoingAway is true, this method will return the _outer_ window. 
That seems highly undesirable.	I doubt we can do much about it for 1.8,
though.  :(

I don't believe you revved the IID of nsIDocument for this change.  I guess
we've revved it since, so it should be OK, right?

>Index: content/base/src/nsDocument.cpp
> nsDocument::GetDefaultView(nsIDOMAbstractView** aDefaultView)

>   nsCOMPtr<nsPIDOMWindow> win(do_QueryInterface(mScriptGlobalObject));
> 
>   if (win) {
>+    nsPIDOMWindow *outer = win->GetOuterWindow();

Why not just use GetWindow() here?

>Index: content/events/src/nsEventStateManager.cpp

We should be able to remove GetDocumentOuterWindow in favor of GetWindow(), I
think.

>Index: dom/src/base/nsFocusController.cpp

Should be able to use GetWindow() in GetWindowFromDocument, I think.

> Index: content/events/src/nsEventListenerManager.cpp

Should probably use GetWindow() in FixContextMenuEvent.

And assert that the window we set mutation event bits on is an inner in
AddEventListener

The last either looks good or has been fixed since  Sorry I took so long to get
to this....

I've filed bug 311830 on the cleanup issues I raised here.

Comment 50

[Boris Zbarsky [:bzbarsky]]

And filed bug 311831 on the GetScriptGlobalObject issue.

Comment 51

[Boris Zbarsky [:bzbarsky]]

This is fixed.