852804 - a11y::SelectionManager shouldn't hold a strong ref to focused element

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

This fixed a leak from bug 694254 locally but it didn't fix that leak on try server https://hg.mozilla.org/try/rev/889b90135ac7. So filing this as separate bug.

Comment 1

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #0)
> Created attachment 727009 [details] [diff] [review]
> patch
> 
> This fixed a leak from bug 694254 locally but it didn't fix that leak on try
> server https://hg.mozilla.org/try/rev/889b90135ac7. So filing this as
> separate bug.

honestly I'd bet that's random chance, I really doubt a11y is involved in that leak, from what I can tell right now we just leak some sql stuff and a bunch of atoms, and it seems hard to believe a11y can leak that without also leaking dom stuff which I don't see in the list of leaked objects.

if you think accessibility service is holding refs why don't you just add delete gAccService; to the end of the xpcom-shutdown observer and push it to try and then retrigger moth on win7 a bunch of times?

Comment 2

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> (In reply to alexander :surkov from comment #0)
> > Created attachment 727009 [details] [diff] [review]
> > patch
> > 
> > This fixed a leak from bug 694254 locally but it didn't fix that leak on try
> > server https://hg.mozilla.org/try/rev/889b90135ac7. So filing this as
> > separate bug.
> 
> honestly I'd bet that's random chance

I had it leaking permanently. This patch prevented it. That's what I observed. Maybe I was to unhappy reproducing this random :)

Regardless of this I doesn't really make sense to keep a strong ref on the content when we don't need it.

>, I really doubt a11y is involved in
> that leak,

yes, that's what I always said :)

> if you think accessibility service is holding refs why don't you just add
> delete gAccService; to the end of the xpcom-shutdown observer and push it to
> try and then retrigger moth on win7 a bunch of times?

I'm curious if accservice was guilty then wouldn't it make it appear in leaked classes?

Comment 3

[Trevor Saunders (:tbsaunde)]

> > if you think accessibility service is holding refs why don't you just add
> > delete gAccService; to the end of the xpcom-shutdown observer and push it to
> > try and then retrigger moth on win7 a bunch of times?
> 
> I'm curious if accservice was guilty then wouldn't it make it appear in
> leaked classes?

I'd hope so, but description seems as if you act as if accService is guilty.

Comment 4

[Trevor Saunders (:tbsaunde)]

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

I'm not exactly thrilled that we'll be including nsIFrame.h all over the place now :(

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> Comment on attachment 727009 [details] [diff] [review]
> patch
> 
> I'm not exactly thrilled that we'll be including nsIFrame.h all over the
> place now :(

me neither, I think we should replace inheritance on aggregation

Comment 6

[Joe Drew (not getting mail)]

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