846204 - unregister DocManager from DOM event listeners

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

spun off bug 694254 (also see bug 694254 comment #64).

1) DocManager is not cycle collected
2) Document and document accessible may have different life cycles (it doesn't make sense to keep listeners if we don't use them)

Comment 1

[Trevor Saunders (:tbsaunde)]

Comment on attachment 719352 [details] [diff] [review]
patch

>+DocManager::RemoveListeners(nsIDocument* aDocument)
>+{
>+  nsPIDOMWindow* window = aDocument->GetWindow();
>+  nsIDOMEventTarget* target = window->GetChromeEventHandler();
>+  nsEventListenerManager* elm = target->GetListenerManager(true);

does it really make sense to force creation of the listener manager?  if we just created it how can it be holding a ref to us?

btw would it make more sense to do this in DocAccessible::Shutdown() so you can just grab the elm off the pres shell you already have?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Comment on attachment 719352 [details] [diff] [review]
> patch
> 
> >+DocManager::RemoveListeners(nsIDocument* aDocument)
> >+{
> >+  nsPIDOMWindow* window = aDocument->GetWindow();
> >+  nsIDOMEventTarget* target = window->GetChromeEventHandler();
> >+  nsEventListenerManager* elm = target->GetListenerManager(true);
> 
> does it really make sense to force creation of the listener manager?  if we
> just created it how can it be holding a ref to us?

makes sense, just copy/paste

> btw would it make more sense to do this in DocAccessible::Shutdown() so you
> can just grab the elm off the pres shell you already have?

I'd like to keep removal where they were added. If DocManager adds listeners then it should take care to remove them. To stay symmetrical.

Comment 3

[Ed Morley [:emorley]]

Is this ready to land?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Ed Morley [:edmorley UTC+0] from comment #3)
> Is this ready to land?

yes, it is

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d253208c90bc

*fingers crossed* - thanks for the patch, Alexander!

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d253208c90bc