Use BrowserAccessibilityComWin::Destroy to clear delegate and owner.

Use BrowserAccessibilityComWin::Destroy to clear delegate and owner.

BUG=703369
Review-Url: https://codereview.chromium.org/2873683003
Cr-Commit-Position: refs/heads/master@{#470760}
Committed: https://chromium.googlesource.com/chromium/src/+/225d81472e1c740fe8719b3bfc99612bc89fed36

[dougt, 2017-05-09 19:00:48]

dougt@chromium.org changed reviewers:
+ dmazzoni@chromium.org

[dougt, 2017-05-09 19:00:48]

ptal, follow ups will allow us to get rid of SetOwner all together.

[dmazzoni, 2017-05-10 06:15:50]

https://codereview.chromium.org/2873683003/diff/1/content/browser/accessibili...
File content/browser/accessibility/browser_accessibility_com_win.h (right):

https://codereview.chromium.org/2873683003/diff/1/content/browser/accessibili...
content/browser/accessibility/browser_accessibility_com_win.h:750: void detach()
{
I think this is the same as AXPlatformNode::Destroy(), can't you override that?

I'm fine with renaming it Detach() or Release()

[dougt, 2017-05-10 07:16:36]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 07:17:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dougt, 2017-05-10 08:28:52]

Yup, this make sense.

[dougt, 2017-05-10 08:29:20]

Description was changed from

==========
Adds BrowserAccessibilityComWin::detach which clears delegate and owner.

BUG=703369
==========

to

==========
Use BrowserAccessibilityComWin::Destory to clear delegate and owner.

BUG=703369
==========

[commit-bot: I haz the power, 2017-05-10 09:09:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 09:09:49]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[dougt, 2017-05-10 10:23:10]

Description was changed from

==========
Use BrowserAccessibilityComWin::Destory to clear delegate and owner.

BUG=703369
==========

to

==========
Use BrowserAccessibilityComWin::Destroy to clear delegate and owner.

BUG=703369
==========

[dougt, 2017-05-10 10:24:52]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 10:25:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 11:11:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 11:11:48]

Dry run: This issue passed the CQ dry run.

[dmazzoni, 2017-05-10 15:17:52]

https://codereview.chromium.org/2873683003/diff/60001/content/browser/accessi...
File content/browser/accessibility/browser_accessibility_com_win.cc (right):

https://codereview.chromium.org/2873683003/diff/60001/content/browser/accessi...
content/browser/accessibility/browser_accessibility_com_win.cc:4105:
AXPlatformNode::Destroy();
You definitely want AXPlatformNodeWin::Destroy() here otherwise you're
shadowing useful behvarior.

Do you need to clear delegate_? I think the superclass should do that.
Maybe DCHECK(!delegate_) instead.

[dougt, 2017-05-10 15:42:27]

On 2017/05/10 15:17:52, dmazzoni wrote:
>
https://codereview.chromium.org/2873683003/diff/60001/content/browser/accessi...
> File content/browser/accessibility/browser_accessibility_com_win.cc (right):
> 
>
https://codereview.chromium.org/2873683003/diff/60001/content/browser/accessi...
> content/browser/accessibility/browser_accessibility_com_win.cc:4105:
> AXPlatformNode::Destroy();
> You definitely want AXPlatformNodeWin::Destroy() here otherwise you're
> shadowing useful behvarior.

Done.

> Do you need to clear delegate_? I think the superclass should do that.
> Maybe DCHECK(!delegate_) instead.

Yes.  The delegate isn't cleared anywhere else.  Maybe it should be?

For example, in the case where there is only one BAW object holding onto the com
object, destrution happens like this:

>	accessibility.dll!ui::AXPlatformNodeBase::~AXPlatformNodeBase() Line 197	C++
 	accessibility.dll!ui::AXPlatformNodeWin::~AXPlatformNodeWin() Line 198	C++
 	content.dll!content::BrowserAccessibilityComWin::~BrowserAccessibilityComWin()
Line 414	C++
 	[External Code]	
 	content.dll!content::BrowserAccessibilityWin::Destroy() Line 43	C++

Line 197 (top of the stop) is a check to ensure that delegate_ was cleared prior
to the call.  Someone (probably BAW) needs to clear it.

[dmazzoni, 2017-05-10 16:21:07]

On 2017/05/10 15:42:27, dougt wrote:
> > Do you need to clear delegate_? I think the superclass should do that.
> > Maybe DCHECK(!delegate_) instead.
> 
> Yes.  The delegate isn't cleared anywhere else.  Maybe it should be?

It looks like it's cleared in AXPlatformNodeBase::Destroy(), no?

[dougt, 2017-05-10 19:22:12]

The CQ bit was checked by dougt@chromium.org to run a CQ dry run

[dougt, 2017-05-10 19:23:04]

On 2017/05/10 16:21:07, dmazzoni wrote:
> On 2017/05/10 15:42:27, dougt wrote:
> > > Do you need to clear delegate_? I think the superclass should do that.
> > > Maybe DCHECK(!delegate_) instead.
> > 
> > Yes.  The delegate isn't cleared anywhere else.  Maybe it should be?
> 
> It looks like it's cleared in AXPlatformNodeBase::Destroy(), no?

This is a much cleaner approach.  Sorry for the back and forth!

[commit-bot: I haz the power, 2017-05-10 19:23:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dmazzoni, 2017-05-10 19:29:27]

lgtm

[commit-bot: I haz the power, 2017-05-10 20:59:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 20:59:20]

Dry run: This issue passed the CQ dry run.

[dougt, 2017-05-10 23:54:21]

The CQ bit was checked by dougt@chromium.org

[commit-bot: I haz the power, 2017-05-10 23:55:19]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-11 00:07:58]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1494460461797930,
"parent_rev": "c714b929856634f69e66588db294ccb8b785ad25", "commit_rev":
"225d81472e1c740fe8719b3bfc99612bc89fed36"}

[commit-bot: I haz the power, 2017-05-11 00:08:09]

Description was changed from

==========
Use BrowserAccessibilityComWin::Destroy to clear delegate and owner.

BUG=703369
==========

to

==========
Use BrowserAccessibilityComWin::Destroy to clear delegate and owner.

BUG=703369

Review-Url: https://codereview.chromium.org/2873683003
Cr-Commit-Position: refs/heads/master@{#470760}
Committed:
https://chromium.googlesource.com/chromium/src/+/225d81472e1c740fe8719b3bfc99...
==========

[commit-bot: I haz the power, 2017-05-11 00:08:10]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/225d81472e1c740fe8719b3bfc99...