Destory base class when win platform accessibily node is destroyed.

Destory base class when win platform accessibily node is destroyed.

Committed: https://crrev.com/0d6808d946ebaf4ad0e324b00ae32800f07feccc
Cr-Commit-Position: refs/heads/master@{#394219}

[mharanczyk, 2016-05-16 12:57:48]

mharanczyk@opera.com changed reviewers:
+ dmazzoni@chromium.org, nektar@chromium.org

[mharanczyk, 2016-05-16 12:57:49]

dmazzoni@, nektar@ this looks like regression caused by
https://codereview.chromium.org/1762143002. It looks like it causes two very
rare problems:
1. It triggers AtExitManager DCHECK on app shutdown after using accessibility.
2. Accessibility object is present until class deletion (and not destruction as
intended).

Since win platform accessibility node has its custom Destroy() method it doesn't
call base class AxPlatformNodeBase::Destroy() (which deletes itself), instead it
uses Release but does not call base's base class Destroy and does not remove
itself from unique id map until object is actually deleted (so AXPlatformNode
destructor removes it). This lingering id causes problem 1 during shutdown if
object is destroyed too late in shutdown sequence. It is probably also a cause
why some objects remain accessible for a while, ie. node parent can be reached
but parent already doesn't have this node in its tree.

Unfortunately I am not able to reproduce those issues reliably, but looking at
code prior to you changes it also removed id from unique map in Destroy(), so it
looks reasonable to still do it like this.
Could you please take a look and give me your opinion on this?

[dmazzoni, 2016-05-17 16:37:36]

Great fix, I think that would explain the behavior you're seeing. Thanks.

I've asked for a comment below so it doesn't look like a bug later,
but feel free to commit once you add that.

https://codereview.chromium.org/1978223002/diff/1/ui/accessibility/platform/a...
File ui/accessibility/platform/ax_platform_node_win.cc (right):

https://codereview.chromium.org/1978223002/diff/1/ui/accessibility/platform/a...
ui/accessibility/platform/ax_platform_node_win.cc:149:
AXPlatformNode::Destroy();
Please add a comment indicating that we're deliberately not calling
AXPlatformNodeBase::Destroy because that would delete this object, but
AXPlatformNode::Destroy is needed to remove the unique ID from the map.

[mharanczyk, 2016-05-17 17:14:58]

Added a comment. Since I am not committer I think I need you official approval
for this to land.

This design is a bit unfortunate, it means that both AXPlatformNodeBase::Destroy
and AXPlatformNodeWin::Destroy has to be somewhat synchronized. Maybe it would
be good to also add a comment in AXPlatformNodeBase::Destroy that any changes
there should probably be also reflected/repeated in AXPlatformNodeWin::Destroy?

[mharanczyk, 2016-05-17 17:23:06]

Or I could add AXPlatformNodeBase::Destroy(bool delete_itself) and make
AXPlatformNodeBase::Destroy call it with true, and call it with false from
AXPlatformNodeWin::Destroy, that way code duplication could be avoided. What do
you think?

[dmazzoni, 2016-05-17 19:09:42]

On 2016/05/17 17:23:06, mharanczyk wrote:
> Or I could add AXPlatformNodeBase::Destroy(bool delete_itself) and make
> AXPlatformNodeBase::Destroy call it with true, and call it with false from
> AXPlatformNodeWin::Destroy, that way code duplication could be avoided. What
do
> you think?

I agree the design could be better.

Let's land this change first. Then rather than passing a boolean I think it
might be
cleaner to separate "Destroy" into two concepts: Detach and Dereference.

Detach() means we're done with the AXPlatformNode and we no longer have any
references to it. It calls Dereference().

Dereference() will call Release() on platforms that do reference-counting,
but will just destroy the object on platforms that don't.

I think with that design it wouldn't ever be necessary to call the base's base
class.
What do you think?

[dmazzoni, 2016-05-17 19:09:49]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2016-05-17 19:09:49]

lgtm

[commit-bot: I haz the power, 2016-05-17 19:10:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1978223002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1978223002/20001

[commit-bot: I haz the power, 2016-05-17 20:40:28]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-17 20:42:21]

Description was changed from

==========
Destory base class when win platform accessibily node is destroyed.
==========

to

==========
Destory base class when win platform accessibily node is destroyed.

Committed: https://crrev.com/0d6808d946ebaf4ad0e324b00ae32800f07feccc
Cr-Commit-Position: refs/heads/master@{#394219}
==========

[commit-bot: I haz the power, 2016-05-17 20:42:22]

Patchset 2 (id:??) landed as
https://crrev.com/0d6808d946ebaf4ad0e324b00ae32800f07feccc
Cr-Commit-Position: refs/heads/master@{#394219}