To add some more COM context: any COM AddRef() needs to be followed by a Release() call when it is no longer needed as COM objects are refcounted. We didn't see a Release() call in a quick search of the code.

When COM objects are created, it is COM convention to return the object with a refcount of 1. An AddRef is not required to indicate the object is alive

The ownership model is as such:

BrowserAccessibilityWin owns a BrowserAccessibilityCOMWin.  BrowserAccessibilityCOMWin is a COM object that can exist beyond the lifetime of BrowserAccessibilityWin.  BrowserAccessibilityCOMWin is handled out to AT and they may hold onto a reference.

This call:

 browser_accessibility_com_->AddRef();

ensures that BrowserAccessibilityWin owns a reference.

This call:

  browser_accessibility_com_->Init(this);

sets up a delegate in BrowserAccessibilityCOMWin.


Can you reproduce any leaks locally?  Do you know any more about this user?  For example, are they running any AT, IME, password managers, etc that use our accessibility APIs?



Nektar, I wonder if this leak is actually: https://bugs.chromium.org/p/chromium/issues/detail?id=791027

> browser_accessibility_com_->AddRef();
I believe that rob was saying that CreateInstance() returns an object with a ref-count of 1 already.

> browser_accessibility_com_->Init(this);
Where is the corresponding release ref call? Setting a delegate doesn't really mean anything other than setting up a future communication channel.


> Can you reproduce any leaks locally?  Do you know any more about this user?  For example, are they running any AT, IME, password managers, etc that use our accessibility APIs?

No, I have not attempted to repro, and we have no further user info [would be a privacy issue].

According to the documentation for CComObject, CreateInstance returns an object with ref count = 1, so the AddRef() appears correct. 
https://msdn.microsoft.com/en-us/library/9e31say1.aspx

Where is the corresponding ReleaeseRef()? And is there any easy way to test this? We can just add some logging and see what [if any] objects are being leaked. 

We are missing a Release here:
    https://cs.chromium.org/chromium/src/content/browser/accessibility/browser_accessibility_win.cc?l=33

I think the assumption was that there is an operator= that would call Release



These objects are created and destroyed a bunch in our layout tests in all of the accessibility tests.  You can also go to:

chrome://accessibility/

And flip the flags for a given window.

I looked at it again and the call to Destroy ends up calling Release().

If you have a ComPtr store the pointer, you'll get the operator=() calling Release() behavior you expect. Holding raw COM pointers is error prone.

As an aside: I adding some logging to BrowserAccessibilityWin and BrowserAccessibilityComWin just to count number of live instances of both.

I ran chrome with "--force-renderer-accessibility"
Observations:
1) A page of cnn.com causes ~1800 live BrowserAccessibilityWin/BrowserAccessibilityComWin instances.

2) The page crashed [not sure if related to the flag]. The number of accessibility objects did not decrease. [Bug?] Closing the tab destroyed the corresponding accessibility objects.

3) I opened 3 tabs of cnn.com, and closed 3 tabs [also causing the last browser window to close]. All ~6k BrowserAccessibilityWin objects were destroyed, but there were still 23-live BrowserAccessibilityComWin objects. This seems to be a leak.

Takeaways:

The number of BrowserAccessibilityWin objects created seems *really* large for cnn.com. Is this correct? Is there any way to reduce this count? It could be that we're seeing a user with this flag enabled, with 10 tabs of CNN open.

I was able to repro the leaked objects on closing 3 tabs of cnn.com - so this seems like an easy-to-repro leak? Can you confirm?

> The number of BrowserAccessibilityWin objects created seems *really* large for cnn.com. Is this correct? Is there any way to reduce this count? It could be that we're seeing a user with this flag enabled, with 10 tabs of CNN open.

We need approximately one object per DOM node. Running document.querySelectorAll('*').length in the console window for cnn.com yields 1787, so I'd say we're in exactly the right order of magnitude.

> I was able to repro the leaked objects on closing 3 tabs of cnn.com - so this seems like an easy-to-repro leak? Can you confirm?

It sounds like if I'm understanding correctly, ~6k objects were destroyed but 23 leaked. I'd like to better understand what was different about those 23.

Given that this is pretty easy to repro [I just adding logging ot the destructor/constructor behind a lock], can you take it from here?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d6fdfe404304aaa1285199a8a34f855466f27543

commit d6fdfe404304aaa1285199a8a34f855466f27543
Author: Dominic Mazzoni <dmazzoni@chromium.org>
Date: Sat Dec 23 00:03:01 2017

Add leak checking to all AXPlatformNodeWin unit tests.

Bug:  793123 
Change-Id: I2ea589a52a600ae0f0a7e6e2afe00727a6acfacf
Reviewed-on: https://chromium-review.googlesource.com/843240
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: Doug Turner <dougt@chromium.org>
Cr-Commit-Position: refs/heads/master@{#526098}
[modify] https://crrev.com/d6fdfe404304aaa1285199a8a34f855466f27543/ui/accessibility/platform/ax_platform_node_win.cc
[modify] https://crrev.com/d6fdfe404304aaa1285199a8a34f855466f27543/ui/accessibility/platform/ax_platform_node_win.h
[modify] https://crrev.com/d6fdfe404304aaa1285199a8a34f855466f27543/ui/accessibility/platform/ax_platform_node_win_unittest.cc

On master, after browsing around with accessibility enabled (including cnn.com), i did see the # of BrowserAccessibilityWin left go to 0.

Hm. Given the small size of the potential leak, and the fact that you can't repro, let's close out this issue. 

I'll marked as fixed since Dominic did add some testing code to trap any leaks in testing.