Closed Bug 734057 Opened 8 years ago Closed 8 years ago

Make nsDOMEventTargetHelper to not have strong pointer to window

Categories

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

12 Branch
x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
>-  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.
And *significantly* slower. Accessing mOwner must be fast.
Just to clarify, this is the other part of Bug 720512.
Attached patch v3 (obsolete) — Splinter Review
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)
nsWebSocket::DisconnectFromOwner() could also call
DontKeepAliveAnyMore
Attached patch additional patchSplinter Review
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)
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)
Attachment #604870 - Flags: review?(jst) → review+
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+
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.
Attached patch hasorhashadSplinter Review
https://hg.mozilla.org/mozilla-central/rev/0c35057e2bb4
https://hg.mozilla.org/mozilla-central/rev/483fb2f1da11
Status: NEW → RESOLVED
Closed: 8 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.
Depends on: 735305
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.