Delay m_axObjectCache clearing during Document detach

Delay m_axObjectCache clearing during Document detach

While blink::Document::detach() is working, AXLayoutObject is re-created
after clearAXObjectCache() because chrome().focusedNodeChanged causes
access to document.accessibilityObject(). It causes
ASSERTION FAILED: !m_hasAXObject at ~LayoutObject().

This patch makes clearAXObjectCache() be called after
chrome().focusedNodeChanged is handled in order to avoid re-access after
clear AX objects.

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

[je_julie(Not used), 2015-04-23 14:47:42]

je_julie.kim@samsung.com changed reviewers:
+ dmazzoni@chromium.org, japhet@chromium.org

[je_julie(Not used), 2015-04-23 14:49:28]

Hi Dominic and Nate Chapin,

I can see ASSERTION FAILED related to Accessibility.
I updated the callstack which causes ASSERTION FAILED at crbug.com/480140.

When AXLayoutObject is created, setHasAXObject is set and AXLayoutObject is
destructed, setHasAXObject is reset.
While document is detached, m_hasAXObject should be false at ~LayoutObject()
because AXObjects are cleared before it.

Currently, AX objects are cleared and then 
frameHost()->chrome().focusedNodeChanged() is called in Document::detach().
chrome().focusedNodeChanged() causes AccessibilityFocusedNodeChanged() and it
calls HandleAXEvent() with document.accessibilityObject().
At this moment, document.accessibilityObject() creates a new AXObject.
That's why hasAXObject is true when layoutobject is destructed and it causes
ASSERTION FAILED.

This patch moves clearAXObjectCache() down focusedNodeChanged() because
clearAXObjectCache() should be called when AXObjects are not used any more.

PTAL.

[dmazzoni, 2015-04-23 17:01:55]

lgtm

Thanks for debugging this and for the excellent change description.

This seems like a good fix, none of the teardown that's happening between the
old and new location of the call to clearAXObjectCache should affect
accessibility.

[je_julie(Not used), 2015-04-24 03:52:15]

On 2015/04/23 17:01:55, dmazzoni wrote:
> lgtm
> 
> Thanks for debugging this and for the excellent change description.
> 
> This seems like a good fix, none of the teardown that's happening between the
> old and new location of the call to clearAXObjectCache should affect
> accessibility.

Thanks, Dominic.
Yes. I can see there is no code to affect accessibility between old and new
location of it.

@Nate Chapin,
Could you take a look? I need owner's confirm.

[je_julie(Not used), 2015-04-24 14:35:23]

je_julie.kim@samsung.com changed reviewers:
+ sof@opera.com

[je_julie(Not used), 2015-04-24 14:35:24]

I think Nate is busy.
@sof, could you take a look?

[sof, 2015-04-27 06:27:07]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com
- sof@opera.com

[sof, 2015-04-27 06:27:07]

Change looks fine, but I wonder if we can push this one step further.

Cc:ing keishi who is busy looking at accessibility/ (wrt Oilpan.)

https://codereview.chromium.org/1089813004/diff/20001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1089813004/diff/20001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2155: if (this == &axObjectCacheOwner())
Does this mean that we no longer need to check & call clearAXObjectCache() in
~Document() ?

[je_julie(Not used), 2015-04-27 19:08:31]

@sof, thanks for your review.
I updated code. PTAL.

https://codereview.chromium.org/1089813004/diff/20001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1089813004/diff/20001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:2155: if (this == &axObjectCacheOwner())
On 2015/04/27 06:27:07, sof wrote:
> Does this mean that we no longer need to check & call clearAXObjectCache() in
> ~Document() ?
You're right.
I can see that commitIfReady() calls detach(), commitData() calls ~Document()
and commitData() is always done after commitIfReady().
I updated patchset to remove it from ~Document().

[sof, 2015-04-27 20:05:59]

Good, this takes care of a latent Oilpan bug -- calling axObjectCacheOwner()
from ~Document() is unsafe in the general case with it enabled. So, great to see
it removed :)

https://codereview.chromium.org/1089813004/diff/40001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1089813004/diff/40001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:531: ASSERT(!parentTreeScope());
Could you add

  ASSERT(!m_axObjectCache);

here, so that we can make sure that the invariant that Document::detach() clears
the cache, continues to hold?

https://codereview.chromium.org/1089813004/diff/40001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:562: #if !ENABLE(OILPAN)
Let's combine these two #if !ENABLE(OILPAN) blocks together.

[sof, 2015-04-27 20:15:10]

Could change the CL title to something like

 Delay AX object cache clearing during Document detach

?

[je_julie(Not used), 2015-04-27 23:21:59]

@sof, I updated the patchset and the CL title.
PTAL.

https://codereview.chromium.org/1089813004/diff/40001/Source/core/dom/Documen...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/1089813004/diff/40001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:531: ASSERT(!parentTreeScope());
On 2015/04/27 20:05:59, sof wrote:
> Could you add
> 
>   ASSERT(!m_axObjectCache);
> 
> here, so that we can make sure that the invariant that Document::detach()
clears
> the cache, continues to hold?

Done.

https://codereview.chromium.org/1089813004/diff/40001/Source/core/dom/Documen...
Source/core/dom/Document.cpp:562: #if !ENABLE(OILPAN)
On 2015/04/27 20:05:59, sof wrote:
> Let's combine these two #if !ENABLE(OILPAN) blocks together.

Done.

[haraken, 2015-04-28 01:18:38]

This is basically a nice change, but unfotunately Document::detach() is not
guaranteed to be called for all documents... See the comment on
DocumentLifecycleNotifier::notifyContextDestroyed() in Document::detach().

[sof, 2015-04-28 05:10:33]

On 2015/04/28 01:18:38, haraken wrote:
> This is basically a nice change, but unfotunately Document::detach() is not
> guaranteed to be called for all documents... See the comment on
> DocumentLifecycleNotifier::notifyContextDestroyed() in Document::detach().

Could such a thing have an AXObjectCache without being attached to a frame (=>
it will get a detach)? i.e., axObjectCache() returns 0 if there's no frame
attached afaict.

[sof, 2015-04-28 07:27:39]

lgtm, let's try this.

if haraken wants to override and suggest keeping the clearing in the dtor,
please do.

[je_julie(Not used), 2015-04-28 10:33:46]

On 2015/04/28 07:27:39, sof wrote:
> lgtm, let's try this.
> 
> if haraken wants to override and suggest keeping the clearing in the dtor,
> please do.

Thanks, sof.
@haraken, WDYT? Can I try to land it?

[je_julie(Not used), 2015-04-28 10:34:43]

je_julie.kim@samsung.com changed reviewers:
+ haraken@chromium.org
- japhet@chromium.org

[haraken, 2015-04-28 10:36:57]

On 2015/04/28 10:33:46, je_julie wrote:
> On 2015/04/28 07:27:39, sof wrote:
> > lgtm, let's try this.
> > 
> > if haraken wants to override and suggest keeping the clearing in the dtor,
> > please do.
> 
> Thanks, sof.
> @haraken, WDYT? Can I try to land it?

Let's keep the clearAXObjectCache in the destructor in this CL. Then LGTM.

After landing this CL, we can try to insert an ASSERT to verify that
Document::detach is always called before Document's destructor is called. (I
guess we'll hit the ASSERT, but this is something we need to fix to ship Oilpan
for accessibility/.)

[sof, 2015-04-28 10:38:53]

On 2015/04/28 10:36:57, haraken wrote:
> On 2015/04/28 10:33:46, je_julie wrote:
> > On 2015/04/28 07:27:39, sof wrote:
> > > lgtm, let's try this.
> > > 
> > > if haraken wants to override and suggest keeping the clearing in the dtor,
> > > please do.
> > 
> > Thanks, sof.
> > @haraken, WDYT? Can I try to land it?
> 
> Let's keep the clearAXObjectCache in the destructor in this CL. Then LGTM.
> 
> After landing this CL, we can try to insert an ASSERT to verify that
> Document::detach is always called before Document's destructor is called. (I
> guess we'll hit the ASSERT, but this is something we need to fix to ship
Oilpan
> for accessibility/.)

Given https://codereview.chromium.org/1089813004/#msg14 , I'm not sure that's
true. But we can keep these two apart, and land the good fix in detach().

[haraken, 2015-04-28 10:47:20]

On 2015/04/28 05:10:33, sof wrote:
> On 2015/04/28 01:18:38, haraken wrote:
> > This is basically a nice change, but unfotunately Document::detach() is not
> > guaranteed to be called for all documents... See the comment on
> > DocumentLifecycleNotifier::notifyContextDestroyed() in Document::detach().
> 
> Could such a thing have an AXObjectCache without being attached to a frame (=>
> it will get a detach)? i.e., axObjectCache() returns 0 if there's no frame
> attached afaict.

ah, that's a good point. (Sorry, this comment is not emailed to me for some
reason.)

> Given https://codereview.chromium.org/1089813004/#msg14 , I'm not sure that's
> true. But we can keep these two apart, and land the good fix in detach().

Yes, sounds better. We can take a safer way.

[je_julie(Not used), 2015-04-28 11:31:04]

On 2015/04/28 10:47:20, haraken wrote:
> On 2015/04/28 05:10:33, sof wrote:
> > Given https://codereview.chromium.org/1089813004/#msg14 , I'm not sure
that's
> > true. But we can keep these two apart, and land the good fix in detach().
> 
> Yes, sounds better. We can take a safer way.

"we can keep these two apart" means that we keep clearAXObjectCache() at both of
dtor and detach? If then, I'll update patchset again.

[sof, 2015-04-28 11:34:32]

On 2015/04/28 11:31:04, je_julie wrote:
> On 2015/04/28 10:47:20, haraken wrote:
> > On 2015/04/28 05:10:33, sof wrote:
> > > Given https://codereview.chromium.org/1089813004/#msg14 , I'm not sure
> that's
> > > true. But we can keep these two apart, and land the good fix in detach().
> > 
> > Yes, sounds better. We can take a safer way.
> 
> "we can keep these two apart" means that we keep clearAXObjectCache() at both
of
> dtor and detach? If then, I'll update patchset again.

Patchset #2 effectively.

[je_julie(Not used), 2015-04-28 11:42:45]

On 2015/04/28 11:34:32, sof wrote:
> On 2015/04/28 11:31:04, je_julie wrote:
> > On 2015/04/28 10:47:20, haraken wrote:
> > > On 2015/04/28 05:10:33, sof wrote:
> > > > Given https://codereview.chromium.org/1089813004/#msg14 , I'm not sure
> > that's
> > > > true. But we can keep these two apart, and land the good fix in
detach().
> > > 
> > > Yes, sounds better. We can take a safer way.
> > 
> > "we can keep these two apart" means that we keep clearAXObjectCache() at
both
> of
> > dtor and detach? If then, I'll update patchset again.
> 
> Patchset #2 effectively.

Is there any way to land patchset#2 on the current status?
Or, do I have to upload it again?

[haraken, 2015-04-28 11:43:39]

On 2015/04/28 11:42:45, je_julie wrote:
> On 2015/04/28 11:34:32, sof wrote:
> > On 2015/04/28 11:31:04, je_julie wrote:
> > > On 2015/04/28 10:47:20, haraken wrote:
> > > > On 2015/04/28 05:10:33, sof wrote:
> > > > > Given https://codereview.chromium.org/1089813004/#msg14 , I'm not sure
> > > that's
> > > > > true. But we can keep these two apart, and land the good fix in
> detach().
> > > > 
> > > > Yes, sounds better. We can take a safer way.
> > > 
> > > "we can keep these two apart" means that we keep clearAXObjectCache() at
> both
> > of
> > > dtor and detach? If then, I'll update patchset again.
> > 
> > Patchset #2 effectively.
> 
> Is there any way to land patchset#2 on the current status?
> Or, do I have to upload it again?

The easiest way would be to reupload :)

[je_julie(Not used), 2015-04-28 11:57:01]

On 2015/04/28 11:43:39, haraken wrote:
> On 2015/04/28 11:42:45, je_julie wrote:
> > On 2015/04/28 11:34:32, sof wrote:
> > > Patchset #2 effectively.
> > 
> > Is there any way to land patchset#2 on the current status?
> > Or, do I have to upload it again?
> 
> The easiest way would be to reupload :)

@sof, @haraken,
Thanks for your time and review.
I uploaded patchset to keep clearAXObjectCache() at ~Document().
Can I try to land it?

[haraken, 2015-04-28 11:57:26]

On 2015/04/28 11:57:01, je_julie wrote:
> On 2015/04/28 11:43:39, haraken wrote:
> > On 2015/04/28 11:42:45, je_julie wrote:
> > > On 2015/04/28 11:34:32, sof wrote:
> > > > Patchset #2 effectively.
> > > 
> > > Is there any way to land patchset#2 on the current status?
> > > Or, do I have to upload it again?
> > 
> > The easiest way would be to reupload :)
> 
> @sof, @haraken,
> Thanks for your time and review.
> I uploaded patchset to keep clearAXObjectCache() at ~Document().
> Can I try to land it?

Sure, thanks.

[haraken, 2015-04-28 11:57:30]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-04-28 11:57:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, haraken@chromium.org, sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/1089813004/#ps80001
(title: "Keep clearAXObjectCache() at ~Document()")

[commit-bot: I haz the power, 2015-04-28 11:57:35]

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

[commit-bot: I haz the power, 2015-04-28 13:22:41]

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