Make nsDOMEventTargetHelper to not have strong pointer to window

RESOLVED FIXED in mozilla13

Status

()

RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

12 Branch
mozilla13
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 604023 [details] [diff] [review]
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.).
Attachment #604023 - Flags: review?(jst)
Why not use an nsIWeakReference to the window?  That seems like it would be significantly simpler.
(Assignee)

Comment 2

7 years ago
And *significantly* slower. Accessing mOwner must be fast.
(Assignee)

Comment 3

7 years ago
Just to clarify, this is the other part of Bug 720512.
(Assignee)

Comment 4

7 years ago
Created attachment 604556 [details] [diff] [review]
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
Attachment #604023 - Attachment is obsolete: true
Attachment #604556 - Flags: review?(jst)
Attachment #604556 - Flags: feedback?(continuation)
Attachment #604023 - Flags: review?(jst)
(Assignee)

Comment 5

7 years ago
nsWebSocket::DisconnectFromOwner() could also call
DontKeepAliveAnyMore
(Assignee)

Comment 6

7 years ago
Created attachment 604870 [details] [diff] [review]
+DontKeepAliveAnyMore
(Assignee)

Comment 7

7 years ago
Created attachment 604943 [details] [diff] [review]
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.
Attachment #604556 - Attachment is obsolete: true
Attachment #604943 - Flags: review?(jst)
Attachment #604556 - Flags: review?(jst)
Attachment #604556 - Flags: feedback?(continuation)
(Assignee)

Updated

7 years ago
Attachment #604870 - Flags: review?(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.
Attachment #604870 - Flags: review?(bent.mozilla)

Updated

7 years ago
Attachment #604870 - Flags: review?(jst) → review+

Updated

7 years ago
Attachment #604943 - Flags: review?(jst) → review+
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.
Attachment #604870 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 11

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

Comment 12

7 years ago
Created attachment 605256 [details] [diff] [review]
hasorhashad
(Assignee)

Comment 13

7 years ago
https://hg.mozilla.org/mozilla-central/rev/0c35057e2bb4
https://hg.mozilla.org/mozilla-central/rev/483fb2f1da11
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
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.
(Assignee)

Updated

7 years ago
Depends on: 735305
You need to log in before you can comment on or make changes to this bug.