Fix leaking AXNodeObjects when sub document detaches

Fix leaking AXNodeObjects when sub document detaches

Node::willBeDeletedFromDocument() is called after the Document has been detached so axObjectCacheOwner() is already disconnected and the node was failing to be removed from the cache.

Test will be added in https://codereview.chromium.org/1202263005/

BUG=502782
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197755

Committed: https://crrev.com/f8161f9b3e993134f47f3b1d296a4f4a24e569f1
Cr-Commit-Position: refs/heads/master@{#359820}

[keishi, 2015-06-24 11:38:17]

keishi@chromium.org changed reviewers:
+ dmazzoni@chromium.org, haraken@chromium.org

[haraken, 2015-06-24 11:52:13]

LGTM

https://codereview.chromium.org/1207613004/diff/1/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1207613004/diff/1/Source/core/dom/Document.cp...
Source/core/dom/Document.cpp:2190: } else if (AXObjectCache* cache =
existingAXObjectCache()) {

Maybe worth having a comment about the relationship between this document and
the AXObjectCache. AXObjectCache and Document are not in a 1:1 relationship.

[dmazzoni, 2015-06-24 14:21:53]

lgtm

[keishi, 2015-06-24 15:07:47]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2015-06-24 15:08:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1207613004/1

[commit-bot: I haz the power, 2015-06-24 16:16:48]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=197755

[amineer_google, 2015-06-26 15:35:10]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1208303003/ by amineer@google.com.

The reason for reverting is: Top crasher in canary - bug 504315..

[keishi, 2015-06-29 14:58:06]

I can't reproduce the crash and I can't quite figure out what's wrong. @dmazzoni
could you comment on my ideas?

One theory I have is that LayoutObject assumes that AXObjectCache::remove(Node*)
is called after AXObjectCache::remove(LayoutObject*).
The comments in LayoutObject::willBeDestroyed says
// For accessibility management, notify the parent of the imminent change to its
child set.
// We do it now, before remove(), while the parent pointer is still available.
Somehow calling remove before childrenChanged is causing it to create ancestor
AXObjects and hitting a stale pointer.
We can move the removals in Document::detach after ContainerNode::detach is
called.
https://codereview.chromium.org/1213203002

Another theory is that AXObject::childrenChanged shouldn't be creating new
AXObjects at all.
ancestorForWhichThisIsAPresentationalChild() is used in
AXNodeObject::childrenChanged so it can skip walking the entire ancestor chain.
However ancestorForWhichThisIsAPresentationalChild() is calling parentObject()
which might create a parent object. If parentObjectIfExists is null there is no
ancestor chain to walk. In that case isn't calling parentObject() just adding
work? This CL changes ancestorForWhichThisIsAPresentationalChild so it only uses
parentObjectIfExists.
https://codereview.chromium.org/1214183002

[keishi, 2015-11-16 08:55:24]

The CQ bit was checked by keishi@chromium.org

[keishi, 2015-11-16 08:55:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1207613004/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2015-11-16 08:55:39]

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

[keishi, 2015-11-16 08:56:30]

I'm going to reland with the first speculative fix and see how it goes.

[haraken, 2015-11-16 09:04:08]

On 2015/11/16 08:56:30, keishi wrote:
> I'm going to reland with the first speculative fix and see how it goes.

LGTM

[commit-bot: I haz the power, 2015-11-16 11:36:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-16 11:37:36]

Patchset 4 (id:??) landed as
https://crrev.com/f8161f9b3e993134f47f3b1d296a4f4a24e569f1
Cr-Commit-Position: refs/heads/master@{#359820}