>- mScriptContext = aScriptContext; > if (aOwnerWindow) { >- mOwner = aOwnerWindow->IsOuterWindow() ? >- aOwnerWindow->GetCurrentInnerWindow() : aOwnerWindow; >+ BindToOwner(aOwnerWindow->IsOuterWindow() ? >+ aOwnerWindow->GetCurrentInnerWindow() : aOwnerWindow); > } else { >- mOwner = nsnull; >+ BindToOwner(aOwnerWindow); > } BindToOwner(aOwnerWindow); makes us use the right version of BindToOwner(nsnull). > nsXMLHttpRequest::GetLoadGroup() const > { >- if (mState & XML_HTTP_REQUEST_BACKGROUND) { >+ if (mState & XML_HTTP_REQUEST_BACKGROUND) { Apparently I added few extra spaces here. I'll fix. The idea with the patch is the nsDOMEventTargetHelper objects, like XHR, doesn't own the window or relevant scriptcontext anymore. Instead the nsDOMEventTargetHelper::mOwner <-> nsGlobalWindow connection is a raw pointer and cleared when either one of the objects is deleted. mHasHadOwner is there to prevent event handling on objects which don't have mOwner anymore (shouldn't change any behavior, since before the patch owner shouldn't be the current inner window anymore, if eventtargethelper is the only one which owns it, and in that case CheckInnerWindowCorrectness would fail.).

Why not use an nsIWeakReference to the window? That seems like it would be significantly simpler.

And *significantly* slower. Accessing mOwner must be fast.

Just to clarify, this is the other part of Bug 720512.

https://tbpl.mozilla.org/?tree=Try&rev=4f503fe12fe8 Minor, but hopefully effective changes: disconnect also onfoo handlers, and disconnect when calling CleanUp in nsGlobalWindow. Andrew, could you try this with gmail? The onfoo changes in this patch are temporary. onfoo handling in eventtargethelpers will be fixed soon. But perhaps we could get this into FF13

nsWebSocket::DisconnectFromOwner() could also call DontKeepAliveAnyMore

Still hoping to get this all to FF13. There are cases when XHR/WebSocket/EventSource is kept alive too long, and that leads to horrible CC times.

Comment on attachment 604870 [details] [diff] [review] +DontKeepAliveAnyMore - In content/events/src/nsDOMEventTargetHelper.h: + bool HasHadOwner() { return mHasHadOwner; } Maybe name this "HasOrHadOwner", or "HasOrHasHadOwner", cause "HasHadOwner" means something other than what this code does? - In nsGlobalWindow::AddEventTargetObject(): +{ + mEventTargetObjects.PutEntry(aObject); Seems making nsDOMEventTargetHelper inherit PRCList would serve us better here than sticking these in a hash. Faster insertions and removal, trivial enumeration, and lower memory overhead. Not sure if you want to change that in this bug or in a followup... r=jst, and bent is looking at the IndexedDB changes here too.

Comment on attachment 604870 [details] [diff] [review] +DontKeepAliveAnyMore Review of attachment 604870 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine other than this. ::: dom/indexedDB/IDBRequest.cpp @@ +142,5 @@ > } > } > else { > + nsIScriptContext* sc = GetContextForEventHandlers(&rv); > + NS_ENSURE_STATE(sc); I don't really understand this, you're not checking the nsresult here. I think you should either check it or not use/require that param.

Well, that is a case where we need a script context. So, if we don't get one, just return. The rv parameter is for event handling where there are cases when we can't get any script context. In that case event handling is allowed to proceed only if rv isn't failure.

https://hg.mozilla.org/mozilla-central/rev/0c35057e2bb4 https://hg.mozilla.org/mozilla-central/rev/483fb2f1da11

Comment on attachment 605256 [details] [diff] [review] hasorhashad Review of attachment 605256 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsEventSource.cpp @@ +307,5 @@ > // Get the load group for the page. When requesting we'll add ourselves to it. > // This way any pending requests will be automatically aborted if the user > // leaves the page. > + nsresult rv; > + nsIScriptContext* sc = GetContextForEventHandlers(&rv); http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/events/nsIDOMEventTarget.idl#303 @note Caller *must* check the value of aRv.