734057 - Make nsDOMEventTargetHelper to not have strong pointer to window

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

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

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

nsWebSocket::DisconnectFromOwner() could also call
DontKeepAliveAnyMore

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: +DontKeepAliveAnyMore

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: additional patch

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 8

[Olli Pettay [:smaug][[e-mail]]]

https://tbpl.mozilla.org/?tree=Try&rev=5232fed3dd3a

Comment 9

[Johnny Stenback (:jst)]

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 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: hasorhashad

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.