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.

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.... :(

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

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

yay!

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

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...

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.

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 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=).

(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

What's happening about the test failure on tinderbox?

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.

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.

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).

(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 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.

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

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 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?

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

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

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).

>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.

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 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 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...)

(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 on attachment 192421 [details] [diff] [review] Cleanup based on bz's review feedback. See previous comments for discussion about what's being changed here.

Does that fix bug #302885 as well?

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.

(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.

(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

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

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.

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.

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...

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...

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...

(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.

> 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.

21 talkback id crashes with dates, comments

> 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 on attachment 192552 [details] [diff] [review] Fix framepage crashes. sr=shaver

Framepage crash fix checked in on trunk and branch.

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.

And filed bug 311831 on the GetScriptGlobalObject issue.

This is fixed.